opamp-go server impl is breaking traces #253
Comments
|
Thanks for opening this. I support instrumenting opamp-go with one caveat: I don't want Otel Go SDK to be a required dependency. It should be possible to use opamp-go without Otel Go SDK. The reason for is that we believe it is important for OpAMP to be possible to use even if you are not an OpenTelemetry user otherwise. There are probably a couple options:
I am open to exploring other options. @open-telemetry/opamp-go-maintainers @open-telemetry/opamp-go-approvers thoughts? |
|
Thanks for the note Tigran, I'm glad I read it before trying to implement anything! And I see what you're going for with avoiding the OTel Go SDK dependency. I'm a bit new to golang but I'll check out those two options you shared. |
gdfast
added a commit
to gdfast/opamp-go
that referenced
this issue
Mar 15, 2024
…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
added a commit
to gdfast/opamp-go
that referenced
this issue
Mar 15, 2024
…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
tigrannajaryan
pushed a commit
that referenced
this issue
Mar 22, 2024
…erimpl (#263) Problem --------------------- Traces for HTTP requests to the opamp-go server break, see #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 #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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Scenario
We have an Envoy gateway in front of an OpAMP server built on top of opamp-go, specifically using Start. The gateway is instrumented to emit OTLP spans. However, when Envoy sends messages to the OpAMP server, the trace terminates with Envoy, and there's an entirely new trace that starts in the OpAMP Server (where we've instrumented spans as part of the server's callbacks).
Problem
Broken traces make for more challenging debugging and monitoring of the OpAMP server
Goal
Minimum: opamp-go's server implementation persists traceID in its context
ideal: opamp-go's server impl, in addition to persisting traceID, also instruments spans
Some solution ideas
I think given that opamp-go is an OpenTelemetry project, it makes sense to Do The Right Thing and update serviceimpl.go – particular the
Handler– to extract baggage and context from the request span and instrument a new server span.A more flexible approach would be to organize the code to allow apps built on top of opamp-go server to inject Middleware. However, I think opamp-go's server should maintain trace integrity, by default, so I wouldn't vote for this as Plan A.
The text was updated successfully, but these errors were encountered: