tracing: finish OTEL bridge span only once (#10095)

* tracing: finish OTEL bridge span only once

#### Problem

The OTEL spec allows to close the span multiple times and that `End` should be idempotent. I think the jaeger implementation doesn't have the same guarantees, so we have to handle this in out bridge implementation.

Currently, we allow to call `Finish` multiple times and we end up reporting the span multiple times: once for exhausting the HTTP request body with `io.EOF` and once for `Close()`. For more details see https://github.com/open-telemetry/opentelemetry-go-contrib /pull/6391

#### The OTEL spec

https://github.com/open-telemetry/opentelemetry-specification/blob/50027a1036746dce293ee0a8592639f131fc1fb8/specification/trace/api.md#end

> Implementations SHOULD ignore all subsequent calls to End and any other Span methods, i.e. the Span becomes non-recording by being ended (there might be exceptions when Tracer is streaming events and has no mutable state associated with the Span).

Signed-off-by: Dimitar Dimitrov <[email protected]>

* Apply suggestions from code review

Co-authored-by: Arve Knudsen <[email protected]>

---------

Signed-off-by: Dimitar Dimitrov <[email protected]>
Co-authored-by: Arve Knudsen <[email protected]>

pkg/util/tracing/tracing.go

@@ -14,6 +14,7 @@ import (
 	"go.opentelemetry.io/otel/codes"
 	"go.opentelemetry.io/otel/trace"
 	"go.opentelemetry.io/otel/trace/embedded"
+	"go.uber.org/atomic"
 )
 
 type OpenTelemetryProviderBridge struct {
@@ -143,6 +144,9 @@ type OpenTelemetrySpanBridge struct {
 
 	span     opentracing.Span
 	provider trace.TracerProvider
+
+	// ended allows us to provide idempotency for End. See the OTEL spec for End: https://github.com/open-telemetry/opentelemetry-specification/blob/50027a1036746dce293ee0a8592639f131fc1fb8/specification/trace/api.md#end
+	ended atomic.Bool
 }
 
 func NewOpenTelemetrySpanBridge(span opentracing.Span, provider trace.TracerProvider) *OpenTelemetrySpanBridge {
@@ -157,6 +161,9 @@ func NewOpenTelemetrySpanBridge(span opentracing.Span, provider trace.TracerProv
 // is called. Therefore, updates to the Span are not allowed after this
 // method has been called.
 func (s *OpenTelemetrySpanBridge) End(options ...trace.SpanEndOption) {
+	if !s.ended.CompareAndSwap(false, true) {
+		return
+	}
 	if len(options) == 0 {
 		s.span.Finish()
 		return

pkg/util/tracing/tracing_test.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"math/rand"
 	"testing"
+	"time"
 
 	"github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/log"
@@ -235,3 +236,62 @@ func TestOpenTelemetryTracerBridge_Start(t *testing.T) {
 
 	require.Same(t, span, ctxSpan, "returned context should contain span")
 }
+
+func TestOpenTelemetrySpanBridge_EndIdempotency(t *testing.T) {
+	tests := map[string]struct {
+		endCalls    int
+		withOptions bool
+		finishCalls int
+		finishOpts  int
+	}{
+		"single end without options": {
+			endCalls:    1,
+			withOptions: false,
+			finishCalls: 1,
+			finishOpts:  0,
+		},
+		"multiple ends without options": {
+			endCalls:    3,
+			withOptions: false,
+			finishCalls: 1,
+			finishOpts:  0,
+		},
+		"single end with options": {
+			endCalls:    1,
+			withOptions: true,
+			finishCalls: 0,
+			finishOpts:  1,
+		},
+		"multiple ends with options": {
+			endCalls:    3,
+			withOptions: true,
+			finishCalls: 0,
+			finishOpts:  1,
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			// Setup mock
+			m := &TracingSpanMock{}
+			m.On("Finish").Return()
+			m.On("FinishWithOptions", mock.Anything).Return()
+
+			// Create bridge
+			s := NewOpenTelemetrySpanBridge(m, nil)
+
+			// Call End potentially multiple times
+			for i := 0; i < testData.endCalls; i++ {
+				if testData.withOptions {
+					s.End(trace.WithTimestamp(time.Now()))
+				} else {
+					s.End()
+				}
+			}
+
+			// Assert expectations
+			m.AssertNumberOfCalls(t, "Finish", testData.finishCalls)
+			m.AssertNumberOfCalls(t, "FinishWithOptions", testData.finishOpts)
+		})
+	}
+}