Skip to content

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