Unset and empty environment variables are equivalent #2045
Conversation
There was a problem hiding this comment.
Before we accept this change the entire list of existing env variables must be scanned to verify if any of these variables have an expectation that the empty variable can have a particular meaning compared to the unset value.
An example of such env variable was the exporter compression setting, which previously could be set to empty string to mean "no compression" and which we updated to have "none" to represent the "no compression" meaning instead.
There may be other env variables which may exhibit the same behavior and in that case this case actually is a breaking change which requires more though.
@pellared will you be able to do that verification and confirm that we are not breaking anything?
P.S. I had a quick look and OTEL_SERVICE_NAME for example is unclear. IIRC if OTEL_SERVICE_NAME unset the SDK set the service.name to "unknown". So if you wanted an empty service.name you could do that by defining OTEL_SERVICE_NAME="". After this change it is no longer possible.
It is likely not important for "service.name" since I don't think an empty name is a useful valid value for it, but there may be other such variables where it actually is important to be able to set an empty string as a value because the default is not empty.
|
@tigrannajaryan Thanks for your feedback
That is one of the main reasons why I propose the As an example ❗ Take also into consideration that for example on Windows it is (almost?) NOT possible to set an empty env var. Not in the command prompt. not via PowerShell, not via GUI. Maybe it is possible via Windows Registry or WinAPI, but I am not sure and it would be VERY inconvenient. I can make a follow-up PR proposing |
Fair enough. I didn't check the actual behavior but it did appear like it could matter. I want to make sure there are no other env variables where it does actually matter. I suppose it is unlikely that any SDK implementations make a difference between unset and empty value but it is certainly technically possible, so better to double check.
I think this confirms one more time that we need to revise all env variables to make sure there are none where being able to set an empty value is an important use case and then we should accept this PR. |
The only issue I found was |
|
E.g. the Python SDK interprets setting FYI @owais |
|
Thanks for the mention @pellared. Python SDK implements Attribute Limits and it treats empty but set values as infinity (does not fallback to default). This is what the spec recommends if I understand it correctly. |
|
Luckily neither of the the two fields specify a default so if we make this change, we can also update attribute spec to set the default for these to "infinity". That way it shouldn't be a breaking change? |
|
@owais This is how I understand it: the default value of Maybe we should put Did ☝️ this here: dd5d00d The other option is to put |
|
For example, I agree that instead of empty value we should use something like |
Trying to address it here: ce2381c |
The Java SDK allows setting an empty name: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/EnvironmentResource.java#L43 |
|
@mateuszrzeszutek Do you think that changing this behavior in Java SDK/AutoInstr could be dangerous or even seen as something unwelcome? |
Truth be told, I have no idea. I doubt that anybody's willingly using |
|
I think it is possible for variables to exist where a value is an empty string. I think the env var spec behaves similarly to URL query parameters for example, where a name without a value means something. If we deny empty variables from having a value I think it's basically ok. But the corner case to consistent is map values like otel.resource.attributes, where it seems like I suspect implementations have treated this per-key so far and probably can without any significant UX issues. UX issues aren't a big deal anymore anyways since OTel engineering values settled on not including the user. |
|
The problem is that not all languages/OSes/shells are handling an empty value in the same way. In .NET (C# etc) setting an empty value unsets the environmental variable (sic!). See: https://docs.microsoft.com/en-us/dotnet/api/system.environment.setenvironmentvariable?view=net-5.0 On Windows setting an empty env var is VERY tricky. I suspect 95% of Windows IT users may have real problems doing it. For me having a different interpretation of an empty and unset environment variable is a kind of "billion-dollar mistake" 😉 I would be happy if the specs would at least encourage us to interpret empty and unset environment variables in the same way. However, maybe it makes even sense to require it? I am mostly worried about a scenario where setting an empty value may be an important scenario. E.g. |
+1. Also from a pure usability point of view, |
I agree. We need to make sure there are no env variables where there is an expectation that unset and set to empty trigger a different behavior. It means that any variable where an empty string is a meaningful choice MUST also have as a default value an empty string. I wonder what do we do if we discover such env vars which violate this rule and are already declared Stable. OTEL_SERVICE_NAME was the only one that I found which appears to violate the rule because the SDKs set it to "unknown" by default. However this is not currently defined by the spec, SDKs do that at their own risk, so perhaps we eliminate the "unknown" by default behavior and do not consider it a breaking change. If OTEL_SERVICE_NAME is unspecified then the service name will be empty. I don't know how much of a problem that is though. |
|
Updating limits spec in another PR sounds good. Just that until we do that, limits spec will be in somewhat of a contradiction with what we are adding in this PR. |
|
We have enough approvals but I'd like to double verify that at least @open-telemetry/java-maintainers and @open-telemetry/javascript-maintainers are fine with this ;) |
|
Not a java maintainer, but for the most part, this is how the java sdk works today. The DefaultConfigProperties class is the type safe accessor for all env var configuration, and almost all the types treat empty values as One exception is the getString() method. But looking at its usage, it should be fine to check for empty and return |
|
Please also rebase from main to make ready for merging. |
Changes
Describe how an empty value of an environment variable should be interpreted.
Related PR: #2052
Why
The problem is that not all languages/OSes/shells are handling an empty value in the same way.
In .NET (C# etc) setting an empty value unsets the environmental variable (sic!). See: https://docs.microsoft.com/en-us/dotnet/api/system.environment.setenvironmentvariable?view=net-5.0
On Windows setting an empty env var is VERY tricky. I suspect 95% of Windows IT users may have real problems doing it. You cannot do it in the command prompt. not via PowerShell, not via GUI. It is possible via Windows Registry or WinAPI, but it is VERY inconvenient and hacky.
Also from a pure usability point of view,
echo $SOMEVARdoes not allow to distinguish between empty and unset.Having a different interpretation of an empty and unset environment variable can be seen as a kind of "billion-dollar mistake" 😉
Reference: https://unix.stackexchange.com/questions/27708/is-there-a-difference-between-setting-an-environment-variable-to-the-empty-strin
TODOs after this PR
-1to set "no limit" Unset and empty environment variables are equivalent #2045 (comment)