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

Limit configuration envvars: unset = empty. #2959

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

Oberon00
Copy link
Member

Fixes #(no issue)

Changes

Hopefully editorial only:

The spec currently already says in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#parsing-empty-value:

The SDK MUST interpret an empty value of an environment variable the same way as when the variable is unset.

But then in the limit specification it used to say

Empty value is treated as infinity.

This would contradict the above spec, but the empty value is also the default, so the default is infinity, and if it's not set the default is used, so in the end you could interpret this with unset = empty.

However, if you did not see the spec about empty=unset above, there was one interpretation possible where setting the envvar to an empty value would overwrite another value explicitly set via a different configuration source (there is no different source specified at the moment though). This interpretation is now no longer possible.

@Oberon00 Oberon00 added bug Something isn't working area:sdk Related to the SDK editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind. area:configuration Related to configuring the SDK labels Nov 17, 2022
@Oberon00 Oberon00 requested review from a team November 17, 2022 16:11
@Oberon00 Oberon00 changed the title limit envvars: unset = empty. Limit configuration envvars: unset = empty. Nov 17, 2022
@Oberon00
Copy link
Member Author

Oberon00 commented Nov 17, 2022

I'm not sure if I should add a changelog entry for this, WDYT?

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

LGTM as long as the dropped note will be covered by #2963.

@reyang reyang merged commit e98d42a into open-telemetry:main Nov 21, 2022
@Oberon00 Oberon00 deleted the emptyvar_limit branch November 24, 2022 12:36
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK bug Something isn't working editorial Editorial changes only (typos, changelog, ...). No content-related changes of any kind.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants