-
Notifications
You must be signed in to change notification settings - Fork 936
[configuration] Clarify that the boolean value environment variable guidance is not applicable to other configuration interfaces #4723
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
base: main
Are you sure you want to change the base?
Conversation
…uidance is not applicable to other configuration interfaces
a7872e5 to
2e2811e
Compare
dashpole
left a comment
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.
I agree with the intent of the change, but it seems like it will create more confusion to me, as it implies (but doesn't specify) that the rest of the document applies to declarative configuration. I would prefer splitting the env var spec into a "common" config spec (excluding the boolean spec), and the env var spec with the current bool spec + anything else that only applies to env vars.
|
Makes sense, I will wait for a few more days and apply the suggested change if there is no dissent :) |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> Marks `configoptional.AddEnabledField` as beta. I have verified that it would work correctly for cookies configuration and made a PoC for keepalives (I will push a PR for it after this PR has been merged). I spent some time updating open-telemetry/opentelemetry-specification/issues/4344 and filed open-telemetry/opentelemetry-specification/pull/4723 as a way to explicitly state that the guidance only applies to environment variables. The issue remains unresolved, but given the current usages of `enabled` (see open-telemetry/opentelemetry-specification#4344 (comment)) I don't see a reason to not go forward with this. #### Link to tracking issue Updates #14021
@dashpole I pushed an attempt to split the guidance:
Other parts of the spec were using these sections as "here are the type definitions" and now the links for "type definitions" are split among two documents. We were never too consistent with this, so I feel like it is okay to do this |
|
cc @open-telemetry/configuration-approvers |
jack-berg
left a comment
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.
Couple small comments but I agree with the direction 👍
| The goal of this specification is to describe common guidance that is applicable | ||
| to all OpenTelemetry SDK configuration sources. | ||
|
|
||
| Implementations MAY choose to allow configuration via any source in this specification, but are not required to. |
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.
"via any source in this specification" What does this refer to? The other configuration interfaces?
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.
It appears that this sentence was adapted from the env var spec:
Implementations MAY choose to allow configuration via the environment variables in this specification, but are not required to. If they do, they SHOULD use the names listed in this document.
In which case, "source" refers to a data type?
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.
Any 'configuration source' was the intended meaning. Do you have suggestions on how to reword this to make it clearer?
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.
I added 'configuration' to that sentence let me know if that is clearer
|
|
||
| #### Enum | ||
|
|
||
| Enum values SHOULD be interpreted in a case-insensitive manner. |
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.
This might not be possible in declarative config. In JSON schema, an enum type is specified as part of the schema (for example). The validation of a config file's adherence to the schema is often done with 3rd party JSON schema tools, and since there is no way to specify a case-insensitive enum in JSON schema, there's (likely) no way for the 3rd party JSON schema tools to ignore case.
Case insensitive enums are also problematic for the programmatic config interface.
Consider moving this back to the env var specific section.
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.
Should I also move the sentence that says:
When reporting configuration errors, implementations SHOULD display the original
user-provided value to aid debugging.
It seems to be there only because of the case in-sensitiveness and I think it is hard to apply in all contexts (does it mean that if I set a: "foo" I should get a different message than when I set a: 'foo' or a: !!str foo?)
Updates #4344
Changes
Clarifies that environment variable guidance regarding boolean values is not applicable to other configuration interfaces. This does not explicitly state what the guidance should be in other cases.