-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
otel/prometheus_exporter: disable otel_scope_info by default #4775
Conversation
The collector doesn't implement this yet and the metrics are not very useful. See: open-telemetry/opentelemetry-collector-contrib#24248 Signed-off-by: Goutham <[email protected]>
Signed-off-by: Goutham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, cc-ing @ptodev who worked on upgrading our OTel dependency this past week to merge.
I'm not necessarily against the change, but I'm not sure if this is going against the compatibility spec. The compatibility spec states:
I took this to mean that the configuration option should exist, and should default to true. Did I misunderstand this line? |
FWIW, I'd interpret this the same way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs are OK as-is.
This doesn't account for the clarification of the default status.
Kicked off a discussion in OTEL land which led to this: open-telemetry/opentelemetry-specification#3657 Having the metrics switched on by default doesn't provide a lot of value and I think the spec will evolve to disable metrics unless there are attributes. Should we go ahead and merge this? cc @rfratto |
The collector doesn't implement this yet and the metrics are not very useful.
See: open-telemetry/opentelemetry-collector-contrib#24248
Config converters updated