Prometheus exporter: Stabilize config option scope_info_enabled#5056
Conversation
dashpole
left a comment
There was a problem hiding this comment.
the _info suffix is a relic of the otel_scope_info metric we used to have. But it doesn't seem worth it to change it now.
There was a problem hiding this comment.
Hey I want us to consider changing the names of a few of these prometheus exporter property names before stabilizing.
I think the whole with_ prefix pattern is weird. Not sure where that came about but we don't see that anywhere else in SDK component config properties.
I think the use of "without" without_scope_info / without_target_info with a default value of false was probably inspired by the "All Boolean environment variables SHOULD be named and defined such that false is the expected safe default behavior." language that we decided is not applicable to other configuration interfaces: #4723
So since there's no env vars for without_scope_info / without_target_info, we don't need to be constrained by that.
I propose these names, which I find more consistent with property naming elsewhere in the project and more intuitive:
without_scope_info->scope_info_enabled(default true)without_target_info->target_info_enabled(default true)with_resource_constant_labels->resource_constant_labels(default none are added)
|
True. I think the pattern probably came from Go. The prefix should be removed. |
I'll open a PR to adjust the parameter names today |
|
I don't have any problems with changing the configuration option. Most existing implementations are already using the "without" wording, though. Including the Collector, which has a HUGE user base. I totally understand that the spec says MAY, so implementors don't need to follow the wording to the letter, but it feels inconsistent to me that the spec suggests one thing and most implementors use another. |
|
A change in the spec property name doesn't stop an implementation from using the function options with pattern. See how opentelemetry-go OTLP exporters use I see that the collector prometheus exporter does include a |
|
Yeah, no strong opinions here. If the group prefers rewording the config, I'll be happy to do it; I just wanted to reinforce that current implementors are all using different names. |
Here, I change: * without_scope_info -> scope_info_enabled (default true) * without_target_info -> target_info_enabled (default true) * with_resource_constant_labels -> resource_constant_labels (default none are added) The `with` prefix of a few (but not all) prometheus exporter options seems to be a carry over from go functional options pattern. We don't see it in the spec'd property names of any other built in components. It doesn't doesn't contribute to the property's meaning in any way. Omitting it doesn't block a language from using it in their implementation, since that choice falls within maintainer discretion. I think the use of "without" without_scope_info / without_target_info with a default value of false was probably inspired by the "All Boolean environment variables SHOULD be named and defined such that false is the expected safe default behavior." language that we decided is not applicable to other configuration interfaces: open-telemetry#4723 So since there's no env vars for without_scope_info / without_target_info, we don't need to be constrained by that. Originated from: open-telemetry#5056 (comment) Corresponding change in declarative config schema: github.com/open-telemetry/opentelemetry-configuration/pull/612
without_scope_infoscope_info_enabled
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
e3ce19f to
7708f57
Compare
cijothomas
left a comment
There was a problem hiding this comment.
Checking if we have met usual requirement of 2-3 sdk implementations before stabilizing?
If needed, I can help get OTel Rust to implement this, if that helps. (Or if this was already covered, no issues!)
We have, so far, 1) Collector's Prometheus exporter, 2) Collector's Prometheus Remote Write exporter, 3) OTel Go SDK, 4) OTel Java SDK. |
Thanks for confirming. The new names are not yet reflected in those implementations, but we haven't released spec with that changes yet! |
cijothomas
left a comment
There was a problem hiding this comment.
LGTM.
A minor non-blocker - the recent rename of the option is not yet implemented in the languages(in flight in some). It won't hurt to wait till that happens before merging this.
### Metrics - Add in-development `Bind` API to synchronous instruments. ([open-telemetry#5050](open-telemetry#5050)) - Clarify that View-provided metric stream `name` is not subject to instrument name syntax validation. ([open-telemetry#5094](open-telemetry#5094)) ### Common - Define the Core packages term. ([open-telemetry#5046](open-telemetry#5046)) - Rework contributing guide to reflect current process. ([open-telemetry#5072](open-telemetry#5072)) ### Compatibility - Stabilize sections of Prometheus and OpenMetrics Compatibility. - Stabilize translation of labels prefixed with `otel_scope_` to OTLP Instrumentation Scope. ([open-telemetry#5004](open-telemetry#5004)) - Stabilize OpenTelemetry Gauge and Sum to Prometheus transformations. ([open-telemetry#5034](open-telemetry#5034)) - Stabilize OpenTelemetry Instrumentation Scope to Prometheus labels transformation. ([open-telemetry#5052](open-telemetry#5052)) - Stabilize sections of Prometheus Metrics Exporter. - Stabilize temporality. ([open-telemetry#5024](open-telemetry#5024)) - Stabilize version and format. ([open-telemetry#5083](open-telemetry#5083)) - Stabilize port configuration. ([open-telemetry#5026](open-telemetry#5026)) - Stabilize `scope_info_enabled` configuration. ([open-telemetry#5056](open-telemetry#5056)) - Change Prometheus Metric Exporter config property recommended names (`without_scope_info` -> `scope_info_enabled`, `without_target_info` -> `target_info_enabled`, `with_resource_constant_labels` -> `resource_constant_labels`) ([open-telemetry#5071](open-telemetry#5071)) - Clarify that OTel SDKs should not use unofficial Prometheus clients. ([open-telemetry#5082](open-telemetry#5082)) ### OTEPs - Add OTEP for Semantic Convention Schema v2 with support for multiple convention registries and resolved schema format ([open-telemetry#4815](open-telemetry#4815)) --------- Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
### Metrics - Add in-development `Bind` API to synchronous instruments. ([open-telemetry#5050](open-telemetry#5050)) - Clarify that View-provided metric stream `name` is not subject to instrument name syntax validation. ([open-telemetry#5094](open-telemetry#5094)) ### Common - Define the Core packages term. ([open-telemetry#5046](open-telemetry#5046)) - Rework contributing guide to reflect current process. ([open-telemetry#5072](open-telemetry#5072)) ### Compatibility - Stabilize sections of Prometheus and OpenMetrics Compatibility. - Stabilize translation of labels prefixed with `otel_scope_` to OTLP Instrumentation Scope. ([open-telemetry#5004](open-telemetry#5004)) - Stabilize OpenTelemetry Gauge and Sum to Prometheus transformations. ([open-telemetry#5034](open-telemetry#5034)) - Stabilize OpenTelemetry Instrumentation Scope to Prometheus labels transformation. ([open-telemetry#5052](open-telemetry#5052)) - Stabilize sections of Prometheus Metrics Exporter. - Stabilize temporality. ([open-telemetry#5024](open-telemetry#5024)) - Stabilize version and format. ([open-telemetry#5083](open-telemetry#5083)) - Stabilize port configuration. ([open-telemetry#5026](open-telemetry#5026)) - Stabilize `scope_info_enabled` configuration. ([open-telemetry#5056](open-telemetry#5056)) - Change Prometheus Metric Exporter config property recommended names (`without_scope_info` -> `scope_info_enabled`, `without_target_info` -> `target_info_enabled`, `with_resource_constant_labels` -> `resource_constant_labels`) ([open-telemetry#5071](open-telemetry#5071)) - Clarify that OTel SDKs should not use unofficial Prometheus clients. ([open-telemetry#5082](open-telemetry#5082)) ### OTEPs - Add OTEP for Semantic Convention Schema v2 with support for multiple convention registries and resolved schema format ([open-telemetry#4815](open-telemetry#4815)) --------- Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
Fixes #4989
Changes
Stabilizes the configuration option
without_scope_info, documented in the Prometheus exporter spec. It is implemented by a few SDKs already #4989 (comment), an open PR for Python(open-telemetry/opentelemetry-python#5123), and an issue for DotNet(open-telemetry/opentelemetry-dotnet#7157). Both prometheus exporters in the collector have the option as well[1][2] (Although remote write exporter usesdisable_scope_infoinstead ofwithout_scope_info.For non-trivial changes, follow the change proposal process.
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check