From 0252c36b1f09a8016fc96f50220db01b11c92bcb Mon Sep 17 00:00:00 2001
From: Damien Mathieu <42@dmathieu.com>
Date: Tue, 8 Nov 2022 16:47:28 +0100
Subject: [PATCH] Ensure http response wrapper always sets a statusCode (#2822)

* ensure http response wrapper always sets a statusCode

* add changelog entry

* store a default status code instead of writing one

* move changelog entry back into unreleased
---
 CHANGELOG.md                                  |  4 ++
 instrumentation/net/http/otelhttp/handler.go  | 15 +++--
 .../net/http/otelhttp/test/handler_test.go    | 55 +++++++++++++++++++
 3 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 95d8c15a97c..0caaa9a5a95 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - The `WithLogger` option to `go.opentelemetry.io/contrib/samplers/jaegerremote` to allow users to pass a `logr.Logger` and have operations logged. (#2566)
 - Add the `messaging.url` & `messaging.system` attributes to all appropriate SQS operations in the `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws` package. (#2879)
 
+### Fixed
+
+- Set the status_code span attribute even if the HTTP handler hasn't written anything. (#2822)
+
 ## [1.11.1/0.36.4/0.5.2]
 
 ### Added
diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go
index 4c037f1d8e0..506e7c06755 100644
--- a/instrumentation/net/http/otelhttp/handler.go
+++ b/instrumentation/net/http/otelhttp/handler.go
@@ -180,7 +180,13 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	rww := &respWriterWrapper{ResponseWriter: w, record: writeRecordFunc, ctx: ctx, props: h.propagators}
+	rww := &respWriterWrapper{
+		ResponseWriter: w,
+		record:         writeRecordFunc,
+		ctx:            ctx,
+		props:          h.propagators,
+		statusCode:     200, // default status code in case the Handler doesn't write anything
+	}
 
 	// Wrap w to use our ResponseWriter methods while also exposing
 	// other interfaces that w may implement (http.CloseNotifier,
@@ -230,10 +236,9 @@ func setAfterServeAttributes(span trace.Span, read, wrote int64, statusCode int,
 	if wrote > 0 {
 		attributes = append(attributes, WroteBytesKey.Int64(wrote))
 	}
-	if statusCode > 0 {
-		attributes = append(attributes, semconv.HTTPAttributesFromHTTPStatusCode(statusCode)...)
-		span.SetStatus(semconv.SpanStatusFromHTTPStatusCodeAndSpanKind(statusCode, trace.SpanKindServer))
-	}
+	attributes = append(attributes, semconv.HTTPAttributesFromHTTPStatusCode(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 645089fb7dc..99160efcd58 100644
--- a/instrumentation/net/http/otelhttp/test/handler_test.go
+++ b/instrumentation/net/http/otelhttp/test/handler_test.go
@@ -88,6 +88,61 @@ func TestHandlerBasics(t *testing.T) {
 	}
 }
 
+func TestHandlerEmittedAttributes(t *testing.T) {
+	testCases := []struct {
+		name       string
+		handler    func(http.ResponseWriter, *http.Request)
+		attributes []attribute.KeyValue
+	}{
+		{
+			name: "With a success handler",
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(http.StatusOK)
+			},
+			attributes: []attribute.KeyValue{
+				attribute.Int("http.status_code", 200),
+			},
+		},
+		{
+			name: "With a failing handler",
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(http.StatusBadRequest)
+			},
+			attributes: []attribute.KeyValue{
+				attribute.Int("http.status_code", 400),
+			},
+		},
+		{
+			name: "With an empty handler",
+			handler: func(w http.ResponseWriter, r *http.Request) {
+			},
+			attributes: []attribute.KeyValue{
+				attribute.Int("http.status_code", 200),
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			sr := tracetest.NewSpanRecorder()
+			provider := sdktrace.NewTracerProvider()
+			provider.RegisterSpanProcessor(sr)
+			h := otelhttp.NewHandler(
+				http.HandlerFunc(tc.handler), "test_handler",
+				otelhttp.WithTracerProvider(provider),
+			)
+
+			h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/", nil))
+
+			require.Len(t, sr.Ended(), 1, "should emit a span")
+			attrs := sr.Ended()[0].Attributes()
+
+			for _, a := range tc.attributes {
+				assert.Contains(t, attrs, a)
+			}
+		})
+	}
+}
+
 func TestHandlerRequestWithTraceContext(t *testing.T) {
 	rr := httptest.NewRecorder()