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

Remove SDK dependency from auto-instrumentation #1420

Merged
merged 16 commits into from
Dec 14, 2020

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Nov 25, 2020

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 an opentelemetry-distro package that configures the defaults.

Fixes #1421

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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 updated
  • Documentation has been updated

@codeboten codeboten requested review from a team, owais and ocelotl and removed request for a team November 25, 2020 22:19
@codeboten codeboten changed the title Instrumentation decoupling Remove SDK dependency from auto-instrumentation Nov 25, 2020
Comment on lines 166 to 168
class Configurator:
def configure(self):
initialize_components()
Copy link
Contributor

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?

Suggested change
class Configurator:
def configure(self):
initialize_components()
def configure():
initialize_components()

and then in sitecustomize.py it would just be entry_point.load()()

Copy link
Contributor

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.

Copy link
Contributor

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)?

Copy link
Contributor

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.

Copy link
Contributor

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 😅

@codeboten codeboten marked this pull request as draft November 26, 2020 01:52
@codeboten codeboten marked this pull request as ready for review December 10, 2020 21:55
tox.ini Show resolved Hide resolved
@codeboten codeboten merged commit 0011637 into open-telemetry:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove SDK dependency from instrumentation package
5 participants