Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
20 changes: 17 additions & 3 deletions instrumentation/net/http/otelhttp/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package otelhttp // import "go.opentelemetry.io/contrib/instrumentation/net/http

import (
"context"
"fmt"
"io"
"net/http"
"net/http/httptrace"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just return the error, as the stdlib does?

https://github.com/golang/go/blob/go1.25.6/src/net/http/client.go#L673

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two issues, actually:

  • If I use transport := http.DefaultTransport and set a breakpoint in the GetBody impl in that test, it is not triggered by the test execution. But it is triggered with the OTEL transport. This means OTEL transport calls GetBody when stdlib does not. stdlib only calls it if it needs another instance of the body (i.e. for second, third, etc attempts).
  • stdlib returns the error from GetBody, it does not proceed. OTEL transport reports it and proceeds.

The unit test encodes incorrect behavior.

My understanding is that the code should:

  • wrap the body that is set in the request, if it is not nil and not NoBody.
  • wrap the GetBody func but not call it. The underlying transport will call it if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

} else {
body = b
}
}

bw := request.NewBodyWrapper(body, func(int64) {})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should only be allocated if it is actually needed - to reduce RAM usage.

Also, this object is unconditionally used below - it shouldn't be if it is not used as a body.

if body != nil && body != http.NoBody {
r.Body = bw
}

Expand Down
88 changes: 88 additions & 0 deletions instrumentation/net/http/otelhttp/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}