[chore] RFC: move /debug/tracez registration to telemetry implementation #13947
Conversation
axw
changed the title
axw
force-pushed
the
zpages-host-spanprocessor-registry
branch
2 times, most recently
from
0cb1a75 to
fcf3ac0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13947 +/- ##
==========================================
- Coverage 91.67% 91.67% -0.01%
==========================================
Files 652 652
Lines 42516 42509 -7
==========================================
- Hits 38978 38968 -10
- Misses 2730 2732 +2
- Partials 808 809 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Instead of having zpagesextension register a zpages span processor on the tracer provider, make this the responsibility of the telemetry implementation. To make this happen, the service now creates the zpages mux and exposes it as a host capability. Components may register their own zpages routes, which the zpagesextension does for exposing expvar; and otherwise the zpagesextension just exists to expose the HTTP endpoint.
axw
force-pushed
the
zpages-host-spanprocessor-registry
branch
from
fcf3ac0 to
69a5a52
Compare
| mux.HandleFunc(path.Join(pathPrefix, zServicePath), host.zPagesRequest) | ||
| mux.HandleFunc(path.Join(pathPrefix, zPipelinePath), host.Pipelines.HandleZPages) | ||
| mux.HandleFunc(path.Join(pathPrefix, zExtensionPath), host.ServiceExtensions.HandleZPages) | ||
| mux.HandleFunc(path.Join(pathPrefix, zFeaturePath), handleFeaturezRequest) |
There was a problem hiding this comment.
I don't have a very strong opinion about this and I doubt it has an impact on performance, but it feels inelegant to run all this code unconditionally, even when the zpagesextension isn't enabled. Instead of this, would it be possible to have Host.RegisterZPages call a new RegisterZPages method on the telemetry.Factory? (Or does extension initialization take place too late to call into the telemetry factory?)
There was a problem hiding this comment.
Yes, the extension creation happens after the telemetry is created, so it's too late to do that. A couple of other options:
- Have the telemetry factory create a callback, and add it to
component.TelemetrySettings - Document that the
TracerProviderincomponent.TelemetrySettingscan implement an optional interface with a methodRegisterZPages(*http.ServeMux)
Those both seem a bit more convoluted to me, but I'm open to changing it. I agree it would be ideal if the span processor would only be added when the extension is enabled.
There was a problem hiding this comment.
Document that the TracerProvider in component.TelemetrySettings can implement an optional interface with a method RegisterZPages(*http.ServeMux)
If we're willing to implement optional interfaces on the TracerProvider, we could apply my previous suggestion and add an Unwrap method on componentattribute's TracerProvider. That way the zpagesextension can access the underlying SDK's provider, and keep the current logic for the most part.
I can write a PoC of that concept over in #13948 if you want.
There was a problem hiding this comment.
Although I'm not a big fan of optional interfaces in general it does seem like a practical option. I'd be happy to see a PoC for comparison. I'm mildly concerned about making it easy to accidentally not expose the SpanProcessor registry, but it's probably not a big deal.
Yet another option: extend what I've done in this PR, but rather than pass an *http.ServeMux via telemetry.Settings and have telemetry and the Host unconditionally register routes with it, make it callback style: we would instead pass in an interface with which telemetry (and other components) can register a callback for later registering routes. That would only be called if zpages is enabled.
Finally: the coupling between telemetry, host, and the zpages extension makes me wonder if the zpages endpoint should really be in an extension. I could see a few alternatives:
- Move it all to the telemetry provider: doesn't seem great, since zPages is mostly independent of the telemetry provider implementation; only /debug/tracez is relevant. I would rule this option out.
- Move it all to the service package. Seems OK. The service package would know whether zPages is enabled via its own config, and could skip various things.
- Move it all to the zpages extension. I like this one, but I don't know how possible/practical it is. I have in mind that the zpages extension would implement the component status watcher interface and use that to expose pipeline details. I don't know if component status provides enough details.
I think option 3 would need the approach you've described.
There was a problem hiding this comment.
I filed #14006 as a PoC for the Unwrap option for comparison.
|
Closing in favour of #14006 |
…nsion (#14006) #### Description This allows components to access SDK-specific `TracerProvider` methods (in `zpagesextension`'s case, `RegisterSpanProcessor`), without requiring `componentattribute` to have a dependency on the official SDK. This is an alternative proposal to #13947 #### Link to tracking issue Updates #13842 #### Testing Should be covered by `TestServiceTelemetryZPages`. (In fact the "missing coverage" should be covered by that test as well, but it's in another module.) --------- Co-authored-by: Pablo Baeyens <[email protected]>
2 participants
Description
Instead of having zpagesextension register a zpages span processor on the tracer provider, make this the responsibility of the telemetry implementation. This will enable us to remove the dependency from various components on the SDK.
To make this happen, the service package now creates the zpages mux and exposes it as a host capability. The host itself registers routes for
/debug/servicez,/debug/pipelinez,/debug/extensionz, and/debug/featurez. The otelconftelemetry package registers/debug/tracez. Components may register their own zpages routes, which the zpagesextension does for exposing expvar; otherwise the zpagesextension just exists to expose the HTTP endpoint.Link to tracking issue
Relates to #13842
Testing
Ran the collector with and without the zpages extension. Does nothing as expected when disabled. When enabled, confirmed that
/debug/servicez,/debug/tracez, and/debug/expvarz(when configured) work as expected.Documentation
N/A