Define how to enable Meter attributes for Prometheus Exporter

[hdost]

Fixes #1906

Changes

• Add OTEL_METRICS_LIBRARY_ATTRIBUTES variable
• Update Metric SDK to clarify when Meter attributes should be exported.

Related issues #1953

[reyang]

Related to #2040.

[reyang]

https://github.com/open-telemetry/opentelemetry-specification/pull/2035/files#r732254987

[hdost]

So I'm not sure if your pr is necessarily opposed to my intent at least.

What i would ask is whether Exporters MUST or MAY export the labels. My concern is cardinality. I fully agree that it changes the metrics but I believe the SDK user should have that control.

[jmacd]

An OTLP exporter doesn't need these options because the protocol includes these fields explicitly. A non-OTLP exporter could use these options, but I wonder if they can be configured via Views. There are already semantic conventions for naming these fields when projecting into non-OTLP systems (from the tracing spec), so I imagine a special case in the View machinery to recognize special cases for instrumentation library name/version, then synthesizing those attributes.

[hdost]

An OTLP exporter doesn't need these options because the protocol includes these fields explicitly. A non-OTLP exporter could use these options, but I wonder if they can be configured via Views. There are already semantic conventions for naming these fields when projecting into non-OTLP systems (from the tracing spec), so I imagine a special case in the View machinery to recognize special cases for instrumentation library name/version, then synthesizing those attributes.

I think that makes sense, I'm just trying to envisage how that might work. Potentially a provider to the view.

Also during the meeting you mentioned something related to how we might use views to add the attributes exported via resources. However, I don't believe that with https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#resource-sdk we would require any additional languages.

... all metrics produced by any Meter from the provider will be associated with this Resource
The way I read that is the Resources should automatically have these attributes.
(Maybe best expanded into a different issue, already exists?)
It seems to me the views should if anything be used to strip away the Attributes.
However, I'm not 100% sure that is even necessary for any special conditions or mechanisms need exists.

[hdost]

this could change the resulting time series, which might be expected or unexpected - depending on the intention
name and version are very generic names that could introduce conflict, we don't have spec about conflict resolution

@reyang I believe based on the comments @jsuereth and the updates I have made, this should be a bit closer to something useful.

[reyang]

LGTM.

[hdost]

I think at this point it just needs approval from @jmacd and/or some other owner.

[jsuereth]

I'm leaving this as a non-blocking comment, but one I that is important (and would make blocking if that didn't restrict merging so much).

TL;DR; I think we need to do a bit better around Prometheus Attributes. I don't want to stand in the way of progress, but I'd like to see a prototype and some usage before specification, because I think this is actually a very tricky thing to get right. Specifically I don't think this PR goes far enough.

• This PR doesn't account for Resource attributes and when those should make it to prometheus, It only accounts for those coming from Meter. I think both are important, and if you look at Initial draft of Prometheus <-> OTLP datamodel specification. #2017 You'll see some details there.
• I believe mapping instrumentation-library-name to labels should be done by default in prometheus exporter (as is the case for trace). Additionally I think it's more important for fine-grained code-based configuration with coarse-grained env variables.
• There should be a consistent configuration used for mapping Resource + InstrumentationLibrary attributes to prometheus exported metrics. I think "all" or "nothing" is not quite right here, but is a decent start.

To make progress:

1. @hdost Would you be willing to put together a prototype or more example use case here around what attributes across all of OTLP should be in prometheus? I'm happy to help, just overloaded in time at the moment trying to build the otel roadmap.
2. I think we should make sure the prometheus-wg has a chance to chime in on this PR. CC @alolita to make sure we have some review from that group.
3. I'd make a counter-proposal with the following:

• A single environment variable that takes a set of attributes to map into prometheus labels. If it's empty, all are mapped by default.
• A set of conventional names for instrumentation library otel.library.version and otel.library.name are what we used for tracing. These would implicitly be used to denote library should be mapped into promtheus label.
  Additionally We actually think about using this convention for non-prometheus backends. Specifically, I discovered these are missing in the Google Cloud Monitoring exporter, and it actually leads to duplicate point errors because the instrumentation library identity is actually a part of the metric identity and should be exported by default for correct behavior.

[hdost]

I'm leaving this as a non-blocking comment, but one I that is important (and would make blocking if that didn't restrict merging so much).

TL;DR; I think we need to do a bit better around Prometheus Attributes. I don't want to stand in the way of progress, but I'd like to see a prototype and some usage before specification, because I think this is actually a very tricky thing to get right. Specifically I don't think this PR goes far enough.

* This PR doesn't account for `Resource` attributes and when those should make it to prometheus,  It only accounts for those coming from `Meter`.  I think both are important, and if you look at [Initial draft of Prometheus <-> OTLP datamodel specification. #2017](https://github.com/open-telemetry/opentelemetry-specification/pull/2017) You'll see some details there.

* I believe mapping instrumentation-library-name to labels should be done by default in prometheus exporter (as is the case for trace).   Additionally I think it's more important for fine-grained code-based configuration with coarse-grained env variables.

* There should be a _consistent_ configuration used for mapping Resource + InstrumentationLibrary attributes to prometheus exported metrics.   I think "all" or "nothing" is not quite right here, but is a decent start.

To make progress:

1. @hdost Would you be willing to put together a prototype or more example use case here around what attributes across all of OTLP should be in prometheus?  I'm happy to help, just overloaded in time at the moment trying to build the otel roadmap.

I think I can arrange time to do that. What is the timeframe we are looking so I can prioritize?

2. I think we should make sure the prometheus-wg has a chance to chime in on this PR.  CC @alolita to make sure we have some review from that group.

