-
Notifications
You must be signed in to change notification settings - Fork 435
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
ddtrace/tracer: only finish execution trace task, restore pprof labels once #2708
Conversation
BenchmarksBenchmark execution time: 2024-05-31 13:52:19 Comparing candidate commit f70a3cd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 1 unstable metrics. |
…s once It's possible for users to call Finish multiple times on a span. We should only record the span finishing in the execution tracer and via pprof labels one time, though. Otherwise we're 1) wasting space in the trace and 2) possibly overriding pprof labels with incorrect values. Move the task ending and label setting inside span.finish, after we check whether the span is already finished.
297fed4
to
3c50bd6
Compare
For tracer reviewers, this is doing some cleanup under the span lock that wasn't under the lock before. The cleanup should be cheap: just taking a stack trace with frame pointers, writing some bytes to a buffer, and updating a pointer in a goroutine structure. Our benchmarks don't show any regression, but I guess we also aren't specifically measuring contended access to a span while a program is calling |
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 🙇
Edit: On second thought, I'm less sure. See my comment on the other PR.
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 🙇
What does this PR do?
Only finish the execution trace task and restore pprof labels for a span
the first time Finish is called.
Motivation
It's possible for users to call Finish multiple times on a span. We
should only record the span finishing in the execution tracer and via
pprof labels one time, though. Otherwise we're 1) wasting space in the
trace and 2) possibly overriding pprof labels with incorrect values.
Move the task ending and label setting inside span.finish, after we
check whether the span is already finished.
#2709 shows an example regression test for this issue. We could
do just the pprof label check in this package, but if we want to check
execution traces as well I think we should move to golang.org/x/exp/trace,
which IMO warrants moving to a separate module to isolate the golang.org/x/exp
dependency. Open to doing a smaller thing in this PR, or we can
go with #2709.
Here's an example program exercising the pprof label behavior:
On current main, we see this:
But all the spans are finished, so there shouldn't be any labels associated with
waste
. With this PR, we see there are no tags associated with the timeReviewer's Checklist
Unsure? Have a question? Request a review!