Propagate sampling_priority
#248
Conversation
|
|
||
| ctx ||= call_context |
There was a problem hiding this comment.
^ that call_context was creating buggy behaviors in corner cases, it broke some tests as I implemented the id propagation using the context. Those tests were fixed when removing this call and creating a fresh context when none is given. Which is what we do in Python BTW.
| @@ -264,7 +264,7 @@ def test_start_span_child_of_context | |||
| thread = Thread.new do | |||
| mutex.synchronize do | |||
| @thread_span = tracer.start_span('a') | |||
| @thread_ctx = tracer.call_context | |||
| @thread_ctx = @thread_span.context | |||
There was a problem hiding this comment.
Nasty, but yes, getting the span context is the right thing to do here.
| @sampled = false | ||
| @parent_trace_id = options.fetch(:trace_id, nil) | ||
| @parent_span_id = options.fetch(:span_id, nil) | ||
| @sampled = options.fetch(:sampled, false) |
There was a problem hiding this comment.
I'm mixed here. Python implementation uses true by default. We could change this, it possibly breaks some internal tests, but none of them should impact behavior from a user POV. AFAIK client sampling is not really massively used today in Ruby, and as a side note, first span added would turn this to true. But I think it's worth noticing.
lib/ddtrace/distributed_headers.rb
Outdated
| def sampling_priority | ||
| hdr = header(HTTP_HEADER_SAMPLING_PRIORITY) | ||
| # It's important to make a difference between no header, | ||
| # and a header defined to zero. |
There was a problem hiding this comment.
This is required because some hosts might have priority sampling disabled and/or have outdated libraries so they are not propagating the information. In this case, we want to just ignore it and not set it.
lib/ddtrace/distributed_headers.rb
Outdated
| def sampling_priority | ||
| hdr = header(HTTP_HEADER_SAMPLING_PRIORITY) | ||
| # It's important to make a difference between no header, | ||
| # and a header defined to zero. |
lib/ddtrace/distributed_headers.rb
Outdated
| headers[HTTP_HEADER_SAMPLING_PRIORITY] = span.sampling_priority.to_s | ||
| end | ||
| env.merge! headers | ||
| env.delete(HTTP_HEADER_SAMPLING_PRIORITY) unless span.sampling_priority |
There was a problem hiding this comment.
Is there a reason for this #delete?
There was a problem hiding this comment.
I did it on purpose indeed, my sense was that, if the headers say: there's no sampling priority, it should actually be removed. After all that function is named inject! so it sort of implies: take whatever step is needed so that this env, afterwards, reflects the state of the headers passed to it, including the fact that if it has no sampling priority, then this field should just not exist at all. It's really a corner case I think.
lib/ddtrace/distributed_headers.rb
Outdated
| env.delete(HTTP_HEADER_SAMPLING_PRIORITY) unless span.sampling_priority | ||
| end | ||
|
|
||
| def self.extract(env) |
There was a problem hiding this comment.
More a design nitpick, but I'd prefer to do less things here and keep DistributedHeaders more like as presenter around the environment object. I think the concept of a Context should belong to somewhere else (probably to whoever call this).
There was a problem hiding this comment.
OK, my sense is that this probably, indeed, does not belong to something like distributed_headers, true enough. How about, then, renaming it to, say, propagation/http as in https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/propagation/http.py Because, it's of general usage, it's proven useful in Python (extracting and injecting those values from headers to hash is quite a straightforward use case and many libraries may benefit from it, even if today only Faraday uses it).
There was a problem hiding this comment.
Generally I'd prefer to defer things like this until they're proven useful, but I'm pretty sure you can anticipate more use-cases and scenarios for it than I do, so I'm fine with it.
lib/ddtrace/context.rb
Outdated
| @finished_spans = 0 | ||
| @current_span = nil | ||
| end | ||
|
|
||
| def trace_id | ||
| @mutex.synchronize do | ||
| return @parent_trace_id |
There was a problem hiding this comment.
If you don't mind changing this, let's get rid of the return just to be more idiomatic.
| @@ -37,11 +64,21 @@ def current_span | |||
| end | |||
| end | |||
|
|
|||
| def set_current_span(span) | |||
lib/ddtrace/distributed_headers.rb
Outdated
| value | ||
| end | ||
|
|
||
| def self.inject!(span, env) |
There was a problem hiding this comment.
Since this is basically related to the faraday integration I would prefer to keep it there. I don't think we should add methods to the public API of this class unless it is reusable for multiple entities.
test/distributed_headers_test.rb
Outdated
| require 'ddtrace/span' | ||
| require 'ddtrace/distributed_headers' | ||
|
|
||
| class DistributedHeadersTest < Minitest::Test |
There was a problem hiding this comment.
👍
Sorry for leaving this behind!
Follow-up to #245
Most significant change here is that context is used to propagate the
trace_id,parent_idandsampling_priority. This makes it more alike the Python implementation and allows more code sharing.