Prometheus exporter: Stabilize Resource Constant Labels#5130
Prometheus exporter: Stabilize Resource Constant Labels#5130ArthurSens wants to merge 2 commits into
Conversation
721a78b to
a750021
Compare
|
| A Prometheus Exporter MAY offer configuration to add resource attributes as metric labels. | ||
| By default, it MUST NOT add any resource attributes as metric labels. | ||
| The configuration SHOULD allow the user to select resource attributes to include or exclude. Copied Resource attributes MUST NOT be | ||
| excluded from the `target_info` metric. The option SHOULD be named `resource_constant_labels`. |
There was a problem hiding this comment.
the spec says the option should let users "include or exclude" resource attributes, but the name resource_constant_labels reads as an include-only list. And most languages already diverge...
The name doesn't fit the described behavior? Should we refine the name, or narrow the spec text to include-only before stabilizing or drop the SHOULD requirement about name completely from spec?
There was a problem hiding this comment.
I'm failing to see how resource_constant_labels suggests an include-only list 🤔. Maybe the previous alternative (with_resource_constant_labels) could suggest something like this, but we got rid of it in #5084
There was a problem hiding this comment.
My bad- I meant to say the name resource_constant_labels does not convey if its include list or exclude list. And each language handles this in their own way. I think spec should avoid the SHOULD name the option as something wording, as the current word is not enough to convey if this is include/exlude list.
There was a problem hiding this comment.
And each language handles this in their own way
Check out how this is defined in declarative config: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema-docs.md#experimentalprometheusmetricexporter-
Its an IncludeExclude type, which support arrays of glob pattern include / exclude. I think this works. Its sufficiently expressive, and this IncludeExclude glob pattern array pattern is used in other places in declarative config so it becomes a pattern users get familiar / comfortable with.
The spec could get really detailed and describe the exact semantics of declarative config, but then we end up writing the same thing in two places and having to keep them in sync and decide when they're out of sync enough to do something about. Given that the declarative config schema is an extension of the spec, and that we now have guidance to evolve spec related to config in lockstep with declarative config, I think practically speaking maintainers should look at both the spec and corresponding declarative config when defining their config APIs.
IMO, it fine for the spec to be more vague and defer to declarative config to do the heavy lifting of hashing out details.
There was a problem hiding this comment.
Interesting, Prometheus uses a string array that works as an exact match, not a glob pattern. And that's what I was aiming for in open-telemetry/opentelemetry-collector-contrib#48922
I'm not sure if we want to be explicit about the include/exclude strategy, but if we do then we might need to reword the text a bit
There was a problem hiding this comment.
Agree, we should let declarative config hashout details. Perhaps the easiest fix is to remove the normative "SHOULD be named" from spec wording completely. Spec states - this functionality must be offered. period.
IOW, "The option SHOULD be named resource_constant_labels." - this statement can be removed IMHO.
There was a problem hiding this comment.
Well, I kinda like the idea of having consistency across all SDKs. Every config option adds to the cognitive load, so I believe it's easier for the end user if they don't need to learn all the different config options across different language SDKs that might or might not do the same thing.
That said, I'm not toooooo strongly opinionated here; if the majority of the group doesn't care about this, then I'm happy to drop it as well.
What are your thoughts @jack-berg, @open-telemetry/prometheus-interoperability?
There was a problem hiding this comment.
so I believe it's easier for the end user if they don't need to learn all the different config options across different language SDKs that might or might not do the same thing.
That's what declarative config does. Its the language agnostic configuration interface. Semantics of all types and properties are laid out explicitly, often in more detail than the spec.
I think its fine as is, but would also think it were fine if the spec was more explicit like declarative config is. 🤷
There was a problem hiding this comment.
I think named parameters in the spec is usually a good thing. I'm open to other names if we don't like the current one. I'm happy with the current state
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
a750021 to
2c30c5a
Compare
|
This PR was marked stale. It will be closed in 14 days without additional activity. |
6 participants
Fixes #4987
Changes
Stabilize the configuration option for transforming OTLP Resource Attributes into Prometheus Labels.
The configuration option is currently available in the following SDKs:
Weird ones:
resource_to_telemetry_conversion, which has an "all or nothing" behavior; it doesn't allow the user to pick which ones to transform.resource_to_telemetry_conversionwith "all or nothing" behavior.All other implementations do not provide this option yet
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check