-
Notifications
You must be signed in to change notification settings - Fork 692
Honor otelhttp.WithMessageEvents(otelhttp.ReadEvents) option with otelhttp.NewTransport(...) #7513
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.
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
base: main
Are you sure you want to change the base?
Honor otelhttp.WithMessageEvents(otelhttp.ReadEvents) option with otelhttp.NewTransport(...) #7513
Conversation
…lhttp.NewTransport(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need a changelog entry.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7513 +/- ##
=====================================
Coverage 81.5% 81.6%
=====================================
Files 200 200
Lines 18129 18133 +4
=====================================
+ Hits 14793 14803 +10
+ Misses 2938 2932 -6
Partials 398 398
🚀 New features to boost your workflow:
|
The linter is now failing :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall.
Co-authored-by: Damien Mathieu <[email protected]>
if len(spans) != 1 { | ||
t.Fatalf("expected 1 span; got: %d", len(spans)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use `require.Len`` instead?
if err != nil { | ||
t.Fatal(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not require.NoError
?
My primary goal is for http client requests to trace time-to-first-response-byte, and there is very likely a much more efficient method to capture just this detail. The time to the first response byte is important for large streaming responses where it is far less helpful to record only the moment after the entire response body has been completely read.
However, using the existing pattern established by
otelhttp.NewHandler(..., otelhttp.WithMessageEvents(otelhttp.ReadEvents))
for http server requests and extending it to also work withotelhttp.NewTransport
the same information (and more) becomes available for a very small code change and no API change.I looked for existing tests for Handler and WithMessageEvents to replicate to Transport but did not find any so I'm not sure what the preferred pattern would be for adding unit tests for this functionality.