From 18e11ae4c06723ee0934300fbb57e51a7e90b63f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 24 Jun 2022 22:06:54 +0200 Subject: [PATCH] Fix HTTP instrumentations to set proper span status (#2427) Add TestSpanStatus tests --- CHANGELOG.md | 6 ++++ .../go-restful/otelrestful/restful.go | 2 +- .../otelrestful/test/restful_test.go | 33 +++++++++++++++++++ .../gin-gonic/gin/otelgin/gintrace.go | 2 +- .../gin/otelgin/test/gintrace_test.go | 29 ++++++++++++++++ .../macaron.v1/otelmacaron/macaron.go | 2 +- .../otelmacaron/test/macaron_test.go | 30 +++++++++++++++++ instrumentation/net/http/otelhttp/handler.go | 2 +- .../net/http/otelhttp/test/handler_test.go | 31 +++++++++++++++++ 9 files changed, 133 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb685f56506..26970a181c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `go.opentelemetry.io/contrib/propagators/autoprop` package to provide configuration of propagators with useful defaults and envar support. (#2258) +### Fixed + +- Fix the `otelhttp`, `otelgin`, `otelmacaron`, `otelrestful` middlewares + by using `SpanKindServer` when deciding the `SpanStatus`. + This makes `4xx` response codes to not be an error anymore. (#2427) + ## [1.7.0/0.32.0] - 2022-04-28 ### Added diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go index 9a77e1e2c03..1cdbdce0f5e 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go +++ b/instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go @@ -65,7 +65,7 @@ func OTelFilter(service string, opts ...Option) restful.FilterFunction { chain.ProcessFilter(req, resp) attrs := semconv.HTTPAttributesFromHTTPStatusCode(resp.StatusCode()) - spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCode(resp.StatusCode()) + spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(resp.StatusCode(), oteltrace.SpanKindServer) span.SetAttributes(attrs...) span.SetStatus(spanStatus, spanMessage) } diff --git a/instrumentation/github.com/emicklei/go-restful/otelrestful/test/restful_test.go b/instrumentation/github.com/emicklei/go-restful/otelrestful/test/restful_test.go index fa838666d50..dbc01ba6ba5 100644 --- a/instrumentation/github.com/emicklei/go-restful/otelrestful/test/restful_test.go +++ b/instrumentation/github.com/emicklei/go-restful/otelrestful/test/restful_test.go @@ -17,6 +17,7 @@ package test import ( "net/http" "net/http/httptest" + "strconv" "testing" "github.com/emicklei/go-restful/v3" @@ -26,6 +27,7 @@ import ( "go.opentelemetry.io/contrib/instrumentation/github.com/emicklei/go-restful/otelrestful" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" oteltrace "go.opentelemetry.io/otel/trace" @@ -171,6 +173,37 @@ func TestMultiFilters(t *testing.T) { assertSpan(t, spans[2], "/library/{name}") } +func TestSpanStatus(t *testing.T) { + testCases := []struct { + httpStatusCode int + wantSpanStatus codes.Code + }{ + {200, codes.Unset}, + {400, codes.Unset}, + {500, codes.Error}, + } + for _, tc := range testCases { + t.Run(strconv.Itoa(tc.httpStatusCode), func(t *testing.T) { + sr := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider() + provider.RegisterSpanProcessor(sr) + handlerFunc := func(req *restful.Request, resp *restful.Response) { + resp.WriteHeader(tc.httpStatusCode) + } + ws := &restful.WebService{} + ws.Route(ws.GET("/").To(handlerFunc)) + container := restful.NewContainer() + container.Filter(otelrestful.OTelFilter("my-service", otelrestful.WithTracerProvider(provider))) + container.Add(ws) + + container.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil)) + + require.Len(t, sr.Ended(), 1, "should emit a span") + assert.Equal(t, sr.Ended()[0].Status().Code, tc.wantSpanStatus, "should only set Error status for HTTP statuses >= 500") + }) + } +} + func assertSpan(t *testing.T, span sdktrace.ReadOnlySpan, name string, attrs ...attribute.KeyValue) { assert.Equal(t, name, span.Name()) assert.Equal(t, oteltrace.SpanKindServer, span.SpanKind()) diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go index 1cfc613cb5f..b1d1258c852 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go @@ -81,7 +81,7 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc { status := c.Writer.Status() attrs := semconv.HTTPAttributesFromHTTPStatusCode(status) - spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCode(status) + spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(status, oteltrace.SpanKindServer) span.SetAttributes(attrs...) span.SetStatus(spanStatus, spanMessage) if len(c.Errors) > 0 { diff --git a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go index 017ddc97001..d67702353f6 100644 --- a/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go +++ b/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go @@ -21,6 +21,7 @@ import ( "html/template" "net/http" "net/http/httptest" + "strconv" "testing" "github.com/gin-gonic/gin" @@ -136,6 +137,34 @@ func TestError(t *testing.T) { assert.Equal(t, codes.Error, span.Status().Code) } +func TestSpanStatus(t *testing.T) { + testCases := []struct { + httpStatusCode int + wantSpanStatus codes.Code + }{ + {200, codes.Unset}, + {400, codes.Unset}, + {500, codes.Error}, + } + for _, tc := range testCases { + t.Run(strconv.Itoa(tc.httpStatusCode), func(t *testing.T) { + sr := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider() + provider.RegisterSpanProcessor(sr) + router := gin.New() + router.Use(otelgin.Middleware("foobar", otelgin.WithTracerProvider(provider))) + router.GET("/", func(c *gin.Context) { + c.Status(tc.httpStatusCode) + }) + + router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil)) + + require.Len(t, sr.Ended(), 1, "should emit a span") + assert.Equal(t, sr.Ended()[0].Status().Code, tc.wantSpanStatus, "should only set Error status for HTTP statuses >= 500") + }) + } +} + func TestHTML(t *testing.T) { sr := tracetest.NewSpanRecorder() provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr)) diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go index f2b3c008758..2629472ad23 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/macaron.go @@ -64,7 +64,7 @@ func Middleware(service string, opts ...Option) macaron.Handler { status := c.Resp.Status() attrs := semconv.HTTPAttributesFromHTTPStatusCode(status) - spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCode(status) + spanStatus, spanMessage := semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(status, oteltrace.SpanKindServer) span.SetAttributes(attrs...) span.SetStatus(spanStatus, spanMessage) } diff --git a/instrumentation/gopkg.in/macaron.v1/otelmacaron/test/macaron_test.go b/instrumentation/gopkg.in/macaron.v1/otelmacaron/test/macaron_test.go index c86b3f22a1b..21a92949fee 100644 --- a/instrumentation/gopkg.in/macaron.v1/otelmacaron/test/macaron_test.go +++ b/instrumentation/gopkg.in/macaron.v1/otelmacaron/test/macaron_test.go @@ -17,6 +17,7 @@ package test import ( "net/http" "net/http/httptest" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -26,6 +27,7 @@ import ( "go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" oteltrace "go.opentelemetry.io/otel/trace" @@ -94,3 +96,31 @@ func TestChildSpanNames(t *testing.T) { assert.Contains(t, attrs, attribute.String("http.method", "GET")) assert.Contains(t, attrs, attribute.String("http.target", "/book/foo")) } + +func TestSpanStatus(t *testing.T) { + testCases := []struct { + httpStatusCode int + wantSpanStatus codes.Code + }{ + {200, codes.Unset}, + {400, codes.Unset}, + {500, codes.Error}, + } + for _, tc := range testCases { + t.Run(strconv.Itoa(tc.httpStatusCode), func(t *testing.T) { + sr := tracetest.NewSpanRecorder() + provider := trace.NewTracerProvider() + provider.RegisterSpanProcessor(sr) + m := macaron.Classic() + m.Use(otelmacaron.Middleware("foobar", otelmacaron.WithTracerProvider(provider))) + m.Get("/", func(ctx *macaron.Context) { + ctx.Resp.WriteHeader(tc.httpStatusCode) + }) + + m.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil)) + + require.Len(t, sr.Ended(), 1, "should emit a span") + assert.Equal(t, sr.Ended()[0].Status().Code, tc.wantSpanStatus, "should only set Error status for HTTP statuses >= 500") + }) + } +} diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index a2a06f8915c..be854df41e6 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -227,7 +227,7 @@ func setAfterServeAttributes(span trace.Span, read, wrote int64, statusCode int, } if statusCode > 0 { attributes = append(attributes, semconv.HTTPAttributesFromHTTPStatusCode(statusCode)...) - span.SetStatus(semconv.SpanStatusFromHTTPStatusCode(statusCode)) + span.SetStatus(semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(statusCode, trace.SpanKindServer)) } if werr != nil && werr != io.EOF { attributes = append(attributes, WriteErrorKey.String(werr.Error())) diff --git a/instrumentation/net/http/otelhttp/test/handler_test.go b/instrumentation/net/http/otelhttp/test/handler_test.go index 7bec62a5ebb..3b8e4ba6588 100644 --- a/instrumentation/net/http/otelhttp/test/handler_test.go +++ b/instrumentation/net/http/otelhttp/test/handler_test.go @@ -21,6 +21,7 @@ import ( "io/ioutil" "net/http" "net/http/httptest" + "strconv" "strings" "testing" @@ -29,6 +30,7 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/sdk/metric/metrictest" sdktrace "go.opentelemetry.io/otel/sdk/trace" @@ -187,3 +189,32 @@ func TestWithPublicEndpoint(t *testing.T) { require.Len(t, done[0].Links(), 1, "should contain link") require.True(t, sc.Equal(done[0].Links()[0].SpanContext), "should link incoming span context") } + +func TestSpanStatus(t *testing.T) { + testCases := []struct { + httpStatusCode int + wantSpanStatus codes.Code + }{ + {200, codes.Unset}, + {400, codes.Unset}, + {500, codes.Error}, + } + for _, tc := range testCases { + t.Run(strconv.Itoa(tc.httpStatusCode), func(t *testing.T) { + sr := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider() + provider.RegisterSpanProcessor(sr) + h := otelhttp.NewHandler( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.httpStatusCode) + }), "test_handler", + otelhttp.WithTracerProvider(provider), + ) + + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil)) + + require.Len(t, sr.Ended(), 1, "should emit a span") + assert.Equal(t, sr.Ended()[0].Status().Code, tc.wantSpanStatus, "should only set Error status for HTTP statuses >= 500") + }) + } +}