opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[service] deprecate TelemetrySettings.MeterProvider

Open codeboten opened this issue 1 year ago • 8 comments

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

codeboten avatar Aug 19 '24 21:08 codeboten

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

codeboten avatar Aug 19 '24 21:08 codeboten

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

codeboten avatar Aug 20 '24 18:08 codeboten

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.

codecov[bot] avatar Aug 20 '24 19:08 codecov[bot]

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

codeboten avatar Aug 21 '24 21:08 codeboten

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.

TylerHelmuth avatar Aug 21 '24 21:08 TylerHelmuth

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 avatar Aug 22 '24 08:08 mx-psi

@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.

TylerHelmuth avatar Aug 22 '24 16:08 TylerHelmuth

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.

codeboten avatar Aug 22 '24 16:08 codeboten

I think we are ready to merge this after the conflicts are fixed

mx-psi avatar Aug 29 '24 10:08 mx-psi