-
Notifications
You must be signed in to change notification settings - Fork 624
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
Adds support OTEL_SDK_DISABLED environment variable #3648
Adds support OTEL_SDK_DISABLED environment variable #3648
Conversation
I don't think this should go in the main SDK code. My understanding is this should be targeted for disabling the auto instrumentation part. |
I went through the environment variable specification and could not find any reference that it should only be for auto instrumentation. Also, had a look at the Javascript library for opentelemetry. Looking at their code for disabling via environment, it did not look as if it is only for auto instrumentation. However I may have understood incorrectly or maybe the implementation of that library is incorrect. Happy to discuss about this. |
I looked at the Java support for this, which was the group that proposed this env to the specification. And my understanding is that it should be applied to auto-instrumentation. Let me ask a question in the specification/java slack channel. |
Hi @srikanthccv, did you get some time to ask around about this? |
It does seem like it's being used in the SDK and not autoinstrumentation for Java so this seems like an appropriate change. |
opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
Outdated
Show resolved
Hide resolved
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.
Just a couple of nits but otherwise LGTM! Please add a CHANGELOG
entry.
ebcf317
to
41cf788
Compare
@lzchen Thanks for the comments. I have updated the PR based on your comments. Also, I think this PR needs a label of "Approve Public API check" which I don't know how to add. If you or someone else can help me out with this, I would be grateful. |
Sorry for the lack of response here and yes you are correct. |
Please fix the build issues and then we can get this merged! |
@lzchen Regarding the pylint errors, I am having a bit of trouble figuring out why I am getting the error. I have suppressed the error, however if you think there is a better way to resolve this, please feel free to opine. As for the mypy errors, I believe it is not related to my change, I am getting the errors on the main branch when I run tox locally on my setup. |
mypy errors should be fixed with this. Please rebase and we can merge this :) |
ca44d16
to
4e0bf2b
Compare
Head branch was pushed to by a user without write access
4e0bf2b
to
4fadaa1
Compare
@lzchen I have rebased my changes. Thanks :) |
Pull Request is not mergeable
Description
Adds support for OTEL_SDK_DISABLED environment variable. If set to "true", the SDK will do no operations.
Fixes #3184
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Output in console:
Does This PR Require a Contrib Repo Change?
Checklist: