diff --git a/tracing.go b/tracing.go index 8466bacca..a6cf743a9 100644 --- a/tracing.go +++ b/tracing.go @@ -60,6 +60,8 @@ type Span struct { //nolint: maligned // prefer readability over optimal memory contexts map[string]Context // profiler instance if attached, nil otherwise. profiler transactionProfiler + // a Once instance to make sure that Finish() is only called once. + finishOnce sync.Once } // TraceParentContext describes the context of a (remote) parent span. @@ -196,35 +198,11 @@ func StartSpan(ctx context.Context, operation string, options ...SpanOption) *Sp // Finish sets the span's end time, unless already set. If the span is the root // of a span tree, Finish sends the span tree to Sentry as a transaction. +// +// The logic is executed at most once per span, so that (incorrectly) calling it twice +// never double sends to Sentry. func (s *Span) Finish() { - // TODO(tracing): maybe make Finish run at most once, such that - // (incorrectly) calling it twice never double sends to Sentry. - - // For the timing to be correct, the profiler must be stopped before s.EndTime. - var profile *profileInfo - if s.profiler != nil { - profile = s.profiler.Finish(s) - } - - if s.EndTime.IsZero() { - s.EndTime = monotonicTimeSince(s.StartTime) - } - - if !s.Sampled.Bool() { - return - } - event := s.toEvent() - if event == nil { - return - } - - event.sdkMetaData.transactionProfile = profile - - // TODO(tracing): add breadcrumbs - // (see https://github.com/getsentry/sentry-python/blob/f6f3525f8812f609/sentry_sdk/tracing.py#L372) - - hub := hubFromContext(s.ctx) - hub.CaptureEvent(event) + s.finishOnce.Do(s.doFinish) } // Context returns the context containing the span. @@ -353,6 +331,35 @@ func (s *Span) SetDynamicSamplingContext(dsc DynamicSamplingContext) { } } +// doFinish runs the actual Span.Finish() logic. +func (s *Span) doFinish() { + // For the timing to be correct, the profiler must be stopped before s.EndTime. + var profile *profileInfo + if s.profiler != nil { + profile = s.profiler.Finish(s) + } + + if s.EndTime.IsZero() { + s.EndTime = monotonicTimeSince(s.StartTime) + } + + if !s.Sampled.Bool() { + return + } + event := s.toEvent() + if event == nil { + return + } + + event.sdkMetaData.transactionProfile = profile + + // TODO(tracing): add breadcrumbs + // (see https://github.com/getsentry/sentry-python/blob/f6f3525f8812f609/sentry_sdk/tracing.py#L372) + + hub := hubFromContext(s.ctx) + hub.CaptureEvent(event) +} + // sentryTracePattern matches either // // TRACE_ID - SPAN_ID diff --git a/tracing_test.go b/tracing_test.go index dc7fd9d75..0e5102e30 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -980,3 +980,28 @@ func TestAdjustingTransactionSourceBeforeSending(t *testing.T) { }) } } + +// This is a regression test for https://github.com/getsentry/sentry-go/issues/587 +// Without the "spans can be finished only once" fix, this test will fail +// when run with race detection ("-race"). +func TestSpanFinishConcurrentlyWithoutRaces(t *testing.T) { + ctx := NewTestContext(ClientOptions{ + EnableTracing: true, + TracesSampleRate: 1, + }) + transaction := StartTransaction(ctx, "op") + + go func() { + for { + transaction.Finish() + } + }() + + go func() { + for { + transaction.Finish() + } + }() + + time.Sleep(50 * time.Millisecond) +}