opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

Include resource attributes as tags for Prometheus exporters

Open robertcoltheart opened this issue 10 months ago • 9 comments

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)

robertcoltheart avatar Mar 29 '24 09:03 robertcoltheart

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

Impacted file tree graph

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

... and 65 files with indirect coverage changes

codecov[bot] avatar Mar 29 '24 09:03 codecov[bot]

@robertcoltheart Why would users want to do this instead of using the target_info thing?

CodeBlanch avatar Mar 29 '24 17:03 CodeBlanch

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 avatar Mar 29 '24 21:03 robertcoltheart

@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 avatar Mar 29 '24 22:03 CodeBlanch

@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 avatar Mar 29 '24 22:03 robertcoltheart

@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

CodeBlanch avatar Apr 04 '24 17:04 CodeBlanch

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

CodeBlanch avatar Apr 09 '24 23:04 CodeBlanch

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.

robertcoltheart avatar Apr 09 '24 23:04 robertcoltheart

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.

github-actions[bot] avatar Apr 24 '24 03:04 github-actions[bot]

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.

github-actions[bot] avatar May 02 '24 03:05 github-actions[bot]

Closing. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/5489#pullrequestreview-2004593753 for details

vishweshbankwar avatar May 02 '24 18:05 vishweshbankwar