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

[EXPORTER] Add option to disable Prometheus otel_scope_name and otel_scope_version attributes #2451

Merged
merged 4 commits into from
Dec 16, 2023

Conversation

timwoj
Copy link
Contributor

@timwoj timwoj commented Dec 13, 2023

Fixes #2442 (plus one bonus fix)

Changes

  • Adds a new populate_otel_scope option to the PrometheusExporterOptions object defaulting to enabled, including an environment variable to control it
  • Uses the option to enable/disable the otel_scope_name and otel_scope_version attributes added to instruments in Prometheus output
  • Adds a new environment variable for controlling the populate_target_info option of PrometheusExporterOptions

I opted to use (abuse) the side effect of the scope variable being null for the actual disabling of the attributes here, instead of passing the option down through the method chain to where it's used. If that's undesirable, I can switch to passing it.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@timwoj timwoj requested a review from a team December 13, 2023 22:09
Copy link

linux-foundation-easycla bot commented Dec 13, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@timwoj timwoj changed the title Prometheus disable otel scope Add option to disable Prometheus otel_scope_name and otel_scope_version attributes Dec 13, 2023
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Merging #2451 (d0b712f) into main (e79a10f) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2451   +/-   ##
=======================================
  Coverage   87.06%   87.06%           
=======================================
  Files         199      199           
  Lines        6087     6087           
=======================================
  Hits         5299     5299           
  Misses        788      788           

@timwoj timwoj force-pushed the prometheus-disable-otel-scope branch 2 times, most recently from 4984656 to fe7eca7 Compare December 13, 2023 23:21
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

exporters/prometheus/src/exporter_options.cc Outdated Show resolved Hide resolved
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

LGTM, see minor renaming.

@timwoj timwoj force-pushed the prometheus-disable-otel-scope branch from 53eafc8 to db79d41 Compare December 15, 2023 20:27
@lalitb lalitb enabled auto-merge (squash) December 15, 2023 22:35
@marcalff marcalff changed the title Add option to disable Prometheus otel_scope_name and otel_scope_version attributes [EXPORTER] Add option to disable Prometheus otel_scope_name and otel_scope_version attributes Dec 16, 2023
@marcalff marcalff added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Dec 16, 2023
@marcalff
Copy link
Member

@timwoj

Thanks for the fixes.

Please update this branch with the latest main changes.

The auto-merge flag is enabled, this PR should merge to main once updated.

auto-merge was automatically disabled December 16, 2023 15:04

Head branch was pushed to by a user without write access

@timwoj timwoj force-pushed the prometheus-disable-otel-scope branch from db79d41 to d0b712f Compare December 16, 2023 15:04
@marcalff marcalff merged commit e8afbb8 into open-telemetry:main Dec 16, 2023
46 checks passed
@marcalff
Copy link
Member

marcalff commented Dec 18, 2023

Reopen.

This collides with the following spec PR, which defines the option and environment variable to use:

@timwoj timwoj deleted the prometheus-disable-otel-scope branch December 24, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Prometheus Exporter] Unable to disable otel_scope_name attribute
3 participants