-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
create propagate span event and publish it in to_digest #4193
base: master
Are you sure you want to change the base?
Conversation
Thank you for updating Change log entry section 👏 Visited at: 2024-12-05 12:04:27 UTC |
Datadog ReportBranch report: ✅ 0 Failed, 22114 Passed, 1475 Skipped, 5m 41.75s Total Time |
BenchmarksBenchmark execution time: 2024-12-16 18:57:27 Comparing candidate commit 5a86486 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. scenario:profiler - sample timeline=false
scenario:tracing - trace.to_digest
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4193 +/- ##
==========================================
+ Coverage 97.74% 97.75% +0.01%
==========================================
Files 1355 1355
Lines 82333 82362 +29
Branches 4226 4228 +2
==========================================
+ Hits 80477 80515 +38
+ Misses 1856 1847 -9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very much in the style of what we discussed; off to a great start.
There are some questions about the design (parameters, naming, idempotency, etc) that I'd like to answer.
I'd also like to see a lot more tests around this new behavior, that affirms a good answer to all the questions above, e.g.:
- Unit tests, including but not limited to...
- Does the trace pass correct parameters to the propagate event block?
- Does triggering the propagation event with different sampling priority values cause sample to run properly?
- Feature tests, including but not limited to...
- When HTTP is injected/log correlation is run/trace is manually continued across execution contexts with
continue_from
, does is trigger the propagation event properly?
- When HTTP is injected/log correlation is run/trace is manually continued across execution contexts with
@@ -311,7 +311,7 @@ def to_digest | |||
span_id = @active_span && @active_span.id | |||
span_id ||= @parent_span_id unless finished? | |||
# sample the trace_operation with the tracer | |||
@tracer&.sample_trace(self) unless sampling_priority | |||
events.propagate.publish(@active_span, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It dawned on me, I don't think to_digest
is just for propagation, it may also be used for log correlations. My concern is that if we print a log message with correlation behavior on, say at the beginning of a web request, then the sampling decision will actually be made very early (based only off the root span in the worst case.)
Should check this isn't the case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I couldn't find it being used in log correlation, however we do use to_digest for opentelemetry here: https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/opentelemetry/trace.rb#L20-L33 Which seems like it could be an issue if I'm understanding the code correctly because this basically runs to_digest every time we create an opentelemetry context? Let me know if I'm off here.
…, change event name
…DataDog/dd-trace-rb into zachg/fix_circ_import_for_lazy_sampling
Not sure I understand what you mean by this one. The propagation event just triggers to_digest to be run, which will trigger the event in tracer.py to run which will run sampling if there's not already a sampling priority set. |
7986f9a
to
7396654
Compare
962be02
to
eba85d9
Compare
What does this PR do?
Fixes the circular import error caused by trying to add the tracer to the trace_operation object by using events from the trace_operation instead.
Changes order of sampling so that the trace sampler runs before the span sampler.
In order to include a method that creates a
TraceDigest
but does not sample (used for otel shim) we createdto_digest_without_propagate
, and also aliasedto_digest
which does sample withpropagate!
which we will move to pushing users towards going forwards. In the future the plan will be to removeto_digest_without_propagate
and remove sampling fromto_digest
. There fore just havingpropagate!
which samples and returns aTraceDigest
andto_digest
which just returns aTraceDigest
.Motivation:
Change log entry
Yes. Fix circular import in TraceOperation.
Additional Notes:
How to test the change?