Skip to content
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

[configuration] Add exception for 'false default' when there is a double negation #4351

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jan 8, 2025

Relates to #4344.

Changes

Adds exception to naming and default value rule for environment variables.

This is a backwards compatible change since the original statement is phrased as a 'SHOULD'. The OTEL_SDK_DISABLED deviates from this new norm (this is not a spec violation but a lack of consistency).

The wording tries to be similar to the one in footnote [1] here: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.40.0/specification/protocol/exporter.md

There are several OpenTelemetry artifacts that are either not bound by the specification or have decided, for usability reasons, to not follow this rule in this case. This change would, I argue, bring more consistency and less confusion to end users.

I plan to submit a follow-up PR to change MeterConfig.disabled to MeterConfig.enabled if this PR is accepted.
I don't plan to submit a PR to migrate OTEL_SDK_DISABLED, but this could be done if there is interest.

Thanks to @pellared for guidance on this :)

For non-trivial changes, follow the change proposal process.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support but would recommend that it be left open through at least the Tue, Jan 21 spec meeting given the contentious discussion around the topic previously.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing ambiguity and inconsistency to a clear and well litigated part of the specification. It is only substantiated with a biased subjective opinion about an undefined user-base.

The prior decision for this part of the specification was made on logical reasoning. There are neither objective data nor facts being presented here that add to the prior discussion. Do these exist? Can you provide them to motivate this change?

The added ambiguity and inconsistency deteriorate the specification and should not be accepted without better motivation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants