Include resource attributes as tags for Prometheus exporters

[robertcoltheart]

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

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

[codecov]

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%)	⬇️
...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%)	⬆️
...s.HttpListener/Internal/PrometheusSerializerExt.cs	100.00% <100.00%> (ø)
...theusHttpListenerMeterProviderBuilderExtensions.cs	81.81% <100.00%> (-11.04%)	⬇️
...heus.HttpListener/PrometheusHttpListenerOptions.cs	100.00% <100.00%> (ø)
...ometheus.AspNetCore/PrometheusAspNetCoreOptions.cs	50.00% <50.00%> (-25.00%)	⬇️

... and 65 files with indirect coverage changes

[CodeBlanch]

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

[robertcoltheart]

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.

[CodeBlanch]

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

[robertcoltheart]

@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).

[CodeBlanch]

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

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

[robertcoltheart]

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.

[alanwest]

This is an optional feature per the Prometheus exporter spec.

A Prometheus Exporter MAY offer configuration to add resource attributes as metric attributes.

I'd personally be ok moving forward with this change simply to maintain feature parity with other key languages that have implemented it (Java, Go, maybe others). It is part of the spec and thank you @robertcoltheart for the time you've spent on this.

That said, we discussed again in today's meeting, and the opinion of other maintainers is to only implement this if there is user demand, so our plan is to not accept this PR at this time. I will open an issue shortly where folks can express their interest in this feature.

I'm leaving some comments with my thoughts just in case we come back to this in the future.

[alanwest]

I understand this implementation was inspired from Java's. It appears both Java and Go have implemented this as a predicate. That said, the spec is not opinionated in how this is implemented:

The configuration SHOULD allow the user to select which resource attributes to copy (e.g. include / exclude or regular expression based).

I think I'd prefer a simple include list. Something like:

/// <summary>
/// Gets or sets the resource attributes to include on each metric point. By default no resource attributes are included.
/// </summary>
public string[] IncludedResourceAttributes

[alanwest]

If we did keep this as a predicate, we could avoid evaluating the predicate on every single export. The configured resource is immutable. It does not change once the exporter is instantiated.

[github-actions]

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]

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.

[vishweshbankwar]

Closing. See #5489 (review) for details