diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c5308fcf7a..8c3430e1a75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Fix panic when passing nil `TracerProvider` or `MeterProvider` to `WithTracerProvider` or `WithMeterProvider` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#8323) +- `Transport` in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` now supports reading request body multiple times for subsequent requests that reuse `http.Request`. (#8352) ### Removed diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 514ae6753b7..1d2f4022116 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -5,6 +5,7 @@ package otelhttp // import "go.opentelemetry.io/contrib/instrumentation/net/http import ( "context" + "fmt" "io" "net/http" "net/http/httptrace" @@ -83,6 +84,8 @@ func defaultTransportFormatter(_ string, r *http.Request) string { // RoundTrip creates a Span and propagates its context via the provided request's headers // before handing the request to the configured base RoundTripper. The created span will // end when the response body is closed or when a read from the body returns io.EOF. +// If GetBody returns an error, the error is reported via otel.Handle and the request +// continues with the original Body. func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { requestStartTime := time.Now() for _, f := range t.filters { @@ -117,11 +120,22 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { r = r.Clone(ctx) // According to RoundTripper spec, we shouldn't modify the origin request. - // if request body is nil or NoBody, we don't want to mutate the body as it + // GetBody is preferred over direct access to Body if the function is set. + // If the resulting body is nil or is NoBody, we don't want to mutate the body as it // will affect the identity of it in an unforeseeable way because we assert // ReadCloser fulfills a certain interface and it is indeed nil or NoBody. - bw := request.NewBodyWrapper(r.Body, func(int64) {}) - if r.Body != nil && r.Body != http.NoBody { + body := r.Body + if r.GetBody != nil { + b, err := r.GetBody() + if err != nil { + otel.Handle(fmt.Errorf("http.Request GetBody returned an error: %w", err)) + } else { + body = b + } + } + + bw := request.NewBodyWrapper(body, func(int64) {}) + if body != nil && body != http.NoBody { r.Body = bw } diff --git a/instrumentation/net/http/otelhttp/transport_test.go b/instrumentation/net/http/otelhttp/transport_test.go index d4d20b2bfa5..bb03883c682 100644 --- a/instrumentation/net/http/otelhttp/transport_test.go +++ b/instrumentation/net/http/otelhttp/transport_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/propagation" @@ -1114,3 +1115,90 @@ func BenchmarkTransportRoundTrip(b *testing.B) { }) } } + +func TestRequestBodyReuse(t *testing.T) { + var bodies []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + assert.NoError(t, err) + + defer r.Body.Close() + bodies = append(bodies, string(body)) + + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + transport := NewTransport(http.DefaultTransport) + client := http.Client{Transport: transport} + + body := bytes.NewBuffer([]byte("hello")) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, server.URL, body) + require.NoError(t, err) + + for range 2 { + resp, err := client.Do(req) + assert.NoError(t, err, "failed to perform http req") + + _, err = io.Copy(io.Discard, resp.Body) + require.NoError(t, err) + + err = resp.Body.Close() + require.NoError(t, err) + } + + require.Len(t, bodies, 2, "expected exactly 2 req bodies") + assert.Equal(t, bodies[0], bodies[1], "req bodies do not match") +} + +type testErrHandler struct { + err error +} + +func (h *testErrHandler) Handle(err error) { h.err = err } + +func TestHandleErrorOnGetBodyError(t *testing.T) { + errHandler := otel.GetErrorHandler() + + testErrHandler := &testErrHandler{} + + otel.SetErrorHandler(testErrHandler) + defer func() { + otel.SetErrorHandler(errHandler) + }() + + var receivedBody string + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, err := io.ReadAll(r.Body) + assert.NoError(t, err) + + receivedBody = string(b) + + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + body := "plain text body" + + r, err := http.NewRequestWithContext(t.Context(), http.MethodPost, server.URL, strings.NewReader(body)) + require.NoError(t, err) + + getBodyErr := errors.New("GetBody error") + r.GetBody = func() (io.ReadCloser, error) { + return nil, getBodyErr + } + + transport := NewTransport(http.DefaultTransport) + client := http.Client{Transport: transport} + + resp, err := client.Do(r) + require.NotNil(t, resp) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, body, receivedBody, "should fallback to http.Request body on http.Request GetBody error") + assert.ErrorContains(t, testErrHandler.err, "http.Request GetBody returned an error: GetBody error") +}