-
Notifications
You must be signed in to change notification settings - Fork 397
Tracing.continue_from! with a block maintains active trace until the block ends #4941
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2025-10-04 02:07:44 UTC |
91fa0bb to
eb4376a
Compare
BenchmarksBenchmark execution time: 2025-12-08 17:55:39 Comparing candidate commit 0524bd4 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:profiling - stack collector (ruby frames - native filenames enabled)
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 0524bd4 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
maycmlee
left a comment
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.
Nothing for docs to review
p-datadog
left a comment
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.
I think this branch is missing #4925, if so it should have master merged into it and formatters rerun.
Strech
left a comment
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.
LGTM 👍🏼
P.S I've left few non-blocking suggestions
| if block | ||
| # When a block is given, the trace will be active until the block finishes. | ||
| context.activate!(trace) do | ||
| yield | ||
| ensure # We have to flush even when an error occurs | ||
| # On block completion, force the trace to finish and flush its finished spans. | ||
| # Unfinished spans are lost as the {TraceOperation} has ended. | ||
| trace.finish! | ||
| flush_trace(trace) | ||
| end | ||
| else | ||
| # Otherwise, the trace will be bound to the current thread after this point | ||
| context.activate!(trace) | ||
| end |
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 could be streamlined with a guard-clause
| if block | |
| # When a block is given, the trace will be active until the block finishes. | |
| context.activate!(trace) do | |
| yield | |
| ensure # We have to flush even when an error occurs | |
| # On block completion, force the trace to finish and flush its finished spans. | |
| # Unfinished spans are lost as the {TraceOperation} has ended. | |
| trace.finish! | |
| flush_trace(trace) | |
| end | |
| else | |
| # Otherwise, the trace will be bound to the current thread after this point | |
| context.activate!(trace) | |
| end | |
| # The trace will be bound to the current thread if no block is given | |
| return context.activate!(trace) unless block | |
| # When a block is given, the trace will be active until the block finishes. | |
| context.activate!(trace) do | |
| yield | |
| ensure # We have to flush even when an error occurs | |
| # On block completion, force the trace to finish and flush its finished spans. | |
| # Unfinished spans are lost as the {TraceOperation} has ended. | |
| trace.finish! | |
| flush_trace(trace) | |
| end |
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.
I actually prefer the current version in the PR because the proposed revision adds several negations.
| # Don't finish the span, so finished_span_count remains 0 | ||
| end | ||
|
|
||
| # No spans should be flushed |
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.
I think comment is unnecessary
| # No spans should be flushed |
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.
I guess it's there for emphasis?
|
Hey, should we go ahead and merge this one? |
* master: (523 commits) DI: relax upper bound for reported long method duration (#5113) Change development guide to use Ruby 3.4 by default (#5112) Fix exception in RC on OpenFeature::Engine absence [🤖] Update Latest Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/19996135509 [🤖] Update System Tests: https://github.com/DataDog/dd-trace-rb/actions/runs/19996155319 Add ruby guikld to all dsm files add DSM to codeowners feat: add process tags to traces (#5033) DEBUG-3558 DI: chunk snapshot payloads (#5086) CI: enable push_to_test_optimization unless this is a pull request from a fork fix steep checks [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/19827679824 [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/19827296218 Add test to verify consumer backlog serialization with kafka_commit tag Remove one diagnostic output Install rubygems Add diagnostic steps Use default rubygems+bundler for each Ruby version Revert "Disable profiling specs for 4.0" Use git `datadog-ruby_core_source` having 4.0 ...
|
I will be clicking the merge button on this after the release is complete. |
What does this PR do?
Fixes an unintended behavior of
Datadog::Tracing.continue_from!(digest, &block)where if multiple local root spans are created inside a theblock, only the first one is associated with thedigest. Any subsequence spans inside that block are associated with a new, unrelated trace.Effectively, this PR makes this block do what it looks like it does:
Before this PR, only the
first.spanwould be associated with thedigest.Motivation:
While trying to address #3465, I wasn't able to get all background spans to be associated with the active span in the main calling thread. This was due to the bug that this PR fixes.
Change log entry
Yes.
Tracing.continue_from!with a block maintains active trace until the block ends.