Skip to content

Commit

Permalink
ddtrace/tracer: only finish execution trace task, restore pprof label…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
nsrip-dd committed May 24, 2024
1 parent b0aa1b8 commit 3c50bd6
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 3c50bd6

Please sign in to comment.