opentelemetry-dotnet
opentelemetry-dotnet copied to clipboard
Include resource attributes as tags for Prometheus exporters
Fixes #3087 Design discussion issue #
See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1
Changes
This is an extension of this pull request which adds target_info
to the output of Prometheus exporters. This PR adds the default resource attributes as tags to each metric. By default, resource attributes are not included, but can be opted in by using the optional filter. This mirrors the Java code functionality.
Note that OpenMetrics support is ignored here, since tags/labels are supported in non-OpenMetrics scrapers too.
Merge requirement checklist
- [x] CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
- [x] Unit tests added/updated
- [x] Appropriate
CHANGELOG.md
files updated for non-trivial changes - [ ] Changes in public API reviewed (if applicable)
Codecov Report
Attention: Patch coverage is 95.83333%
with 1 lines
in your changes are missing coverage. Please review.
Project coverage is 85.30%. Comparing base (
6250307
) to head (61c0b4d
). Report is 204 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #5489 +/- ##
==========================================
+ Coverage 83.38% 85.30% +1.92%
==========================================
Files 297 290 -7
Lines 12531 12611 +80
==========================================
+ Hits 10449 10758 +309
+ Misses 2082 1853 -229
Flag | Coverage Δ | |
---|---|---|
unittests | ? |
|
unittests-Solution-Experimental | 85.30% <95.83%> (?) |
|
unittests-Solution-Stable | 85.27% <95.83%> (?) |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files | Coverage Δ | |
---|---|---|
...tpListener/Internal/PrometheusCollectionManager.cs | 76.74% <100.00%> (-1.04%) |
:arrow_down: |
...etheus.HttpListener/Internal/PrometheusExporter.cs | 100.00% <100.00%> (ø) |
|
...HttpListener/Internal/PrometheusExporterOptions.cs | 100.00% <100.00%> (ø) |
|
...stener/Internal/PrometheusResourceTagCollection.cs | 100.00% <100.00%> (ø) |
|
...heus.HttpListener/Internal/PrometheusSerializer.cs | 86.04% <100.00%> (+1.17%) |
:arrow_up: |
...s.HttpListener/Internal/PrometheusSerializerExt.cs | 100.00% <100.00%> (ø) |
|
...theusHttpListenerMeterProviderBuilderExtensions.cs | 81.81% <100.00%> (-11.04%) |
:arrow_down: |
...heus.HttpListener/PrometheusHttpListenerOptions.cs | 100.00% <100.00%> (ø) |
|
...ometheus.AspNetCore/PrometheusAspNetCoreOptions.cs | 50.00% <50.00%> (-25.00%) |
:arrow_down: |
@robertcoltheart Why would users want to do this instead of using the target_info
thing?
I mean, I agree with you. However it is part of the spec (link above) that this feature be supported. The Java version has this ability too. There is one use case I can think of which is for non-OpenMetrics scrapers, since the 'info' type (ie target_info) is only supported in OpenMetrics format.
@robertcoltheart This bit from the spec?
The Resource attributes MAY be copied to labels of exported metric families if required by the exporter configuration, or MUST be dropped.
The "MAY" part seems like an optional thing.
There is one use case I can think of which is for non-OpenMetrics scrapers, since the 'info' type (ie target_info) is only supported in OpenMetrics format.
Things like exemplars are only supported in OpenMetrics format right?
What I'm trying to gauge is if this is a short-lived thing we shouldn't bother supporting (forever) as everything in the prometheus world seems to be moving to OpenMetrics format.
@CodeBlanch Yep agree with everything you said. I'll leave it to a maintainer to decide on what to do with this, I'm not heavily invested in it either way, and the otel_collector solves issues like adding common labels to metrics anyway (for example deployment_environment
).
@robertcoltheart I added this to the agenda for our next SIG meeting so we can discuss if this is something we should do or not. Feel free to join! https://docs.google.com/document/d/1yjjD6aBcLxlRazYrawukDgrhZMObwHARJbB9glWdHj8/edit
@robertcoltheart We just discussed this a bit on our SIG meeting. The outcome was more or less no one really has expertise enough in Prometheus to make a call. @alanwest volunteered to look into it and our plan is to make a decision next week. Sorry about the delay!
No worries thanks for the update. As above, I don't have any skin in this game and only raised the PR as a way of fulfilling the spec as laid out, and to mirror the functionality of the Java version.
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.
Closing. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/5489#pullrequestreview-2004593753 for details