diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c5d1a37850..939f5b18e1a 100644 --- a/CHANGELOG.md +++ b/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) + diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go index 67e03f24810..6ef23721cb7 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go +++ b/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())) } diff --git a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go b/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go index 966473dd0c3..b6eebfe703e 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/clienttrace_test.go +++ b/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 +} diff --git a/instrumentation/net/http/httptrace/otelhttptrace/go.mod b/instrumentation/net/http/httptrace/otelhttptrace/go.mod index 340c83d92f5..7ddb1e2a9ad 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/go.mod +++ b/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 ) diff --git a/instrumentation/net/http/httptrace/otelhttptrace/go.sum b/instrumentation/net/http/httptrace/otelhttptrace/go.sum index ee029086df6..20bb413ce59 100644 --- a/instrumentation/net/http/httptrace/otelhttptrace/go.sum +++ b/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=