From 297fed451d67a463a574c5c1a3ca20f73ba31bbb Mon Sep 17 00:00:00 2001
From: Nick Ripley <nick.ripley@datadoghq.com>
Date: Thu, 23 May 2024 10:26:23 -0400
Subject: [PATCH] WIP: ddtrace/tracer: only finish execution trace task,
 restore pprof 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?
---
 ddtrace/tracer/span.go | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/ddtrace/tracer/span.go b/ddtrace/tracer/span.go
index 7d5ebf3a52..ee201c4f9c 100644
--- a/ddtrace/tracer/span.go
+++ b/ddtrace/tracer/span.go
@@ -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
@@ -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.
@@ -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 {
@@ -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