That makes perfect sense.

3. I'd make a counter-proposal with the following:


* A single environment variable that takes a set of attributes to map into prometheus labels.  If it's empty, _all_ are mapped by default.

* A set of conventional names for instrumentation library `otel.library.version` and `otel.library.name` are what we used for tracing.   These would implicitly be used to denote library should be mapped into promtheus label.
  Additionally We actually think about using this convention for non-prometheus backends.  Specifically, I discovered these are missing in the Google Cloud Monitoring exporter, and it actually leads to duplicate point errors because the instrumentation library identity is _actually_ a part of the metric identity and _should_ be exported by default for correct behavior.

I believe we did talk about something like this, and I would agree that I think makes a lot of sense. Potentially an addendum.
KEEP_LIST and DROP_LIST

KEEP_LIST	DROP_LIST	Result
Empty	Empty	ALL attributes
Populated	Empty	Set intersection ALL & KEEP_LIST
Populated	Populated	Set difference KEEP_LIST - DROP_LIST
Empty	Populated	Set difference ALL - DROP_LIST

[jsuereth]

1. @hdost Would you be willing to put together a prototype or more example use case here around what attributes across all of OTLP should be in prometheus? I'm happy to help, just overloaded in time at the moment trying to build the otel roadmap.

I think I can arrange time to do that. What is the timeframe we are looking so I can prioritize?

I'd like to see a prototype/usage example in prometheus prior to merging the specification change, if that's feasible.

[hdost]

1. @hdost Would you be willing to put together a prototype or more example use case here around what attributes across all of OTLP should be in prometheus? I'm happy to help, just overloaded in time at the moment trying to build the otel roadmap.

I think I can arrange time to do that. What is the timeframe we are looking so I can prioritize?

I'd like to see a prototype/usage example in prometheus prior to merging the specification change, if that's feasible.

@jsuereth Yup, I was more wondering in the context of timeline, is the goal to have this behavior defined before Feature freeze of he SDK or just stabilization.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[jsuereth]

@hdost IIUC - Prometheus Exporter specification can happen post SDK-freeze unless you need a feature from the SDK for this.

It's my understanding that we do not need SDK features for this, so we're free to take some time to get this right. I'd like to make sure whatever you do here lines up with the formal specification for prometheus.

I think the only thing I'm objecting too on this PR right now is that I think adding otel.library (or whatever the label is we'll use for insturmentation library) should be default on for prometheus export.

[jmacd]

We discussed this at length in the Prometheus working group SIG and consulted with @brian-brazil.

There is a fundamental question which is practically orthogonal to the issue at hand, which is when can we ever say that two metrics are the "same". This question borders on philosophical--we discussed a hypothetical set of two libraries both with a "request_count" metric. Brian's experience says that rarely are two implementations exactly the same, so they should be considered different almost always. While cpu.seconds may be given a single meaning, very often "request_count" differs slightly based on where in a pipeline you are counting requests, so a more-refined metric name should be used. E.g., if you were counting Kafka requests and Redis requests, you should probably use different metric names.

Here are the recommendations we arrived at when we landed this discussion:

• Inside the same binary,
  • two instruments with the same name SHOULD use the same instrument kind, SDK MUST warn the user and reject one
  • two instruments with the same name SHOULD use distinct attribute sets, because the collector can't validate
• Inside a collector, when exporting to Prometheus:
• do nothing by default; if the application follows the rules, nothing bad happens except possibly combining metrics w/ slightly different implementations
• optionally, if the user wants to consistently apply an instrumentation-library attribute, this should be an option, but it's not a good default behavior because it has to be done consistently across a system
• General recommendations
  • Prefix the library name to the metric name if two libraries might use the same attribute set inside a single binary
  • Follow distinct resource practices to avoid breaking the single-writer rule, so ultimately this issue boils down to another case of How do I know which resource attributes are important enough to identify a Prometheus Metrics Stream? #1782

[hdost]

Prefix the library name to the metric name if two libraries might use the same attribute set inside a single binary

Oh so now that is quite interesting 🤔 basically this would namespace things, so if you're instrumenting
"io.otel.metrics" and "io.otel.traces" then the metrics could optionally be appended with those names sanitized for prometheus as an example:io_otel_traces_<metric_name>

I could see this exploding the metric name length depending on what comes through.

Edit: i saw from the meeting notes "Solution: Do not prefix else always prefix."

[reyang]

I did some quick prototype in .NET open-telemetry/opentelemetry-dotnet#2593.

Couple findings:

1. the label name name and version are concerning, I think it is very likely to conflict with existing labels (it is totally possible to invent some rule although I didn't cover it in the prototype).
2. the Metrics API Spec guarantees a non-empty meter name, however there is no guarantee on the meter version. I think we should call out here what's the behavior if version is not available or is an empty string.
3. I think there are many cases people might care about the meter name, but not the version, I think the current proposal (having both or nothing) might be less useful.

[reyang]

Please find addition feedback based on the prototype https://github.com/open-telemetry/opentelemetry-specification/pull/2035/files#r747236300.

[reyang]

I guess it meant to be "Prometheus labels" instead of "Attributes"?

[jonatan-ivanov]

Since some backends has reserved words for tag name, I would try to choose something more specific than name and version, like meter_name and meter_version. Also, if I see name and version on the tags, I'm not sure it will be clear to me what these exactly mean.

(Not really Prometheus-related but datadog reserves version)

[jmacd]

As discussed in the SIG call today, we should be using the same attribute names already specified for tracing systems for consistency. This means "otel.library.name" and "otel.library.version", which in a Prometheus setting should have dots converted to underscores.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.