From 5286e934c08043e8bac167910425fb68ba797fd0 Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Wed, 22 May 2024 09:32:30 +0200 Subject: [PATCH 1/3] introduce respWriter.Flush so we don't write the status twice --- instrumentation/net/http/otelhttp/handler.go | 3 ++ .../net/http/otelhttp/handler_test.go | 3 ++ instrumentation/net/http/otelhttp/wrap.go | 10 +++++ .../net/http/otelhttp/wrap_test.go | 37 +++++++++++++++++++ 4 files changed, 53 insertions(+) create mode 100644 instrumentation/net/http/otelhttp/wrap_test.go diff --git a/instrumentation/net/http/otelhttp/handler.go b/instrumentation/net/http/otelhttp/handler.go index c64f8beca71..810b7dd769d 100644 --- a/instrumentation/net/http/otelhttp/handler.go +++ b/instrumentation/net/http/otelhttp/handler.go @@ -204,6 +204,9 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http WriteHeader: func(httpsnoop.WriteHeaderFunc) httpsnoop.WriteHeaderFunc { return rww.WriteHeader }, + Flush: func(httpsnoop.FlushFunc) httpsnoop.FlushFunc { + return rww.Flush + }, }) labeler := &Labeler{} diff --git a/instrumentation/net/http/otelhttp/handler_test.go b/instrumentation/net/http/otelhttp/handler_test.go index 0ba6eeeb985..8b0b7a23a3b 100644 --- a/instrumentation/net/http/otelhttp/handler_test.go +++ b/instrumentation/net/http/otelhttp/handler_test.go @@ -29,6 +29,9 @@ func TestHandler(t *testing.T) { return otelhttp.NewHandler( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Implements(t, (*http.Flusher)(nil), w) + + w.(http.Flusher).Flush() + _, _ = io.WriteString(w, "Hello, world!\n") }), "test_handler", ) }, diff --git a/instrumentation/net/http/otelhttp/wrap.go b/instrumentation/net/http/otelhttp/wrap.go index 2f4cc124dc6..948f8406c09 100644 --- a/instrumentation/net/http/otelhttp/wrap.go +++ b/instrumentation/net/http/otelhttp/wrap.go @@ -87,3 +87,13 @@ func (w *respWriterWrapper) WriteHeader(statusCode int) { } w.ResponseWriter.WriteHeader(statusCode) } + +func (w *respWriterWrapper) Flush() { + if !w.wroteHeader { + w.WriteHeader(http.StatusOK) + } + + if f, ok := w.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} diff --git a/instrumentation/net/http/otelhttp/wrap_test.go b/instrumentation/net/http/otelhttp/wrap_test.go new file mode 100644 index 00000000000..3fff3d1faf1 --- /dev/null +++ b/instrumentation/net/http/otelhttp/wrap_test.go @@ -0,0 +1,37 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otelhttp + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRespWriterWriteHeader(t *testing.T) { + rw := &respWriterWrapper{ + ResponseWriter: &httptest.ResponseRecorder{}, + record: func(int64) {}, + } + + rw.WriteHeader(http.StatusTeapot) + assert.Equal(t, http.StatusTeapot, rw.statusCode) + assert.True(t, rw.wroteHeader) + + rw.WriteHeader(http.StatusGone) + assert.Equal(t, http.StatusTeapot, rw.statusCode) +} + +func TestRespWriterFlush(t *testing.T) { + rw := &respWriterWrapper{ + ResponseWriter: &httptest.ResponseRecorder{}, + record: func(int64) {}, + } + + rw.Flush() + assert.Equal(t, http.StatusOK, rw.statusCode) + assert.True(t, rw.wroteHeader) +} From 8e0ad8898565e8c48d2a94970d5a5d5d1a658a7f Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Wed, 22 May 2024 09:36:17 +0200 Subject: [PATCH 2/3] add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90a375fa7a9..0604c9f9da8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed + +- The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634) + ## [1.27.0/0.52.0/0.21.0/0.7.0/0.2.0] - 2024-05-21 ### Added From 74e43bab3acce0469c3ed1f322396754b8fb074b Mon Sep 17 00:00:00 2001 From: dmathieu <42@dmathieu.com> Date: Wed, 22 May 2024 09:41:05 +0200 Subject: [PATCH 3/3] ensure a non-flushable response writer doesn't fail --- .../net/http/otelhttp/wrap_test.go | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/instrumentation/net/http/otelhttp/wrap_test.go b/instrumentation/net/http/otelhttp/wrap_test.go index 3fff3d1faf1..d4b89411a29 100644 --- a/instrumentation/net/http/otelhttp/wrap_test.go +++ b/instrumentation/net/http/otelhttp/wrap_test.go @@ -35,3 +35,26 @@ func TestRespWriterFlush(t *testing.T) { assert.Equal(t, http.StatusOK, rw.statusCode) assert.True(t, rw.wroteHeader) } + +type nonFlushableResponseWriter struct{} + +func (_ nonFlushableResponseWriter) Header() http.Header { + return http.Header{} +} + +func (_ nonFlushableResponseWriter) Write([]byte) (int, error) { + return 0, nil +} + +func (_ nonFlushableResponseWriter) WriteHeader(int) {} + +func TestRespWriterFlushNoFlusher(t *testing.T) { + rw := &respWriterWrapper{ + ResponseWriter: nonFlushableResponseWriter{}, + record: func(int64) {}, + } + + rw.Flush() + assert.Equal(t, http.StatusOK, rw.statusCode) + assert.True(t, rw.wroteHeader) +}