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

Re-export public symbols in opentelemetry.context #3332

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

ngnpope
Copy link
Contributor

@ngnpope ngnpope commented Jun 3, 2023

Description

Avoids issues with typing where mypy complains because Context is not re-exported when using the following import:

from opentelemetry.context import Context

It's possible to work around this with the following, but it seems as though this is not intended:

from opentelemetry.context.context import Context

Type of change

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

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project

@ngnpope ngnpope requested a review from a team as a code owner June 3, 2023 15:51
@ngnpope ngnpope force-pushed the patch-2 branch 2 times, most recently from b07a1c3 to 9b9aee3 Compare June 12, 2023 07:56
@ngnpope
Copy link
Contributor Author

ngnpope commented Jun 13, 2023

@srikanthccv This should also be straightforward - it was one of the typing-related issues I encountered along with the changes in #3285.

@srikanthccv
Copy link
Member

It's possible to work around this with the following, but it seems as though this is not intended:

There is already __all__ = ["Context"] in opentelemetry.context.context for this. I think that is intended.

@aabmass
Copy link
Member

aabmass commented Jun 22, 2023

@srikanthccv this seems to be pretty widely used in the wild, I'm on board with this PR.

Avoids issues with typing where `mypy` complains because `Context` is
not re-exported when using the following import:

```python
from opentelemetry.context import Context
```

It's possible to work around this with the following, but it seems as
though this is not intended:

```python
from opentelemetry.context.context import Context
```
@ngnpope ngnpope changed the title Re-export Context Re-export public symbols in opentelemetry.context Jun 22, 2023
@ngnpope
Copy link
Contributor Author

ngnpope commented Jun 22, 2023

Thanks @aabmass. I've updated this to (re-)export all public symbols as requested.

The only one I left out was opentelemetry.environment_variables.OTEL_PYTHON_CONTEXT which didn't seem as though it ought to be re-exported and there doesn't seem to be a precedent for that elsewhere.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the PR! @srikanthccv does it look OK to you?

@srikanthccv
Copy link
Member

srikanthccv commented Jun 24, 2023

I am fine with this. These transitive imports scare me because you never know how people are using them, and it was an overlooked aspect from our side. Not until metrics SDK we started using private internals and __all__ with specific symbols, so we need to be very cautious about refactoring anything with tracing components.

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 5, 2023
@ocelotl ocelotl enabled auto-merge (squash) July 5, 2023 10:38
@ocelotl ocelotl merged commit 84357bf into open-telemetry:main Jul 5, 2023
111 checks passed
@ngnpope ngnpope deleted the patch-2 branch July 5, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants