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] prometheus: add otel_scope_name and otel_scope_version labels #2293

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Sep 1, 2023

Part of #1841

Changes

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#instrumentation-scope-1:

Prometheus exporters MUST add the scope name as the otel_scope_name label and the scope version as the otel_scope_version label on all metric points by default, based on the scope the original data point was nested in.

This adds the otel_scope_name and otel_scope_version labels to all metrics, if they are non-empty.

@dashpole dashpole force-pushed the prometheus_scope_labels branch 2 times, most recently from 232617f to 212071b Compare September 1, 2023 20:19
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #2293 (afa9009) into main (bd11434) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2293   +/-   ##
=======================================
  Coverage   87.39%   87.39%           
=======================================
  Files         199      199           
  Lines        6009     6009           
=======================================
  Hits         5251     5251           
  Misses        758      758           

@dashpole dashpole force-pushed the prometheus_scope_labels branch 4 times, most recently from 6f7795f to 58afc94 Compare September 1, 2023 20:57
@dashpole
Copy link
Contributor Author

dashpole commented Sep 5, 2023

[ RUN      ] PrometheusExporterUtils.TranslateToPrometheusIntegerCounter
unknown file: Failure
C++ exception with description "basic_string::_M_create" thrown in the test body.

Any pointers on what this might be?

Copy link
Contributor

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

EDIT: I was wrong about this. There is an implicit conversion to std::string. I am not sure what will happen in the case where OTel uses the standard library though.


Also, maybe related, this part of the implementation is error prone:

: name_(name), version_(version), schema_url_(schema_url), attributes_(std::move(attributes))

This assumes that the string_views are null-terminated, which might not be the case. I can send a PR to fix it.

exporters/prometheus/src/exporter_utils.cc Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
@dashpole dashpole force-pushed the prometheus_scope_labels branch 3 times, most recently from bdcb870 to 4fe16cf Compare September 18, 2023 16:03
Copy link
Contributor

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

If you want to see the CI pass, you could steal the changes from #2301

const opentelemetry::sdk::instrumentationscope::InstrumentationScope *scope_;

Basically make this

 const opentelemetry::sdk::instrumentationscope::InstrumentationScope *scope_ = nullptr;

exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
@dashpole dashpole force-pushed the prometheus_scope_labels branch 3 times, most recently from fbfc0c4 to 49946cb Compare September 21, 2023 15:59
Copy link
Contributor

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

I tried for an hour to get this to work. I am giving up. Here are some code nits for you:

exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved
@punya

This comment was marked as resolved.

@dashpole dashpole marked this pull request as ready for review September 22, 2023 14:25
@dashpole dashpole requested a review from a team September 22, 2023 14:25
@dashpole dashpole force-pushed the prometheus_scope_labels branch 3 times, most recently from c178713 to 850dfc4 Compare September 26, 2023 00:54
@marcalff
Copy link
Member

Please merge with a recent main, to resolve the conflicts.

@dashpole
Copy link
Contributor Author

dashpole commented Oct 2, 2023

@marcalff done

@lalitb
Copy link
Member

lalitb commented Oct 2, 2023

Depending on which PR is merged first - #2293 or #2301, the other PR will need to address merge conflicts. One argument could be to merge smaller and less complex PR (this?) first, so easy to resolve conflicts in other one.

@dashpole
Copy link
Contributor Author

dashpole commented Oct 2, 2023

I'm happy to wait and rebase my PR.

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.

LGTM, thanks for the fix.

@marcalff marcalff merged commit 0803e6a into open-telemetry:main Oct 2, 2023
45 checks passed
@dashpole dashpole deleted the prometheus_scope_labels branch October 2, 2023 18:21
@marcalff marcalff changed the title prometheus exporter: Add otel_scope_name and otel_scope_version labels [EXPORTER] prometheus: add otel_scope_name and otel_scope_version labels Oct 11, 2023
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.

5 participants