feat: Add compatibility with env OTEL_SDK_DISABLED

[pedrofurtado]

feat: Add compatibility with env OTEL_SDK_DISABLED

Currently, the ruby gem does not have compatibility with env OTEL_SDK_DISABLED defined in Open Telemetry specification.

Specification: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration

This PR adds this integration.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pedrofurtado / name: Pedro Furtado (0b2653d, 5a7ae60, 3debcde, 7bbf5c1, ffac037, 0e69b0e, 3ee1b46, 93d77cb)

[kaylareopelle]

Thanks for this PR, @pedrofurtado!

There is a test helper, OpenTelemetry::TestHelpers.with_env that can be used to help test behavior with environment variables.

For example:

opentelemetry-ruby/common/test/opentelemetry/common/utilities_test.rb

Lines 102 to 104 in 84d64ca

	OpenTelemetry::TestHelpers.with_env('a' => 'b') do
	_(common_utils.config_opt('a', default: 'bar')).must_equal('b')
	end

I started writing this comment before you removed the test. Could you add a test that leverages this helper?

[pedrofurtado]

@arielvalentin @kaylareopelle I've created a test for this.
But the CI is not running, is pending approval. Can you approve the workflow to run? 👍

[pedrofurtado]

@arielvalentin @kaylareopelle The CI finished, but a rubocop offense was notified. I fixed that, but I need approval again for CI please 😄

[arielvalentin]

@arielvalentin @kaylareopelle The CI finished, but a rubocop offense was notified. I fixed that, but I need approval again for CI please 😄

Thanks for your patience. You'll require approvals to run CI until your first PR is merged.

[pedrofurtado]

Good news, CI is green ✅

[arielvalentin]

@robertlaurin is this the best place for us to check this?

Does it make sense to check that in the instance method instead? https://github.com/open-telemetry/opentelemetry-ruby/blob/main/sdk/lib/opentelemetry/sdk/configurator.rb#L144

Do we want to support being able to change the envar and re-initialize in a running program?

[pedrofurtado]

Here in our applications works like a charm in this place (using monkey patch).

The PR content was based on this comment: #1605 (comment)

[arielvalentin]

Yes, that was my comment 😄

I reached out to @robertlaurin here since he designed this component and wanted to solicit his input about where this check should happen.

If a user is not using the DSL, they can still create an instance of the Configurator and call the configure method bypassing the envar.

They could also not use the configurator at all and assemble the tracer provider and resources themselves.

I just want to make sure we are hooking this into the appropriate scope, so folks are not surprised that it does not work as anticipated in specific scenarios; or if we are OK with this minimal implementation.

[arielvalentin]

I had similar questions @kaylareopelle to the ones you brought up

[kaylareopelle]

thank you! missed this! (link to comment)

[kaylareopelle]

The spec for this environment variable feels a little vague to me. Do we know the scope of the OTEL_SDK_DISABLED env var?

The current implementation would skip creating tracer providers through configuration, but someone could still manually create functional tracer providers and tracers. Is that acceptable?

I took a look at Python's implementation, and it seems like they always return no-ops when OTEL_SDK_DISABLED is detected.

However, Java's implementation seems closer to this approach. Configuration is skipped, but I'm not sure if that means you cannot create working tracers, etc.

[pedrofurtado]

Hi @arielvalentin @kaylareopelle! 👋
Is the PR ok?

[arielvalentin]

I don't know. We don't quite understand how this environment variable is supposed to work per the specification. Right now this PR only works in a specific use case.

If our users initialize the SDK in a different way, I.e. manual configuration, then the envar will not work as expected and we may get a bug report.

That is why we are asking the question about whether or not this check is happening the the correct place.

[kaylareopelle]

I opened an issue in the semantic conventions repo to see if we can get some help clarifying the functionality: open-telemetry/semantic-conventions#1667

Once we understand the spec better, we can move forward. Thanks for your patience, @pedrofurtado!
cc @arielvalentin

[arielvalentin]

@kaylareopelle I think that should be in the spec repo thought right? Not semconv?

[kaylareopelle]

@arielvalentin - Thank you, you're right! 🤦 I'll get that fixed.

[kaylareopelle]

New issue in the right repo! open-telemetry/opentelemetry-specification#4332

Thanks, @arielvalentin!