Skip to content

Commit

Permalink
Ensure http response wrapper always sets a statusCode (#2822)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
dmathieu committed Nov 8, 2022
1 parent d9bccb7 commit 0252c36
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -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
Expand Down
15 changes: 10 additions & 5 deletions instrumentation/net/http/otelhttp/handler.go
Expand Up @@ -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,
Expand Down Expand Up @@ -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()))
}
Expand Down
55 changes: 55 additions & 0 deletions instrumentation/net/http/otelhttp/test/handler_test.go
Expand Up @@ -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()

Expand Down

0 comments on commit 0252c36

Please sign in to comment.