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

Update Prometheus exporter name and unit processing #4753

Merged

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Aug 8, 2023

Fixes #4742
Design discussion issue #

Changes

Updates the .NET Prometheus exporter to follow the metric and unit naming rules.

See https://github.com/open-telemetry/opentelemetry-specification/blob/b2f923fb1650dde1f061507908b834035506a796/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus

Merge requirement checklist

  • CONTRIBUTING guidelines followed (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)

@JamesNK JamesNK requested a review from a team August 8, 2023 12:39
@JamesNK
Copy link
Contributor Author

JamesNK commented Aug 8, 2023

cc @noahfalk @lmolkova

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #4753 (d51688e) into main (031ed48) will increase coverage by 0.27%.
The diff coverage is 99.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4753      +/-   ##
==========================================
+ Coverage   82.12%   82.40%   +0.27%     
==========================================
  Files         313      314       +1     
  Lines       12783    12910     +127     
==========================================
+ Hits        10498    10638     +140     
+ Misses       2285     2272      -13     
Files Changed Coverage Δ
...ometheus.HttpListener/Internal/PrometheusMetric.cs 99.23% <99.23%> (ø)
...tpListener/Internal/PrometheusCollectionManager.cs 77.77% <100.00%> (+1.95%) ⬆️
...heus.HttpListener/Internal/PrometheusSerializer.cs 81.25% <100.00%> (+0.41%) ⬆️
...s.HttpListener/Internal/PrometheusSerializerExt.cs 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

build/Common.props Outdated Show resolved Hide resolved
JamesNK and others added 3 commits August 10, 2023 10:15
…ometheusMetric.cs

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
@JamesNK
Copy link
Contributor Author

JamesNK commented Aug 11, 2023

There are some failing tests. Are they broken by this PR or flaky tests?

@utpilla
Copy link
Contributor

utpilla commented Aug 11, 2023

There are some failing tests. Are they broken by this PR or flaky tests?

Not related to your PR. That's a flaky test.

Tracking issue #4763

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM with a CHANGELOG update

@JamesNK
Copy link
Contributor Author

JamesNK commented Aug 15, 2023

I'd like to get this in for the next OTel release. Also, I'd like to be able to use these changes right now from the nightly feed.

What work remains before this can be merged?

@CodeBlanch
Copy link
Member

@JamesNK I'm happy to merge but I do think this is a substantial change we should mention in the logs:

Something like this should work...

## Unreleased

* Added support for unit and name conversion following the [OpenTelemetry Specification](https://github.com/open-telemetry/opentelemetry-specification/blob/065b25024549120800da7cda6ccd9717658ff0df/specification/compatibility/prometheus_and_openmetrics.md?plain=1#L235-L240)
  ([#4753](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4753))

@JamesNK
Copy link
Contributor Author

JamesNK commented Aug 16, 2023

Done.

@CodeBlanch CodeBlanch merged commit 49031ba into open-telemetry:main Aug 16, 2023
44 checks passed
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.

Prometheus metric names incorrectly append unit
5 participants