Skip to content

otelhttp: Remove custom wrapper after handling request #6914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VirrageS wants to merge 1 commit into
base: main
Choose a base branch
from
Open

otelhttp: Remove custom wrapper after handling request #6914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Changed

- Jaeger remote sampler's probabilistic strategy now uses the same sampling algorithm as `trace.TraceIDRatioBased` in `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#6892)
- Remove the custom body wrapper from the request's body after the request is processed to allow body type comparisons with the original type in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` and `go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux`. (#6914)

### Removed

Expand Down
5 changes: 5 additions & 0 deletions instrumentation/github.com/gorilla/mux/otelmux/mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ func (tw traceware) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// ReadCloser fulfills a certain interface and it is indeed nil or NoBody.
bw := request.NewBodyWrapper(r.Body, readRecordFunc)
if r.Body != nil && r.Body != http.NoBody {
prevBody := r.Body
r.Body = bw

// Restore the original body after the request is processed to avoid issues
// with extra wrapper since `http/server.go` later checks type of `r.Body`.
defer func() { r.Body = prevBody }()
Copy link
Member

@dmathieu dmathieu Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is missing tests.

Copy link
Author

@VirrageS VirrageS Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Turns out that we don't really need this code in gorilla/mux as internally gorilla/mux does the following:

...
	if r.Match(req, &match) {
		handler = match.Handler
		req = requestWithVars(req, match.Vars)
		req = requestWithRoute(req, match.Route)
	}
...

func requestWithVars(r *http.Request, vars map[string]string) *http.Request {
	ctx := context.WithValue(r.Context(), varsKey, vars)
	return r.WithContext(ctx)
}

func requestWithRoute(r *http.Request, route *Route) *http.Request {
	ctx := context.WithValue(r.Context(), routeKey, route)
	return r.WithContext(ctx)
}

and r.WithContext(ctx) does a shallow copy of the http.Request so that the processed request is different than the original request and therefore r.Body is a different pointer (but still pointing to the same memory). Thus, original r.Body is left untouched.

But I would still leave this code just in case something changes in the future in gorilla/mux and for consistency with net/http code.

}

writeRecordFunc := func(int64) {}
Expand Down
21 changes: 21 additions & 0 deletions instrumentation/github.com/gorilla/mux/otelmux/mux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/gorilla/mux"
"github.com/stretchr/testify/assert"

"go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux/internal/request"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/trace"
Expand All @@ -28,6 +29,26 @@ var sc = trace.NewSpanContext(trace.SpanContextConfig{
TraceFlags: trace.FlagsSampled,
})

func TestRequestBodyWrapper(t *testing.T) {
router := mux.NewRouter()
router.Use(Middleware("foobar"))
router.HandleFunc("/user/{id}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, ok := r.Body.(*request.BodyWrapper)
assert.Truef(t, ok, "body should be wrapped when request is processed")

w.WriteHeader(http.StatusOK)
}))

r := httptest.NewRequest("POST", "/user/123", strings.NewReader(`{"name":"John Doe","age":30}`))
r = r.WithContext(trace.ContextWithRemoteSpanContext(context.Background(), sc))
w := httptest.NewRecorder()

router.ServeHTTP(w, r)

_, ok := r.Body.(*request.BodyWrapper)
assert.Falsef(t, ok, "body should not be wrapped after request is processed")
}

func TestPassthroughSpanFromGlobalTracer(t *testing.T) {
var called bool
router := mux.NewRouter()
Expand Down
5 changes: 5 additions & 0 deletions instrumentation/net/http/otelhttp/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,12 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http
// ReadCloser fulfills a certain interface and it is indeed nil or NoBody.
bw := request.NewBodyWrapper(r.Body, readRecordFunc)
if r.Body != nil && r.Body != http.NoBody {
prevBody := r.Body
r.Body = bw

// Restore the original body after the request is processed to avoid issues
// with extra wrapper since `http/server.go` later checks type of `r.Body`.
defer func() { r.Body = prevBody }()
}

writeRecordFunc := func(int64) {}
Expand Down
3 changes: 3 additions & 0 deletions instrumentation/net/http/otelhttp/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/request"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/propagation"
Expand Down Expand Up @@ -99,6 +100,8 @@ func TestHandler(t *testing.T) {
rr := httptest.NewRecorder()
tc.handler(t).ServeHTTP(rr, r)
assert.Equal(t, tc.expectedStatusCode, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
_, ok := r.Body.(*request.BodyWrapper)
assert.Falsef(t, ok, "body should not be wrapped after request is processed")
})
}
}
Expand Down
Loading