Add OTEL_SDK_ENABLED env var to disable opentelemetry SDK.

[brunobat]

trafficstars

Fixes #2667

Changes

Add OTEL_SDK_ENABLED environment property This will allow applications to easily deactivate opentelemetry on deployments where it's not supported or needed. This property is already present in the Java SDK version under experimental status. See: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#disabling-opentelemetrysdk

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: brunobat / name: Bruno Baptista (27e70e4c2245c0710605aff8636d13c7c4cab65e)

[brunobat]

Did the requested changes @arminru @joaopgrassi .

[brunobat]

Hi @jmacd, can we move forward with the merge?

[Emily-Jiang]

It seems this PR was blocked from merging as it requires Code Owner to review. Can someone from that group view this and merge this PR? Thanks

[brunobat]

@Aneurysm9 is this ok for you now?

[atoulme]

+1 from me as a SDK user.

[ocelotl]

On the Python side, it looks good in my opinion but I find it a bit unspecific since I imagine a few ways of implementing this, probably being unspecific is intentional.

[brunobat]

I think this has merit to be included. However, by introducing the new boolean value type to the specification for environment variables it needs to be done with understanding of the impact it will have at large to the specification.

The boolean type needs to be explicitly defined in the special configuration types section before or as a part of this PR for the changes to be accepted.

• Generalizing the type will ensure that if another boolean variable is added it is added with the same interpretation. This will also help future authors understand the need to define variables as the positive action being taken and how values will be interpreted.
• There is still a remaining unspecified aspects to the boolean type, it is not explicit if its values (false, true, or "any other value") are case sensitive.
• Restricting values for all SIGs to only true, false, and "any other value" may be too restrictive. It may prevent idiomatic behavior of language implementation. Similar to the other configuration types, booleans should be defined in a way that allows languages to extend their definition.

Help is appreciated. Do you have a suggestion on how to define the boolean in the spec?

[MrAlias]

Help is appreciated. Do you have a suggestion on how to define the boolean in the spec?

Sure, I'll comment with a rough-draft of a Boolean value sub-section suggestion in a bit.

[MrAlias]

Help is appreciated. Do you have a suggestion on how to define the boolean in the spec?

Sure, I'll comment with a rough-draft of a Boolean value sub-section suggestion in a bit.

### Boolean value

Any value that represents a boolean MUST interpret the case-insensitive string `"true"` and the integral value of `1` as true values.
An SDK MAY extend this definition and define additional values that are interpreted as true.
Any value not explicitly defined here or by the SDK as a true value, including unset and empty values, MUST be interpreted as false.

[arminru]

@MrAlias Your suggestion looks good to me overall, thank you!

An SDK MAY extend this definition and define additional values that are interpreted as true.

Do you already have any value other than "true", "True", "TRUE", or 1 in mind here, or would you add this MAY clause just to make sure all potential (future) languages are covered?

[MrAlias]

Do you already have any value other than "true", "True", "TRUE", or 1 in mind here

The Linux CLI space will also use t, y,and yes at times for this. Though, I didn't think these common enough to include at the spec level.

or would you add this MAY clause just to make sure all potential (future) languages are covered?

Mostly this ^. Similar to the integer value being extended by a language, I wanted to make sure idiomatic usage in other languages are supported if they exist.

[MrAlias]

I'm also okay with dropping 1 as a defined value as well and sticking with just true (case insensitive) if that is more desired.

[lalitb]

On the C++ side, need to look if we can support this environment variable. The SDK gets enabled automatically once it is linked to the application, and it can't be disabled afterwards. We do however have limited support of dynamic loading the SDK as shared object using dlopen(Linux) / LoadLibrary(Windows) where the env variable can be used.

[brunobat]

@MrAlias Updated the PR to include the Boolean Value definition you proposed.

[carlosalberto]

@lalitb This is a rather optional behavior, so it would be perfectly fine to not support it for C++ (although it's good you know about its existence, etc)

[Aneurysm9]

Any value not explicitly defined here or by the SDK as a true value, including unset and empty values, MUST be interpreted as false.

I would invert this and explicitly define falsy values as unset, empty string, false or 0 and any other value as truthy. I think this would align better with the experience of developers used to dynamic languages that treat any non-falsy value as truthy.

[brunobat]

The Boolean definition has now been moved to #2729

[jmacd]

@MrAlias do you accept the current contents of this PR? I believe the goal of specifying a "DISABLED" variable with exactly one meaningful value ("true") was to avoid the general problem you described with interpreting booleans.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[carlosalberto]

Ping @MrAlias

[MrAlias]

@MrAlias do you accept the current contents of this PR? I believe the goal of specifying a "DISABLED" variable with exactly one meaningful value ("true") was to avoid the general problem you described with interpreting booleans.

This is the correct direction, but a boolean value is still under-specified in this PR. It looks like #2729 needs to be resolved before this can progress, as was discussed in the spec SIG meeting two weeks ago.

[marcalff]

Ok in principle, this will allows to disable telemetry in every applications the same way, without application specific options, or no options at all when the application forgot to provide one: very desirable feature.

About the naming: OTEL_SDK_DISABLED=False, the default behavior, is a double negation

This is equivalent to: OTEL_SDK_ENABLED=True

with no OTEL_SDK_ENABLED environment variable provided meaning True.

The concern about the double negation is the confusion it can create, in particular for non English speakers.

It seems the PR started with OTEL_SDK_ENABLED=True, then later changed to OTEL_SDK_DISABLED=False, but I can't find a comment or a rationale for it, so not sure who requested that, and why.

This is minor (or bikeshed even), feel free to ignore. Looking to support this in opentelemetry-cpp.

[jack-berg]

@marcalff see this comment. The current thinking is that any boolean environment variable should be named *_ENABLED_* or *_DISABLED_* such that if a default value of false aligns with the desired default behavior.

[marcalff]

@marcalff see this comment. The current thinking is that any boolean environment variable should be named *_ENABLED_* or *_DISABLED_* such that if a default value of false aligns with the desired default behavior.

Thanks @jack-berg for the clarification.

I think this is a dangerous path to take : if somehow, for a given variable, we decide to change the default for whatever reason, the variable will not be renamed, creating a weird state.

In other words, it could be desirable to decouple the variable name (immutable to avoid breaking configuration for users) from the default value (possibly subject to change other time). This convention creates coupling.

Regards, -- Marc

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[brunobat]

This is still waiting for https://github.com/open-telemetry/opentelemetry-specification/issues/2729

[reyang]

This is still waiting for #2729

#2729 is resolved by #2755.