-
Notifications
You must be signed in to change notification settings - Fork 588
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
Setup body wrapper in otelmux #6650
base: main
Are you sure you want to change the base?
Setup body wrapper in otelmux #6650
Conversation
c0b62ee
to
21809eb
Compare
instrumentation/github.com/gorilla/mux/otelmux/request/resp_writer_wrapper.go
Fixed
Show fixed
Hide fixed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6650 +/- ##
=======================================
+ Coverage 68.7% 68.8% +0.1%
=======================================
Files 200 202 +2
Lines 16872 16968 +96
=======================================
+ Hits 11601 11690 +89
- Misses 4927 4933 +6
- Partials 344 345 +1
|
// TODO: The wrapped http.ResponseWriter doesn't implement any of the optional | ||
// types (http.Hijacker, http.Pusher, http.CloseNotifier, etc) | ||
// that may be useful when using it in real life situations. | ||
type RespWriterWrapper struct { |
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.
Does this still apply, since the way you wrap the default writer preserves the original implemented interfaces. Why would you want to implement them in RespWriterWrapper directly.
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.
Note that this package is a copy from the otelhttp one.
https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/net/http/otelhttp/internal/request
Which makes me think maybe it should be templatized instead, to ensure things remain in sync.
setup and implementation of a body wrapper in the
go.opentelemetry.io/contrib/instrumentation/github.com/gorilla/mux/otelmux
packageThis PR addresses feedback provided in #6648