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 unnecessary dependencies #631

Merged
merged 7 commits into from
Sep 9, 2021
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Aug 26, 2021

Fixes #547

@ocelotl ocelotl self-assigned this Aug 26, 2021
@ocelotl ocelotl requested review from a team, owais and lzchen and removed request for a team August 26, 2021 19:54
@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 26, 2021
@lzchen
Copy link
Contributor

lzchen commented Aug 27, 2021

I think hhtpx can be removed as well.

@@ -41,7 +41,6 @@ install_requires =
opentelemetry-api ~= 1.3
opentelemetry-semantic-conventions == 0.24b0
opentelemetry-instrumentation == 0.24b0
opentelemetry-instrumentation-botocore == 0.24b0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this unnecessary or does botocore add some lower level spans that boto does not? Kind of like how flask and wsgi instrumentations work together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I'm not sure. Even if that is the case, do we want that to happen? It seems like a very problematic way of adding spans, just because some dependency is installed by an instrumentation package, imagine debugging that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is reasonable to do. Otel Node does it quite a bit IIRC. Debugging would be the same whether it is installed automatically or manually but with automatic install, we ship a much better default UX IMO.

@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 30, 2021

I think hhtpx can be removed as well.

Sorry, where is httpx a removable dependency?

@lzchen
Copy link
Contributor

lzchen commented Aug 30, 2021

@ocelotl
Click the link that I linked. wrapt doesn't need to be in httpx I believe.

@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 31, 2021

@ocelotl
Click the link that I linked. wrapt doesn't need to be in httpx I believe.

🤔 wrapt does need to be in httpx.

@lzchen
Copy link
Contributor

lzchen commented Aug 31, 2021

@ocelotl

wrapt does need to be in httpx.

If you take a look at that file, wrapt is not being used anywhere. Not sure how this was not caught in the linter.

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 1, 2021

@ocelotl

wrapt does need to be in httpx.

If you take a look at that file, wrapt is not being used anywhere. Not sure how this was not caught in the linter.

Ok, I see. I have removed wrapt from httpx 👍

I also submitted #646 to deal with unused imports in contrib.

@lzchen
Copy link
Contributor

lzchen commented Sep 7, 2021

@owais
Has your question been addressed?

@owais
Copy link
Contributor

owais commented Sep 7, 2021

Not a hard blocker for me but I'd like a better understanding of why boto has botocore as a dep. Does removing it mean users will have degraded experience if they just install boto instrumentation?

@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 9, 2021

Not a hard blocker for me but I'd like a better understanding of why boto has botocore as a dep. Does removing it mean users will have degraded experience if they just install boto instrumentation?

I think this is something to address in a different issue/PR. Even if that is the case, it definitely needs more than just a dependency, that kind of behavionr needs to be documented somewhere, having spans just showing up when another package is installed is not user friendly either.

@lzchen
Copy link
Contributor

lzchen commented Sep 9, 2021

Discussed in SIG, we are okay to merge this.

@lzchen lzchen merged commit 915acb1 into open-telemetry:main Sep 9, 2021
@ocelotl ocelotl deleted the issue_547 branch September 9, 2021 17:01
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.

opentelemetry-util-http requiring asgiref
4 participants