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] Add service::telemetry::metrics::views config key #12433

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Feb 19, 2025

Description

This PR adds a service::telemetry::metrics::views config key, which explicitly sets the list of metric views used for internal telemetry, mirroring meter_provider::views in the SDK config. This can be used to disable specific internal metrics, among other uses.

This key will cause an error if used alongside other features which would normally implicitly create views, such as:

  • not setting service::telemetry::metrics::level to detailed;
  • enabling the telemetry.disableHighCardinalityMetrics feature gate.

Further discussion needed

  • A comment notes that the telemetry.disableHighCardinalityMetrics alpha gate "will be removed when the collector allows for view configuration". I think setting the gate as deprecated first would be the correct thing to do, but it means that users relying on it will see their Collectors crash on update. Is that okay?

  • In the context of being able to enable/disable specific metrics, this key is only useful to disable metrics from an "all enabled" baseline. It cannot easily be used to customize the set of metrics emitted at level: normal, level: basic. Discussion is ongoing in Own-telemetry: Ability to enabled/disable individual metrics #10769 on how to handle that, but the solution will probably involve a new key, which should hopefully be backward-compatible with the user-visible changes in this PR.

Link to tracking issue

Updates #10769

Testing

None yet

Documentation

None yet, except the changelog

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 65.21739% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (dae5d19) to head (099fbd2).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
service/service.go 75.00% 4 Missing and 1 partial ⚠️
service/telemetry/config.go 0.00% 2 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (65.21%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12433      +/-   ##
==========================================
- Coverage   92.19%   92.16%   -0.04%     
==========================================
  Files         465      465              
  Lines       25285    25285              
==========================================
- Hits        23311    23303       -8     
- Misses       1575     1581       +6     
- Partials      399      401       +2     

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

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.

Can you increase the test coverage? Could you also add information about this (in a draft PR) to opentelemetry.io docs?

@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Feb 20, 2025

Regarding the lack of test coverage: it's almost entirely from the existing code for the feature gate that I just moved. I can try to add a new test for it; is there a way to set feature gates in tests?

@mx-psi
Copy link
Member

mx-psi commented Feb 20, 2025

Regarding the lack of test coverage: it's almost entirely from the existing code for the feature gate that I just moved. I can try to add a new test for it; is there a way to set feature gates in tests?

You can check the tests that are modified in #12377

Comment on lines +114 to +115
if c.Metrics.Views != nil && c.Metrics.Level != configtelemetry.LevelDetailed {
return errors.New("service::telemetry::metrics::views can only be set when service::telemetry::metrics::level is detailed")
Copy link
Member

Choose a reason for hiding this comment

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

Why this? I think we should allow views only if the level is not set.

Copy link
Contributor Author

@jade-guiton-dd jade-guiton-dd Feb 21, 2025

Choose a reason for hiding this comment

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

The way we implement levels is by adding views that filter out unwanted metrics. But setting views explicitly completely replaces the default views (for now, we may decide to merge them later). There is discussion of this in the tracking issue, but I thought it would be confusing to add an empty views section and suddenly see more metrics than before. For that reason, I decided to only allows views with level: detailed, which enables all metrics and thus sets no views by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was also the idea of adding a new level: custom value, so users explicitly declare their intent to bypass the level-based system and use more advanced view-based configuration, not sure what you think of it.

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.

3 participants