-
Notifications
You must be signed in to change notification settings - Fork 653
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
Remove SDK dependency from auto-instrumentation #1420
Remove SDK dependency from auto-instrumentation #1420
Conversation
class Configurator: | ||
def configure(self): | ||
initialize_components() |
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.
This could be just a function as well right?
class Configurator: | |
def configure(self): | |
initialize_components() | |
def configure(): | |
initialize_components() |
and then in sitecustomize.py
it would just be entry_point.load()()
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.
Hmno, the load
method will yield the object that is pointed by the entry point value, in this case opentelemetry.sdk.configuration:Configurator
which is a class. An entry point can point to a method, or some other object, but what this approach that we have been using is to declare an ABC in the API that mandates that certain method is implemented in order for the entry pointed classes to have it and run it when sitecustomize
is invoked.
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.
I see, having the ABC would be helpful. Where would that definition go though? In opentelemetry-instrumentation
or opentelemetry-sdk
?
Does that mean that the other package would have to take the package that includes the ABC as a dependency?
(The opentelemtery-sdk
to have its Configurator
up to requirements and the opentelmetry-instrumentation
to be able to validate classes it finds through the entry points)?
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.
The ABC will be in the API.
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.
No, wait, that ABC should be in opentelemetry-instrumentation
. Sorry 😅
Description
This change moves the component initializing code that needs the SDK into the SDK's configuration module. This is then loaded in the auto instrumentation via entrypoint to ensure other SDKs can implement their own.
NOTE: the dependency from the SDK on the instrumentation package is only needed if we choose to include the configurator interface in the SDK. There is a following PR coming that propose this could be moved into a
configure_distro
entrypoint. This future PR will also include anopentelemetry-distro
package that configures the defaults.Fixes #1421
Type of change
Please delete options that are not relevant.
Does This PR Require a Contrib Repo Change?
Checklist: