Skip to content

Commit

Permalink
WIP: ddtrace/tracer: only finish execution trace task, restore pprof …
Browse files Browse the repository at this point in the history
…labels 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.

TODO - regression test? esp. for pprof label behavior
TODO - this is doing the trace task emission under a lock where it
	wasn't before. Bottleneck issue?
  • Loading branch information
nsrip-dd committed May 23, 2024
1 parent b0aa1b8 commit 297fed4
Showing 1 changed file with 10 additions and 9 deletions.
19 changes: 10 additions & 9 deletions ddtrace/tracer/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,9 +478,7 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
s.Unlock()
}
}
if s.taskEnd != nil {
s.taskEnd()
}

if s.goExecTraced && rt.IsEnabled() {
// Only tag spans as traced if they both started & ended with
// execution tracing enabled. This is technically not sufficient
Expand All @@ -505,12 +503,6 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
}

s.finish(t)

if s.pprofCtxRestore != nil {
// Restore the labels of the parent span so any CPU samples after this
// point are attributed correctly.
pprof.SetGoroutineLabels(s.pprofCtxRestore)
}
}

// SetOperationName sets or changes the operation name.
Expand Down Expand Up @@ -543,6 +535,9 @@ func (s *span) finish(finishTime int64) {
if s.Duration < 0 {
s.Duration = 0
}
if s.taskEnd != nil {
s.taskEnd()
}

keep := true
if t, ok := internal.GetGlobalTracer().(*tracer); ok {
Expand Down Expand Up @@ -583,6 +578,12 @@ func (s *span) finish(finishTime int64) {
s, s.Name, s.Resource, s.Meta, s.Metrics)
}
s.context.finish()

if s.pprofCtxRestore != nil {
// Restore the labels of the parent span so any CPU samples after this
// point are attributed correctly.
pprof.SetGoroutineLabels(s.pprofCtxRestore)
}
}

// newAggregableSpan creates a new summary for the span s, within an application
Expand Down

0 comments on commit 297fed4

Please sign in to comment.