Skip to content
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

[service] deprecate TelemetrySettings.MeterProvider #10912

Merged

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Aug 19, 2024

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 #9510

@codeboten
Copy link
Contributor Author

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

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks sensible to me. 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)

@codeboten codeboten marked this pull request as ready for review August 20, 2024 18:44
@codeboten codeboten requested review from a team and bogdandrutu August 20, 2024 18:44
@codeboten
Copy link
Contributor Author

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 added a commit to codeboten/opentelemetry-collector that referenced this pull request Aug 20, 2024
This is just splitting a change from open-telemetry#10912 to make it easier to review

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Copy link

codecov bot commented Aug 20, 2024

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 ⚠️
component/componenttest/obsreporttest.go 0.00% 2 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codeboten added a commit to codeboten/opentelemetry-collector that referenced this pull request Aug 20, 2024
This func will MeterProvider and MetricsLevel in the near future. Split from open-telemetry#10912

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit that referenced this pull request Aug 20, 2024
This is just splitting a change from #10912 to make it easier to review

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit to codeboten/opentelemetry-collector that referenced this pull request Aug 20, 2024
This will use the LeveledMeterProvider in TelemetrySettings.

Split from open-telemetry#10912, follows open-telemetry#10931

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit that referenced this pull request Aug 20, 2024
This func will MeterProvider and MetricsLevel in the near future. Split
from
#10912

---------

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten force-pushed the codeboten/level-internal-telemetry branch from a7e221f to 3ec1ec1 Compare August 20, 2024 21:41
codeboten added a commit that referenced this pull request Aug 21, 2024
This will use the LeveledMeterProvider in TelemetrySettings.
    
Split from
#10912,
follows #10931

---------

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten force-pushed the codeboten/level-internal-telemetry branch from 3ec1ec1 to 6c2449e Compare August 21, 2024 15:33
codeboten added a commit to codeboten/opentelemetry-collector that referenced this pull request Aug 21, 2024
This is split from open-telemetry#10912

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
codeboten added a commit that referenced this pull request Aug 21, 2024
This is split from
#10912

---------

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten force-pushed the codeboten/level-internal-telemetry branch from fcdf7a5 to 8788129 Compare August 21, 2024 17:13
@codeboten codeboten changed the title [service] deprecate TelemetrySettings.MetricsLevel [service] deprecate TelemetrySettings.MeterProvider Aug 21, 2024
@codeboten
Copy link
Contributor Author

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

@TylerHelmuth
Copy link
Member

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.

@mx-psi
Copy link
Member

mx-psi commented Aug 22, 2024

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?

@TylerHelmuth
Copy link
Member

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

@codeboten
Copy link
Contributor Author

codeboten commented Aug 22, 2024

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.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

You are right, I think this would have been equally hard before this change

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.

Thanks for the links! After taking a look I also feel like this is the best solution I can think of

@mx-psi
Copy link
Member

mx-psi commented Aug 29, 2024

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

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.

NOTE: there's still additional changes needed here, opening this to gather
feedback.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten force-pushed the codeboten/level-internal-telemetry branch from 31a8ec0 to f26375b Compare August 29, 2024 16:23
component/telemetry.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@codeboten codeboten merged commit 7da6b61 into open-telemetry:main Aug 29, 2024
47 of 49 checks passed
@codeboten codeboten deleted the codeboten/level-internal-telemetry branch August 29, 2024 19:05
@github-actions github-actions bot added this to the next release milestone Aug 29, 2024
mx-psi added a commit that referenced this pull request Aug 30, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Fixes #10277

Depends on #10912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[component] TelemetrySettings.MetricsLevel is experimental
3 participants