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

Sanitize metric names in the prometheus exporter #3212

Merged
merged 11 commits into from Sep 22, 2022
Merged

Sanitize metric names in the prometheus exporter #3212

merged 11 commits into from Sep 22, 2022

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Sep 21, 2022

Prometheus has different metric restrictions than the specification provides us.
All prometheus metrics must match this regex: https://github.com/prometheus/common/blob/main/model/metric.go#L27

While the specification will use dots in metric names, and therefore cause invalid metric names in the exporter.
See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/process-metrics.md

With this change, libraries can use metric names valid with semantic conventions, for which any invalid character will be replaced with an underscore.

Related: open-telemetry/opentelemetry-go-contrib#2787

Fixes #3183

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #3212 (2822387) into main (2ae5922) will increase coverage by 0.0%.
The diff coverage is 97.5%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3212   +/-   ##
=====================================
  Coverage   77.3%   77.3%           
=====================================
  Files        159     159           
  Lines      11101   11139   +38     
=====================================
+ Hits        8583    8617   +34     
- Misses      2323    2325    +2     
- Partials     195     197    +2     
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 78.5% <97.5%> (+1.6%) ⬆️
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+0.8%) ⬆️

@MrAlias MrAlias added this to the Metric v0.32.1 milestone Sep 21, 2022
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

I think this is a good stop gap, but it will iterate through all names on every collect.

We should revisit this and see if there is a way to optimize it so that there is approximately constant time sanitizing the names.

exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter_test.go Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

🥇

@MrAlias MrAlias merged commit bfaff3a into open-telemetry:main Sep 22, 2022
@dmathieu dmathieu deleted the prometheus-sanitize-names branch September 22, 2022 14:25
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.

Sanitize OpenTelemetry metric instrument names for Prometheus
6 participants