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

Support Prometheus UNIT metadata #3651

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 13, 2022

Changes

  • Added support for UNIT metadata.
  • Renamed some of the internal methods to better align with OpenMetrics terms (e.g. "metadata").
  • Adjusted the ordering of the metadata to follow the OpenMetrics best practices.

Reference: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

image

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@reyang reyang requested a review from a team September 13, 2022 16:56
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #3651 (3d1fb73) into main (de98eb7) will increase coverage by 0.02%.
The diff coverage is 94.73%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3651      +/-   ##
==========================================
+ Coverage   87.58%   87.60%   +0.02%     
==========================================
  Files         283      283              
  Lines       10218    10234      +16     
==========================================
+ Hits         8949     8966      +17     
+ Misses       1269     1268       -1     
Impacted Files Coverage Δ
...heus.HttpListener/Internal/PrometheusSerializer.cs 80.34% <93.75%> (+2.12%) ⬆️
...s.HttpListener/Internal/PrometheusSerializerExt.cs 100.00% <100.00%> (ø)
...tpListener/Internal/PrometheusCollectionManager.cs 78.04% <0.00%> (-2.44%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+8.82%) ⬆️

@reyang reyang added pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package enhancement New feature or request labels Sep 13, 2022
.AddInMemoryExporter(metrics)
.Build();

meter.CreateObservableGauge("test_gauge", () => 123, unit: "seconds", description: "Hello, world!");
Copy link
Member

Choose a reason for hiding this comment

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

nice description :)

Copy link
Member

Choose a reason for hiding this comment

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

Needs some clever Greek mythology thing. Defer to @alanwest to come up with it 🤣

Copy link
Member

Choose a reason for hiding this comment

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

I mean, would "Hello, Prometheus!" not have been clever enough? And also what the heck do I know about Greek mythology?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we consider the Prometheus Exporters to be renamed to OpenMetrics Exporters? 😆

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

@CodeBlanch CodeBlanch merged commit 6ea078e into open-telemetry:main Sep 13, 2022
@reyang reyang deleted the reyang/prometheus-unit branch September 13, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants