Skip to content

Remove SDK's dependency on opentelemetry-instrumentation #2184

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

Closed
owais opened this issue Oct 8, 2021 · 4 comments · Fixed by #2187 or #2188
Closed

Remove SDK's dependency on opentelemetry-instrumentation #2184

owais opened this issue Oct 8, 2021 · 4 comments · Fixed by #2187 or #2188

Comments

@owais
Copy link
Contributor

owais commented Oct 8, 2021

Today the SDK depends on opentelemetry-instrumentation package because it needs to import opentelemetry.instrumentation.utils._SUPPRESS_INSTRUMENTATION_KEY in order to suppress instrumentation in its own exporters. We decided in last SIG call to get rid of this dependency as it does not make sense to have SDK depend on the instrumentation package and it complicates things in general.

Right now the _SUPRESS_INSTRUMENTATION variable is a an opentelemetry context key i.e, it is a randomly generated uuid. We have two options to fix this:

  1. Make _SUPPRESS_INSTRUMENTATION_KEY a static symbol e.g, a well-known string value that is used by convention by SDK and opentelemetry-instrumentation both without importing it from anywhere. This is the simplest and quite robust but it technically violates the spec as context keys should not be static strings.

  2. Move the _SUPRESS_INSTRUMENTATION_KEY to opentelemetry.context._SUPRESS_INSTRUMENTATION_KEY. We'll place it in the API package but it'll not be part of the public context API just like it is not a part of public instrumentation API today. It'll continue to be strictly used by internal package just like today. Both SDK and instrumention packages will import it from opentelemetry.context._SUPPRESS_INSTRUMENTATION_KEY. Once the spec recommends a way to implement this officially, we can get rid of this or make it part of the public API.

3rd options is to have SDK or instrumentation package "register" a global key like this in opentelemetry.context. namespace just like we register propagators or providers but I feel it'd be overkill for this problem. I think we should go with 2.

@codeboten
Copy link
Contributor

codeboten commented Oct 8, 2021

I would support Option 2, I think the dependency on the instrumentation package is problematic.

@owais
Copy link
Contributor Author

owais commented Oct 8, 2021

@open-telemetry/python-approvers @open-telemetry/python-maintainers thoughts?

@NathanielRN
Copy link
Contributor

It does feel wrong to have opentelemetry-sdk depend on opentelemetry-instrumentation just for that one value, but I’ve found it really convenient to install the SDK package and get the instrumentation package for free. I think the common use case would include both, but have the requirement means the “SDK without instrumentation” cannot exist right now. So this change makes sense.

Maybe installing all these “regular use case” packages is what distros should solve.

@owais
Copy link
Contributor Author

owais commented Oct 10, 2021

Each distro can solve whatever it wants :) but yes this is something that all distros would obviously want to solve.

The upcoming opentelemetry python package will also solve this problem i.e, install everything required to get started with Otel + Python

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants