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] Fix OpenMetrics format suffixes #5646

Merged

Conversation

robertcoltheart
Copy link
Contributor

Fixes #5502

Changes

According to the OpenMetrics specs, unit must be a suffix of the metric name, and not just infix in the name. This PR also complies with the requirement that counters MUST end with _total, although this clashes with the OTEL spec that says it should be optional. Additionally, UNIT, HELP and TYPE metadata should not include the _total suffix, as seen in examples in the OpenMetrics specs.

Tested with the latest version of Prometheus.

See:

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@robertcoltheart robertcoltheart requested a review from a team May 22, 2024 20:09
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.12%. Comparing base (6250307) to head (2fc263d).
Report is 243 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #5646       +/-   ##
===========================================
- Coverage   83.38%   20.12%   -63.27%     
===========================================
  Files         297      185      -112     
  Lines       12531     7917     -4614     
===========================================
- Hits        10449     1593     -8856     
- Misses       2082     6324     +4242     
Flag Coverage Δ
unittests ?
unittests-UnstableCoreLibraries-Experimental 20.12% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ometheus.HttpListener/Internal/PrometheusMetric.cs 74.12% <100.00%> (+2.37%) ⬆️
...heus.HttpListener/Internal/PrometheusSerializer.cs 85.71% <100.00%> (+0.84%) ⬆️
...s.HttpListener/Internal/PrometheusSerializerExt.cs 100.00% <100.00%> (ø)

... and 242 files with indirect coverage changes

@cijothomas
Copy link
Member

@robertcoltheart Thanks a lot for taking good care of Prometheus exporters!

@CodeBlanch
Copy link
Member

@robertcoltheart I'm happy to merge this because there are approvals but there is also some open feedback from @dashpole. Are you going to respond or make more changes or should I merge?

@CodeBlanch CodeBlanch added pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package labels May 23, 2024
@CodeBlanch CodeBlanch changed the title Fix OpenMetrics format suffixes for Prometheus [prometheus] Fix OpenMetrics format suffixes May 23, 2024
@robertcoltheart
Copy link
Contributor Author

Let me make the suggested changes, and then we can review again

@CodeBlanch CodeBlanch merged commit b9d56aa into open-telemetry:main May 28, 2024
38 checks passed
@robertcoltheart robertcoltheart deleted the bug/fix-openmetrics-suffixes branch May 28, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unit "bytes" not a suffix of metric "process_runtime_dotnet_gc_allocations_size_bytes_total"
7 participants