-
Notifications
You must be signed in to change notification settings - Fork 579
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
Should we have expert, or generalist handlers? #2782
Comments
I stand with proposal B, because I think we can use |
For gRPC instrumentation I suggest to consider doing this refactoring as part of the change #197. |
about this option, I faced problems with trying to use a Noop to disable only traces, I was told that this is not what they are for. |
The separation of the handlers is useful in the cases, for example, when using |
I would prefer Option A, adding an option of having a "combined" handler also, to avoid breaking current code. |
How would this work? Is it possible to tell if a FooProvider is the Noop implementation? The other potential problem is that even if a Noop is currently registered, a non-noop provider could be registered in the future: https://github.com/open-telemetry/opentelemetry-go/blob/4580e06de09960506d9ecb36da59979b10d0fda2/internal/global/meter.go#L62 |
My thinking at this point is that both metrics and traces would be enabled by default, with options to disable them. |
Background
Right now, our handlers are generalists. They perform both tracing and metrics (with some/most handlers only doing tracing).
This is introducing some logic duplication in those specific bits of code, as some metrics reproduce the same behavior a trace is going to provide.
For HTTP handlers for example, the specification is asking us to provide metrics that can already be correlated from traces.
So it seems to me (that may just be me) that folks using tracing will not care about the duplicated data in metrics.
Doing both traces and metrics in the same code blocks also makes the libraries a bit harder to read. And a better separation of concerns would improve readability.
Hence this proposal to offer alternative ways.
Proposal A
This first proposal consists in providing two handlers for each instrumentation. a
MetricHandler
, and aTraceHandler
, each managing only the appropriate signal.Someone loading only metrics will not get any traces from their component instrumentation.
For example, the grpc instrumentation will introduce 4 new methods:
WithTraceStreamInterceptor
WithTraceUnaryInterceptor
WithMetricStreamInterceptor
WithMetricUnaryInterceptor
And deprecate the existing
WithStreamInterceptor
andWithUnaryInterceptor
methods (which can alias to the tracer ones during the deprecation period since this specific instrumentation has no metrics. Instrumentations with metrics may need to duplicate their code during that period).Proposal B
This second proposal consists in still providing a single public handler, with private handlers that would be loaded only if metric/tracer providers are configured.
There would be no visible change to users of the library, they would keep using
WithStreamInterceptor
andWithUnaryInterceptor
.Under the hood however, we would setup 4 private methods:
traceStreamInterceptor
taceUnaryInterceptor
metricStreamInterceptor
metricUnaryInterceptor
The public handler would analyse whether metrics and traces are configured or not, and automatically load the appropriate handlers.
This proposal doesn't require any deprecations, since no public methods would need to be changed.
Proposal C: Do nothing
This third proposal keeps the current status quo. Each instrumentation has a single handler, which can do both traces and metrics.
The text was updated successfully, but these errors were encountered: