Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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 @@ -16,6 +16,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`. (#8258)
Comment thread
adomaskizogian marked this conversation as resolved.
Outdated

### 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 @@ -117,11 +118,24 @@ 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 resulting body is nil or is NoBody or GetBody returns an error, 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 {
var body io.ReadCloser
if r.GetBody != nil {
b, err := r.GetBody()
if err != nil {
otel.Handle(fmt.Errorf("http.Request GetBody returned an error: %w", err))
Comment thread
dmathieu marked this conversation as resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

} else {
body = b
}
} else {
body = r.Body
}

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

Copy link
Copy Markdown
Member

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 {
Comment thread
dmathieu marked this conversation as resolved.
r.Body = bw
}

Expand Down
78 changes: 78 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,80 @@ 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)
}()

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
}))
defer server.Close()

r, err := http.NewRequestWithContext(t.Context(), http.MethodPost, server.URL, http.NoBody)
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.ErrorContains(t, testErrHandler.err, "http.Request GetBody returned an error: GetBody error")
}