-
Notifications
You must be signed in to change notification settings - Fork 624
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
Setup SDK to be a Namespace Package so it can be extended #1205
Setup SDK to be a Namespace Package so it can be extended #1205
Conversation
Are there any other implications when moving from init.py to init.pyi? |
The pyi files aren't used at runtime at all, AFAIK, so this changes the behavior. Unless we are ok to change the behavior, this won't work
|
3273260
to
9bf4113
Compare
9bf4113
to
569c098
Compare
@owais What I understand echoes what @aabmass said: Anything in this file ( PEP 561 would consider this package a "stub"
And according to this explanation from the website for a Python IDE,
We could add typing in the future potentially, but the code that was in |
Sounds good. Slight concern that this might force us to never have init.py which is sometimes useful if we want to change internal structure without breaking public APIs or allow to easily expose things via public API that would otherwise require more drastic code structure changes. Also, how do we decide which module get to have init.py and which ones don't? I think we probably need some sort of guidelines on code-structure and packaging to sort this out. |
@owais I can see what you're saying about the benefits. On the other hand, the current I would also say that one of the points of OTel is for it to be a developer's No. 1 choice for everything tracing. That's why in the API package we let it be extended so that we can easily bring in community-implemented
I would say it would be for well-defined packages (like API and SDK uniquely are in the spec) which can support vendor specific features without including them in the packages themselves (like we do not want vendor specific features in the SDK and API packages). Because there is a use case to provide implementations of the API defined items (e.g. custom Google/Microsoft/Amazon/Splunk/Datadog |
Makes sense @NathanielRN. No objections from my side on removing it and making this technically possible but I still wonder if 3rd party extensions can just live in their own independent packages instead. For example, I don't see a strong reason for 3rd party packages to re-use opentelemetry import paths. This can still cause conflicts down the line as we as upstream cannot guarantee changing the directory structure in a way that breaks an unknown 3rd party package. We do use this pattern today to extend some packages such as opentelemetry.instrumentation but the key difference is that all those packages are maintained by us in this project. In other words, they are all "official" OpenTelemetry pacakges. We essentially ship these packages under the root I think only core and contrib packages should use the Also, other OpenTelemetry projects have clear distinction between official and 3rd party packages as overloading import paths like this is not as easy with other languages. So it'd be better if we stay consistent with other languages as well. I think @aabmass had the same concern in the last SIG call. |
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.
@NathanielRN can you open a separate issue for the discussion on where "extensions" should live, maybe summarize the pro/cons of each approach? We should document the guidelines on code-structure and packaging like Owais mentioned.
As for this PR, I agree removing the opentelemetry/sdk/__init__.py
file makes sense so I'm approving. I'd be interested if anyone remembers why its there.
Description
Sets up SDK package to be a Namespace Package.
A
.pyi
file (instead of a.py
file) allows packages to be registered and imported under theopentelemetry.sdk.*
namespace.This will allow vendors to ship packages under a
opentelemetry.sdk.extension
package, for example.Fixes #1204
Type of change
How Has This Been Tested?
and see that if you run
you can now start a python environment and install BOTH the regular SDK and the extension:
Checklist:
[] Changelogs have been updatedNot necessary?