opentelemetry-go-contrib
opentelemetry-go-contrib copied to clipboard
open-telemetry
Should we have expert, or generalist handlers?
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 a TraceHandler, 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:
WithTraceStreamInterceptorWithTraceUnaryInterceptorWithMetricStreamInterceptorWithMetricUnaryInterceptor
And deprecate the existing WithStreamInterceptor and WithUnaryInterceptor 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 and WithUnaryInterceptor.
Under the hood however, we would setup 4 private methods:
traceStreamInterceptortaceUnaryInterceptormetricStreamInterceptormetricUnaryInterceptor
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.
I stand with proposal B, because I think we can use WithTracerProvider and WithMeterProvider to control whether to enable trace or metric.
For gRPC instrumentation I suggest to consider doing this refactoring as part of the change https://github.com/open-telemetry/opentelemetry-go-contrib/issues/197.
I stand with proposal B, because I think we can use
WithTracerProviderandWithMeterProviderto control whether to enable trace or metric.
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 otelmux instead of otelhttp, but it does only traces. I would like to use only the metrics of otelhttp, instead of adding another layer of traces for the same thing.
I would prefer Option A, adding an option of having a "combined" handler also, to avoid breaking current code.
The public handler would analyse whether metrics and traces are configured or not, and automatically load the appropriate handlers.
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.