opentelemetry-collector
opentelemetry-collector copied to clipboard
[service] deprecate TelemetrySettings.MeterProvider
This is replaced by a LeveledMeterProvider method on the struct instead. This reduces the complexity from the view of component authors, in that there's no need to check the level before invoking the meter to record a metric.
Closes https://github.com/open-telemetry/opentelemetry-collector/issues/9510
Mainly what I'm looking for feedback on is https://github.com/open-telemetry/opentelemetry-collector/pull/10912/files#diff-8992ea9c665081035bd866a386bc9510ee77af8cbb7137182e0fbcf38b92d64a and the changes in service.go
The generated code needs some more fixing. Pinging @mx-psi as this is following the proposed approach in #9510
We give up on being able to have different metrics on the same 3rd party library being spread across different levels, but I think that's okay (we are not in the business of deciding 3rd party libraries telemetry for them)
Is this something we had the ability to do previous? We don't have a mechanism for using the levels at the instrumentation library level, users can always configure views if they need additional granularity
Codecov Report
Attention: Patch coverage is 94.31818% with 5 lines in your changes missing coverage. Please review.
Project coverage is 92.03%. Comparing base (
f641f23) to head (2a3c587). Report is 5 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| component/componenttest/nop_telemetry.go | 25.00% | 2 Missing and 1 partial :warning: |
| component/componenttest/obsreporttest.go | 0.00% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #10912 +/- ##
==========================================
+ Coverage 91.80% 92.03% +0.22%
==========================================
Files 412 412
Lines 19357 19313 -44
==========================================
+ Hits 17770 17774 +4
+ Misses 1224 1185 -39
+ Partials 363 354 -9
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
After spending a long time discussing various options, I think the best way to move forward is to replace MeterProvider with LeveledMeterProvider, and keep MetricsLevel.
Adding LeveledMeterProvider gives slightly better ergonomics for component authors, as they may need to access a meter provider directly (for components that don't use mdatagen). Doing this prior to this change, required a check for a level where now just passing the level to the LeveledMeterProvider method is enough.
I tried a few different ways to remove the MetricsLevel, however there are a few components that rely on this information to decide when to make expensive computations (prior to recording). I considered replacing this with a callback that could be set a component instantiation, but that would still require the component (or whatever is doing the recording, telemetryBuilder or whatever) to be aware of the MetricsLevel. As the components themselves instantiate a telemetry builder, they would still need access to the telemetry level, which doesn't save anything.
If others have ideas of how to make this easier/better, I'm all ears, pinging @open-telemetry/collector-approvers for reviews/input
I spent time thinking about this as well and haven't come up with a better solution that a field on the struct for how to give component access to the actual level. We have use cases today that require knowing the actual configured telemetry level, and introducing more structs/interfaces/methods all felt more complex and unnecessary than leaving MetricsLevel.
there are a few components that rely on this information to decide when to make expensive computations (prior to recording).
Do you have a link for those? I am guessing these need to be recorded by sync instruments, right?
@mx-psi the batchprocessor uses it to check later if it should calculate a value.
The other is otelarrow, doing similar checks to see if if should do some math. The situation is more complex, but this calculation only ends up happening based on the metrics level.
Do you have a link for those? I am guessing these need to be recorded by sync instruments, right?
Right in both cases they're synchronous instruments. I thought of allowing callbacks to be passed in as a workaround but I'm not sure if it's worth the effort.
I think we are ready to merge this after the conflicts are fixed