Skip to content
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

Conversation

puskardeb
Copy link
Contributor

@puskardeb puskardeb commented Jan 24, 2024

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.

  • New feature (non-breaking change which adds functionality)

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

  • Ran locally in console with the help of the following code.
from opentelemetry.sdk.resources import SERVICE_NAME, Resource

from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import BatchSpanProcessor, ConsoleSpanExporter

from opentelemetry import metrics
from opentelemetry.sdk.metrics import MeterProvider
from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader, ConsoleMetricExporter

resource = Resource(attributes={
    SERVICE_NAME: "your-service-name"
})

traceProvider = TracerProvider(resource=resource)
processor = BatchSpanProcessor(ConsoleSpanExporter())
traceProvider.add_span_processor(processor)
trace.set_tracer_provider(traceProvider)

reader = PeriodicExportingMetricReader(ConsoleMetricExporter())
meterProvider = MeterProvider(resource=resource, metric_readers=[reader])
metrics.set_meter_provider(meterProvider)

tracer = trace.get_tracer("test tracer")
with tracer.start_as_current_span("test span"):
    print(1)

Output in console:

$ export OTEL_SDK_DISABLED=true
SDK is disabled.
1
$ unset OTEL_SDK_DISABLED
1
{
    "name": "test span",
    "context": {
        "trace_id": "0x19b1ce33587fbe0064b072bde41e1e5d",
        "span_id": "0xaabfc3da341721d4",
        "trace_state": "[]"
    },
    "kind": "SpanKind.INTERNAL",
    "parent_id": null,
    "start_time": "2024-01-24T13:40:37.576284Z",
    "end_time": "2024-01-24T13:40:37.576361Z",
    "status": {
        "status_code": "UNSET"
    },
    "attributes": {},
    "events": [],
    "links": [],
    "resource": {
        "attributes": {
            "service.name": "your-service-name"
        },
        "schema_url": ""
    }
}

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@puskardeb puskardeb requested a review from a team as a code owner January 24, 2024 13:49
Copy link

linux-foundation-easycla bot commented Jan 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@srikanthccv
Copy link
Member

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.

@puskardeb
Copy link
Contributor Author

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.

@srikanthccv
Copy link
Member

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.

@puskardeb
Copy link
Contributor Author

Hi @srikanthccv, did you get some time to ask around about this?

@lzchen
Copy link
Contributor

lzchen commented Mar 7, 2024

@srikanthccv

@srikanthccv

It does seem like it's being used in the SDK and not autoinstrumentation for Java so this seems like an appropriate change.

Copy link
Contributor

@lzchen lzchen left a 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.

@puskardeb puskardeb closed this Mar 8, 2024
@puskardeb puskardeb force-pushed the 3184-support-for-env-var-otel-sdk-disabled branch from ebcf317 to 41cf788 Compare March 8, 2024 12:16
@puskardeb puskardeb reopened this Mar 8, 2024
@puskardeb
Copy link
Contributor Author

@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.

@lzchen lzchen added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Mar 8, 2024
@srikanthccv
Copy link
Member

Sorry for the lack of response here and yes you are correct.

@lzchen
Copy link
Contributor

lzchen commented Mar 12, 2024

@puskardeb

Please fix the build issues and then we can get this merged!

@puskardeb
Copy link
Contributor Author

@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.

@lzchen
Copy link
Contributor

lzchen commented Mar 14, 2024

@puskardeb

mypy errors should be fixed with this. Please rebase and we can merge this :)

@ocelotl ocelotl force-pushed the 3184-support-for-env-var-otel-sdk-disabled branch from ca44d16 to 4e0bf2b Compare March 14, 2024 18:36
@ocelotl ocelotl enabled auto-merge (squash) March 14, 2024 18:40
auto-merge was automatically disabled March 14, 2024 21:38

Head branch was pushed to by a user without write access

@puskardeb puskardeb force-pushed the 3184-support-for-env-var-otel-sdk-disabled branch from 4e0bf2b to 4fadaa1 Compare March 14, 2024 21:38
@puskardeb
Copy link
Contributor Author

@lzchen I have rebased my changes. Thanks :)

@lzchen lzchen enabled auto-merge (squash) March 14, 2024 22:28
auto-merge was automatically disabled March 14, 2024 22:34

Pull Request is not mergeable

@ocelotl ocelotl merged commit 2574707 into open-telemetry:main Mar 20, 2024
232 checks passed
@puskardeb puskardeb deleted the 3184-support-for-env-var-otel-sdk-disabled branch March 21, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OTEL_SDK_DISABLED
4 participants