-
Notifications
You must be signed in to change notification settings - Fork 566
breaking(transport): Make HTTP2Transport the default #4492
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## potel-base #4492 +/- ##
==============================================
+ Coverage 84.75% 84.85% +0.09%
==============================================
Files 158 158
Lines 16093 16093
Branches 2564 2564
==============================================
+ Hits 13640 13656 +16
+ Misses 1660 1648 -12
+ Partials 793 789 -4
|
|
The test failures look unrelated and consistent with each other. |
|
ty @BYK, will take over and fix the tests. will also make this a draft till we're ready to merge! |
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.
Thanks @BYK, this lgtm. Two things:
- please check out the Cursor comment about still setting the option via experiments in one of the tests, afait this is actually an oversight
- please add a note about this to the MIGRATION_GUIDE
I'll take some time this week to also add this to the migration guide in the docs, together with the async transport.
Making the HTTP2 transport the default when
h2andhttpcorepackages are installed. We've been testing this on Sentry SaaS for a while without any issues.We should promote installing the SDK as
sentry-sdk[http2]for this to be picked up though. Since we still have to support Python 3.7 andh2not being supported there, we cannot install it by default and use HTTP2 directly.EDIT: please merge after potel dogfooding