otelhttptrace: handle missing getconn hook without panic (#1)

* otelhttptrace: handle missing getconn hook without panic

We have many reports that end() gets called without the
span being defined in start() and causes a panic.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

* add tests for clientTracer.end

* CHANGELOG.md: update PR number for changelog entry

* otelhttptrace/go.mod: add go.opentelemetry.io/otel/sdk/trace

---------

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Co-authored-by: Tonis Tiigi <tonistiigi@gmail.com>

CHANGELOG.md

@@ -24,6 +24,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - The deprecated `go.opentelemetry.io/contrib/processors/baggagecopy` package is removed. (#5853)
 
+### Fixed
+
+- Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965)
+
 <!-- Released section -->
 <!-- Don't change this section unless doing release -->
 

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

@@ -215,6 +215,10 @@ func (ct *clientTracer) start(hook, spanName string, attrs ...attribute.KeyValue
 
 func (ct *clientTracer) end(hook string, err error, attrs ...attribute.KeyValue) {
 	if !ct.useSpans {
+		// sometimes end may be called without previous start
+		if ct.root == nil {
+			ct.root = trace.SpanFromContext(ct.Context)
+		}
 		if err != nil {
 			attrs = append(attrs, attribute.String(hook+".error", err.Error()))
 		}

instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go

@@ -5,9 +5,20 @@ package otelhttptrace
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptrace"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/attribute"
+	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/trace/tracetest"
+	"go.opentelemetry.io/otel/trace"
 
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 )
@@ -32,3 +43,119 @@ func ExampleNewClientTrace() {
 
 	fmt.Println(resp.Status)
 }
+
+func Test_clientTracer_end(t *testing.T) {
+	t.Run("end called with no parent clientTracer span", func(t *testing.T) {
+		fixture := prepareClientTraceTest()
+		fixture.ct.end("http.getconn", nil, HTTPConnectionReused.Bool(true), HTTPConnectionWasIdle.Bool(true))
+		assert.Len(t, fixture.spanRecorder.Ended(), 0)
+	})
+
+	t.Run("end called with no sub spans, no root span, and no errors", func(t *testing.T) {
+		fixture := prepareClientTraceTest()
+		WithoutSubSpans().apply(fixture.ct)
+
+		ctx, span := fixture.tracer.Start(fixture.ct.Context, "client request")
+		fixture.ct.Context = ctx
+
+		fixture.ct.end("http.getconn", nil, HTTPConnectionReused.Bool(true), HTTPConnectionWasIdle.Bool(true))
+		span.End()
+
+		require.Len(t, fixture.spanRecorder.Ended(), 1)
+		recSpan := fixture.spanRecorder.Ended()[0]
+
+		require.Len(t, recSpan.Events(), 1)
+		gotEvent := recSpan.Events()[0]
+		assert.Equal(t, "http.getconn.done", gotEvent.Name)
+
+		assert.Equal(t,
+			[]attribute.KeyValue{HTTPConnectionReused.Bool(true), HTTPConnectionWasIdle.Bool(true)},
+			gotEvent.Attributes,
+		)
+	})
+
+	t.Run("end called with no sub spans, root span set, and no errors", func(t *testing.T) {
+		fixture := prepareClientTraceTest()
+		WithoutSubSpans().apply(fixture.ct)
+
+		ctx, span := fixture.tracer.Start(fixture.ct.Context, "client request")
+		fixture.ct.Context = ctx
+		fixture.ct.root = span
+
+		fixture.ct.end("http.getconn", nil, HTTPConnectionReused.Bool(true), HTTPConnectionWasIdle.Bool(true))
+		span.End()
+
+		require.Len(t, fixture.spanRecorder.Ended(), 1)
+		recSpan := fixture.spanRecorder.Ended()[0]
+
+		require.Len(t, recSpan.Events(), 1)
+		gotEvent := recSpan.Events()[0]
+		assert.Equal(t, "http.getconn.done", gotEvent.Name)
+
+		assert.Equal(t,
+			[]attribute.KeyValue{
+				HTTPConnectionReused.Bool(true),
+				HTTPConnectionWasIdle.Bool(true),
+			},
+			gotEvent.Attributes,
+		)
+	})
+
+	t.Run("end called with no sub spans, root span set, and error", func(t *testing.T) {
+		fixture := prepareClientTraceTest()
+		WithoutSubSpans().apply(fixture.ct)
+
+		ctx, span := fixture.tracer.Start(fixture.ct.Context, "client request")
+		fixture.ct.Context = ctx
+		fixture.ct.root = span
+
+		fixture.ct.end("http.getconn", errors.New("testError"), HTTPConnectionReused.Bool(true), HTTPConnectionWasIdle.Bool(true))
+		span.End()
+
+		require.Len(t, fixture.spanRecorder.Ended(), 1)
+		recSpan := fixture.spanRecorder.Ended()[0]
+
+		require.Len(t, recSpan.Events(), 1)
+		gotEvent := recSpan.Events()[0]
+		assert.Equal(t, "http.getconn.done", gotEvent.Name)
+
+		assert.Equal(t,
+			[]attribute.KeyValue{
+				HTTPConnectionReused.Bool(true),
+				HTTPConnectionWasIdle.Bool(true),
+				attribute.Key("http.getconn.error").String("testError"),
+			},
+			gotEvent.Attributes,
+		)
+	})
+}
+
+type clientTraceTestFixture struct {
+	spanRecorder *tracetest.SpanRecorder
+	tracer       trace.Tracer
+	ct           *clientTracer
+}
+
+func prepareClientTraceTest() clientTraceTestFixture {
+	fixture := clientTraceTestFixture{}
+	fixture.spanRecorder = tracetest.NewSpanRecorder()
+	provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(fixture.spanRecorder))
+	otel.SetTracerProvider(provider)
+
+	fixture.tracer = provider.Tracer(
+		ScopeName,
+		trace.WithInstrumentationVersion(Version()))
+
+	fixture.ct = &clientTracer{
+		Context:         context.Background(),
+		tracerProvider:  otel.GetTracerProvider(),
+		root:            nil,
+		tr:              fixture.tracer,
+		activeHooks:     make(map[string]context.Context),
+		redactedHeaders: map[string]struct{}{},
+		addHeaders:      true,
+		useSpans:        true,
+	}
+
+	return fixture
+}

instrumentation/net/http/httptrace/otelhttptrace/go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/stretchr/testify v1.9.0
 	go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0
 	go.opentelemetry.io/otel v1.28.0
+	go.opentelemetry.io/otel/sdk v1.28.0
 	go.opentelemetry.io/otel/trace v1.28.0
 )
 
@@ -15,8 +16,10 @@ require (
 	github.com/felixge/httpsnoop v1.0.4 // indirect
 	github.com/go-logr/logr v1.4.2 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
+	github.com/google/uuid v1.6.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	go.opentelemetry.io/otel/metric v1.28.0 // indirect
+	golang.org/x/sys v0.21.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
 

instrumentation/net/http/httptrace/otelhttptrace/go.sum

@@ -9,6 +9,8 @@ github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
 github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
 github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
+github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
@@ -17,8 +19,12 @@ go.opentelemetry.io/otel v1.28.0 h1:/SqNcYk+idO0CxKEUOtKQClMK/MimZihKYMruSMViUo=
 go.opentelemetry.io/otel v1.28.0/go.mod h1:q68ijF8Fc8CnMHKyzqL6akLO46ePnjkgfIMIjUIX9z4=
 go.opentelemetry.io/otel/metric v1.28.0 h1:f0HGvSl1KRAU1DLgLGFjrwVyismPlnuU6JD6bOeuA5Q=
 go.opentelemetry.io/otel/metric v1.28.0/go.mod h1:Fb1eVBFZmLVTMb6PPohq3TO9IIhUisDsbJoL/+uQW4s=
+go.opentelemetry.io/otel/sdk v1.28.0 h1:b9d7hIry8yZsgtbmM0DKyPWMMUMlK9NEKuIG4aBqWyE=
+go.opentelemetry.io/otel/sdk v1.28.0/go.mod h1:oYj7ClPUA7Iw3m+r7GeEjz0qckQRJK2B8zjcZEfu7Pg=
 go.opentelemetry.io/otel/trace v1.28.0 h1:GhQ9cUuQGmNDd5BTCP2dAvv75RdMxEfTmYejp+lkx9g=
 go.opentelemetry.io/otel/trace v1.28.0/go.mod h1:jPyXzNPg6da9+38HEwElrQiHlVMTnVfM3/yv2OlIHaI=
+golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
+golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=