feat: Add optional HTTP Middleware function to StartSettings for serverimpl #263
Conversation
…erimpl Problem --------------------- Traces for HTTP requests to the opamp-go server break, see open-telemetry#253 Solution --------------------- - Add an HTTP Handler middleware function to `StartSettings` - If this function is configured, apply it in serverimpl's `Start` where the HTTP Handler is set - (add unit tests) Code review notes --------------------- - This is a step in addressing open-telemetry#253 but mostly just for HTTP clients and requests. There is likely more to do for maintaining trace linkage through requests that come over websocket connections - I figured if users are using `Attach` instead of `Start`, they might have their own middleware configured for their HTTP server, so it makes more sense to hook this into `StartSettings` and the `Start` function
gdfast
force-pushed
the
gfast-add-server-middleware-hook
branch
from
e1d08d7
to
bdd080a
Compare
|
I was curious why we weren't running into this and realized that we're using Attach. I'm fine with adding this, but I would expect most users to use Attach. @tigrannajaryan @evan-bradley please have a look. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #263 +/- ##
==========================================
+ Coverage 74.70% 74.98% +0.27%
==========================================
Files 25 25
Lines 1625 1631 +6
==========================================
+ Hits 1214 1223 +9
+ Misses 299 297 -2
+ Partials 112 111 -1 ☔ View full report in Codecov by Sentry. |
@gdfast Can you please post the outline of the full plan with all the steps? I want to see the big picture before we merge this PR. |
Please sign the CLA. |
Working on it, it's in progress. I'm signing on as a Corprorate Contributor and am waiting for my company's CLA Manager |
My company/team has some HTTP Middleware code that adds OTel instrumentation that I'm planning to use in our OpAMP service that is built around opamp-go. I'm planning to follow up with my leads/management about contributing some version of this middleware in a separate, public Go module as you recommended, but I don't know what they'll say. I think giving users the ability to add their own HTTP Middleware is net good though, right? Meanwhile, I'll need to start looking into how to propagate context for the messages that come in on a websocket connection. I imagine we may need some similar hook to add middleware in order to facilitate Otel Go SDK instrumentation, but I'm not sure. I think I mentioned that I'm pretty new to Go and OTel instrumentation, so I'm learning as I go |
If you end up not contributing your implementation to Otel I would still want to see some example showing how the new capability can be used. It can be a very minimal example, just to demonstrate the idea. |
Sure thing. Where did you have in mind for this? I could add to https://github.com/open-telemetry/opamp-go/blob/main/internal/examples/server/opampsrv/opampsrv.go but I want to be careful not to add the OTel Go SDK to the repo unnecessarily. I guess adding to the examples directory is fine though? |
Yes, it is fine to add to opampsrv in the examples directory since it is a separate Go module. The example can use the Otel Go SDK. |
@tigrannajaryan I went with a very minimal example of using |
|
Hoping this is good to go now @tigrannajaryan . Thanks for your guidance and feedback! |
|
Thank you @gdfast |
Problem
Traces for HTTP requests to the opamp-go server break, see #253
Solution
StartSettingsStartwhere the HTTP Handler is setCode review notes
Attachinstead ofStart, they might have their own middleware configured for their HTTP server, so it makes more sense to hook this intoStartSettingsand theStartfunction