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

[Prometheus Exporter] Unable to disable otel_scope_name attribute #2442

Closed
timwoj opened this issue Dec 8, 2023 · 10 comments · Fixed by #2451 or #2479
Closed

[Prometheus Exporter] Unable to disable otel_scope_name attribute #2442

timwoj opened this issue Dec 8, 2023 · 10 comments · Fixed by #2451 or #2479
Assignees
Labels
bug Something isn't working help wanted Good for taking. Extra help will be provided by maintainers triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@timwoj
Copy link
Contributor

timwoj commented Dec 8, 2023

According to the spec for the Prometheus exporter:

Prometheus exporters SHOULD provide a configuration option to disable the otel_scope_info metric and otel_scope_ labels.

I can't find any way to actually disable these. The prometheus exporter would disable them assuming the strings passed in via the scope data are empty. The data in the InstrumentationScope that gets passed into the exporter comes from agruments passed to GetMeter(). Passing a blank string for library_name would ostensibly disable it based on the code in the exporter, but passing a blank there results in an error:

[Warning] File: <>/auxil/opentelemetry-cpp/sdk/src/metrics/meter_provider.cc:54 [MeterProvider::GetMeter] Library name is empty.
@timwoj timwoj added the bug Something isn't working label Dec 8, 2023
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 8, 2023
@timwoj timwoj changed the title [Prometheus Exporter] Unable to disable otel_scope attribute [Prometheus Exporter] Unable to disable otel_scope_name attribute Dec 8, 2023
@marcalff
Copy link
Member

See PrometheusExporterOptions::populate_target_info.

This can be set programatically.

The missing part is to expose an environment variable so that this property can also be set from the execution environment.

@timwoj
Copy link
Contributor Author

timwoj commented Dec 11, 2023

See PrometheusExporterOptions::populate_target_info.

This can be set programatically.

I set this to false and otel_scope_name is still part of every instrument in the output. It looks like all this setting does is remove the target_info instrument from being reported.

@marcalff
Copy link
Member

Indeed, populate_target_info is for something different, sorry about the confusion.

So we need a new exporter option attributes, and the environment variable that goes with it, this is a valid issue.

@marcalff marcalff added help wanted Good for taking. Extra help will be provided by maintainers triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 13, 2023
Copy link

This issue is available for anyone to work on. Make sure to reference this issue in your pull request.
✨ Thank you for your contribution! ✨

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

@marcalff marcalff reopened this Dec 18, 2023
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 18, 2023
@marcalff marcalff removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 18, 2023
@marcalff marcalff self-assigned this Dec 18, 2023
@marcalff marcalff added the issue:blocked Fix blocked, waiting for other fixes as prerequisites label Dec 18, 2023
@marcalff
Copy link
Member

On hold, waiting for the spec PR to be approved and merged:

options and environment variables to be renamed after that.

Important: to fix before the next opentelemetry-cpp release is shipped, to avoid un necessary changes to users.

@marcalff marcalff removed the issue:blocked Fix blocked, waiting for other fixes as prerequisites label Dec 19, 2023
@timwoj
Copy link
Contributor Author

timwoj commented Dec 21, 2023

Looks like the spec PR was merged. I'll try to get a PR that renames it done this week.

@timwoj
Copy link
Contributor Author

timwoj commented Dec 24, 2023

Looks like the spec PR was merged. I'll try to get a PR that renames it done this week.

Should I rename all of the populate_otel_scope variables in the original PR, or just the environment variable? I'm only asking because of the clash with the naming of populate__target_info.

@timwoj
Copy link
Contributor Author

timwoj commented Jan 3, 2024

Ping on the above question, which probably got lost in the holidays.

@marcalff
Copy link
Member

marcalff commented Jan 8, 2024

@timwoj

Everything related to populate_otel_scope is not published yet in opentelemetry-cpp, because it was merged after the last release 1.13.0.

So, please rename C++ member names, parameter names, etc, and the environment variable name.

Once this renaming is done, we can then ship a clean 1.14.0.

Not sure what conflict with populate_target_info this can cause, if any.

Given that the spec itself is in experimental status for this area, renaming can be done around populate_target_info if necessary, we just have to document this as a breaking change in the changelog (breaking changes in the SDK are acceptable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Good for taking. Extra help will be provided by maintainers triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
2 participants