-
Notifications
You must be signed in to change notification settings - Fork 887
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
Add OTEL_SDK_DISABLED env var to disable opentelemetry SDK. #2679
Conversation
|
Signed-off-by: Bruno Baptista <[email protected]>
Signed-off-by: brunobat <[email protected]>
Signed-off-by: brunobat <[email protected]>
Did the requested changes @arminru @joaopgrassi . |
Hi @jmacd, can we move forward with the merge? |
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 |
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 advise to circulate this PR in the Maintainers meeting on Monday to make sure there are no languages where peculiarities of env variable querying can impact on how the variable's values should be defined. We have had some issues in this area which resulted in the precise behavior specification for Numbers and Enums.
I think folks brought up important topics since I reviewed that have to be discussed/addressed so I'm reverting it for now.
…a new issue. Signed-off-by: brunobat <[email protected]>
The Boolean definition has now been moved to #2729 |
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.
As the review of this spec update has evolved, it seems the title, description, and content of the change have diverged.
The current state of the proposal has the env var name ending with _DISABLED
.
✅ That's reflected in the title and the tables in compliance matrix and SDK env vars list.
_ENABLED
.
❓ The PR's original description also says _ENABLED
which was the original proposal. It could help people reviewing this PR for the first time after all the discussion for the PR description to be edited to include an update describing the current state of the proposal.
Co-authored-by: Robb Kidd <[email protected]>
@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 PR was marked stale due to lack of activity. It will be closed in 7 days. |
Ping @MrAlias |
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. |
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: This is equivalent to: with no The concern about the double negation is the confusion it can create, in particular for non English speakers. It seems the PR started with This is minor (or bikeshed even), feel free to ignore. |
@marcalff see this comment. The current thinking is that any boolean environment variable should be named |
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, |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This is still waiting for #2729 |
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