-
Notifications
You must be signed in to change notification settings - Fork 772
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
Add scope info and version to Prometheus exporter #5086
Add scope info and version to Prometheus exporter #5086
Conversation
|
@robertcoltheart Thanks for sending out this PR! Could you fix the build errors? |
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusCollectionManager.cs
Outdated
Show resolved
Hide resolved
Fixed build errors, I think, can you re-run? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5086 +/- ##
==========================================
+ Coverage 83.26% 83.45% +0.18%
==========================================
Files 297 297
Lines 12366 12386 +20
==========================================
+ Hits 10297 10337 +40
+ Misses 2069 2049 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@robertcoltheart Could you run this example with a PrometheusExporter and share the scraped output? We would like to get an idea of how the serialized data looks like with these changes. |
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusSerializer.cs
Outdated
Show resolved
Hide resolved
Updated in the PR description from the example ASPNET app. The output is shortened a bit for brevity but shows histograms and counters with and without meter versions, and their respective |
…bertcoltheart/opentelemetry-dotnet into feature/prometheus-scope-info
can you use this example? https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/docs/metrics/getting-started-prometheus-grafana/Program.cs and share the output with and without the feature? |
Here is the before and after using the sample above: Before:
After:
|
I took a lot of the changes in this PR from the Java version of OTEL. An example output of the Java metrics using the above format is below:
|
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusExporter.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusSerializerExt.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Prometheus.AspNetCore/.publicApi/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
Could you make this example in the PR description to keep it easy to follow? |
src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusCollectionManager.cs
Outdated
Show resolved
Hide resolved
…bertcoltheart/opentelemetry-dotnet into feature/prometheus-scope-info
@@ -3,6 +3,7 @@ | |||
## Unreleased | |||
|
|||
* Export OpenMetrics format from Prometheus exporters ([#5107](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5107)) | |||
* For requests with OpenMetrics format, scope info is automatically added ([#5086](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5086)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertcoltheart Scope info is added regardless of whether OpenMetrics is added, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otel_scope_name
and otel_scope_version
tags are added regardless of what is requested, yes. This mirrors the behavior of the Java OTEL library. The otel_scope_info
is only added when OpenMetrics is requested, otherwise Prometheus doesn't know how to handle info
types and will fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The otel_scope_info is only added when OpenMetrics is requested
The example that you provided in the PR description has otel_scope_info
added for both the cases though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, I don't know how I missed that. Sorry about that, let me raise a PR for that right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #5182
Hi guys, It’s cool, of course, that you added this a feature, but looks like that manuals describes the ability for the exporter to disable this feature
|
Fixes #3972
Changes
Add instrument scope info and version to Prometheus exporter as per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#instrumentation-scope-1
Scope info is automatically added to the Prometheus exporters.
Before this change:
After (with no
Accept
header):After (when passing
Accept: application/openmetrics-text
:Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes