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

[SDK] Support OTEL_SDK_DISABLED environment variable #3245

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jan 15, 2025

Fixes #1631

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Copy link

netlify bot commented Jan 15, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 5da4f7f
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67997132cbac2a00081e2ede

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.84%. Comparing base (6603c3a) to head (5da4f7f).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3245      +/-   ##
==========================================
+ Coverage   87.80%   87.84%   +0.05%     
==========================================
  Files         198      202       +4     
  Lines        6324     6345      +21     
==========================================
+ Hits         5552     5573      +21     
  Misses        772      772              
Files with missing lines Coverage Δ
api/include/opentelemetry/logs/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/metrics/provider.h 100.00% <ø> (ø)
api/include/opentelemetry/trace/provider.h 100.00% <ø> (ø)
exporters/ostream/test/ostream_log_test.cc 96.18% <100.00%> (ø)
sdk/src/common/disabled.cc 100.00% <100.00%> (ø)
sdk/src/logs/provider.cc 100.00% <100.00%> (ø)
sdk/src/metrics/provider.cc 100.00% <100.00%> (ø)
sdk/src/trace/provider.cc 100.00% <100.00%> (ø)

@marcalff marcalff changed the title Implement OTEL_SDK_DISABLED [SDK] Support OTEL_SDK_DISABLED environment variable Jan 28, 2025
@marcalff marcalff added breaking change API or ABI breaking change pr:please-review This PR is ready for review labels Jan 29, 2025
@marcalff marcalff marked this pull request as ready for review January 29, 2025 00:40
@marcalff marcalff requested a review from a team as a code owner January 29, 2025 00:40
@lalitb
Copy link
Member

lalitb commented Jan 29, 2025

@marcalff Thanks for the PR. Just a thought—rather than including set-provider methods in the SDK that check for the disabled , wouldn't it be better to offer more granular control within the get_logger/tracer/meter methods, allowing them to return no-op objects when the SDK is disabled? This way, users wouldn't need to use a new api, and it also ensures that new loggers or tracers won't function if the environment variable is disabled after the application starts.

@marcalff
Copy link
Member Author

@marcalff Thanks for the PR. Just a thought—rather than including set-provider methods in the SDK that check for the disabled , wouldn't it be better to offer more granular control within the get_logger/tracer/meter methods, allowing them to return no-op objects when the SDK is disabled? This way, users wouldn't need to use a new api, and it also ensures that new loggers or tracers won't function if the environment variable is disabled after the application starts.

@lalib

Thanks for the suggestion.

I considered it as well, but decided against it for the following reasons:

  • In terms or robustness, technical risk, and performance overhead,
    it is better to talk to the default Noop provider when the SDK is
    disabled, rather that invoke code in a real SDK implementation that will
    degrade itself to a Noop.

  • For SDK code that opentelemetry-cpp has control of, this may work,
    but there are unknowns with SDK classes implemented by third parties,
    which still may not honor OTEL_SDK_DISABLED.

  • Implementing a check in the SDK on get_tracer() implies that the SDK
    always honor environment variables defined in the spec.
    This is invalidated by the declarative configuration project
    (config.yaml), because the spec says explicitly that every environment
    variable beside OTEL_EXPERIMENTAL_CONFIG_FILE is to be ignored
    when a yaml configuration file is used.
    Implementing the check inside get_tracer() in the SDK would break this.
    See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#declarative-configuration

As a result, the current proposal:

  • Puts the test for OTEL_SDK_DISABLED in the SetTracerProvider() code path,
    when a configured SDK is installed by the application main().

  • Will work to support declarative configuration later, I plan to add
    opentelemetry::sdk::trace::Provider::SetTracerProviderNoEnv(),
    to be used with a config.yaml file.

  • Leaves the SDK execution (Get() code path) unchanged,
    and independent on how the SDK was configured.

As for environment variables changing during the process execution,
I don't think there is any expectation for this to work.
For example, changing an endpoint URL after the SDK is already configured
will have no effect.

In my understanding,
environment variables are meant to be a simple way to change the SDK
configuration without having to change code in the main application,
but are not meant to change during execution.
Honoring a change in OTEL_SDK_DISABLED between start and end span will cause
chaos, for unclear benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change API or ABI breaking change pr:please-review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SDK] Support OTEL_SDK_DISABLED environment variable
3 participants