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

Instrumentation Package depends on the OTel SDK #1405

Merged

Conversation

NathanielRN
Copy link
Contributor

Description

As a follow up to #1036, with the new configurability, the instrumentation package takes a dependency on opentelemetry.sdk because of its use in opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/components.py

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
    - [ ] Unit tests have been added
    - [] Documentation has been updated

@NathanielRN NathanielRN requested review from a team, aabmass and ocelotl and removed request for a team November 20, 2020 20:41
@NathanielRN NathanielRN force-pushed the instrumentation-dependencies branch from dd4f7f0 to 6ec98a6 Compare November 20, 2020 20:42
@NathanielRN
Copy link
Contributor Author

@owais Should be able to give a quick review since he has context!

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a direct contradiction to this?

@NathanielRN
Copy link
Contributor Author

@lzchen Yes I would agree that it contradicts that issue which has a valid point...

However, the opentelemetry-instrumentation package right now, and more specifically "auto instrumentation" inside of it, does depend on the opentelemetry-sdk package.

I can think of these options:

  1. Keep auto instrumentation here in opentelemetry-instrumentation and mention in the docs that auto instrumentation only works for the opentelemetry-sdk TracerProvider
  2. Split auto instrumentation out into opentelemetry-auto-instrumentation and make it depend on opentelemtry-instrumentation and the opentelemetry-sdk TracerProvider, which means we can validate inputs
  3. Split auto instrumentation out into opentelemetry-auto-instrumentation and find the users' TracerProvider using OTEL_PYTHON_TRACER_PROVIDER so that it is generalizable, but we can't validate inputs... and it's not clear to me how we would find the classes

I think we should do 1 and then 2 as a quick follow up. If users really want 3 it shouldn't be hard to create a modification of the opentelemetry-auto-instrumentation package. (That way they can support their own custom TracerProvider auto instrumentation packages)

Please let me know what you think! Also cc @codeboten

@NathanielRN NathanielRN requested a review from lzchen November 23, 2020 17:25
@lzchen
Copy link
Contributor

lzchen commented Nov 24, 2020

I think we should do 1 and then 2 as a quick follow up.

I agree with this.

@NathanielRN NathanielRN force-pushed the instrumentation-dependencies branch from 6ec98a6 to 3749d9c Compare November 24, 2020 17:25
@codeboten
Copy link
Contributor

do we need the SDK at all here? couldn't we use the entrypoints defined in opentelemetry_tracer_provider and opentelemetry_exporter to determine what should be available to load for a user? I guess the one thing that we don't have a solution for is loading the resource class though.

@NathanielRN
Copy link
Contributor Author

The point about the opentelemetry_tracer_provider is a really good point! And the Resource point is also a good call out...

I would stick by my point earlier that we don't really have to provide a 1 size fits all auto instrumentation solution. The example isn't too complicated, so I would imagine people could make their own auto instrumentation package if they really want to use another TracerProvider.

@NathanielRN
Copy link
Contributor Author

Closing this in favor of #1415

@NathanielRN NathanielRN deleted the instrumentation-dependencies branch November 25, 2020 18:12
@NathanielRN NathanielRN restored the instrumentation-dependencies branch November 26, 2020 00:15
@codeboten
Copy link
Contributor

To unblock the release, i propose we merge this change and follow up with removing the dependency on the SDK in the next release

@NathanielRN NathanielRN reopened this Nov 26, 2020
@NathanielRN NathanielRN force-pushed the instrumentation-dependencies branch from 3749d9c to 59728f8 Compare November 26, 2020 00:18
@NathanielRN
Copy link
Contributor Author

Agreed, reopening this PR as a result of the conversion on gitter found here: https://gitter.im/open-telemetry/opentelemetry-python?at=5fbef0f82cb422778f3a1d9c

@NathanielRN NathanielRN force-pushed the instrumentation-dependencies branch 2 times, most recently from afd89be to 033a015 Compare November 26, 2020 00:22
@NathanielRN NathanielRN changed the title Instrumentation Package should depend on OTel SDK Instrumentation Package has a dependency on the OTel SDK Nov 26, 2020
@NathanielRN NathanielRN force-pushed the instrumentation-dependencies branch from 033a015 to fa676f8 Compare November 26, 2020 00:23
@NathanielRN NathanielRN changed the title Instrumentation Package has a dependency on the OTel SDK Instrumentation Package depends on the OTel SDK Nov 26, 2020
@codeboten
Copy link
Contributor

Filed #1421 to track removing this dependency

@codeboten
Copy link
Contributor

@lzchen please review the discussion and approve if you're ok with this plan moving of removing the dependency in the next release.

@codeboten codeboten merged commit 895be85 into open-telemetry:master Nov 26, 2020
@NathanielRN NathanielRN deleted the instrumentation-dependencies branch November 26, 2020 17:46
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.

5 participants