-
-
Notifications
You must be signed in to change notification settings - Fork 363
fix: Wrong flush timeout when called sequentially #5565
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
Fix an edge case leading to flush continuing to time out when it timed out once before the HTTP transport finished sending.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5565 +/- ##
=============================================
- Coverage 86.552% 86.129% -0.423%
=============================================
Files 417 417
Lines 35478 35442 -36
Branches 15342 15014 -328
=============================================
- Hits 30707 30526 -181
- Misses 4728 4875 +147
+ Partials 43 41 -2
... and 29 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Converting back to draft because the unit tests are failing. |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b714cb9 | 1224.71 ms | 1238.04 ms | 13.33 ms |
55f739c | 1226.06 ms | 1248.78 ms | 22.71 ms |
65f8d2e | 1221.15 ms | 1243.96 ms | 22.81 ms |
32e7197 | 1226.91 ms | 1245.48 ms | 18.56 ms |
d7461dc | 1233.69 ms | 1255.29 ms | 21.60 ms |
b0e13a7 | 1227.71 ms | 1245.88 ms | 18.16 ms |
f92cfa9 | 1228.45 ms | 1251.33 ms | 22.88 ms |
2609f7a | 1218.17 ms | 1241.34 ms | 23.17 ms |
e18d392 | 1228.69 ms | 1244.43 ms | 15.73 ms |
e64d3d4 | 1241.90 ms | 1260.10 ms | 18.20 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b714cb9 | 23.75 KiB | 858.69 KiB | 834.93 KiB |
55f739c | 23.75 KiB | 858.73 KiB | 834.98 KiB |
65f8d2e | 23.74 KiB | 872.67 KiB | 848.93 KiB |
32e7197 | 23.75 KiB | 866.69 KiB | 842.94 KiB |
d7461dc | 23.75 KiB | 874.45 KiB | 850.70 KiB |
b0e13a7 | 23.75 KiB | 860.98 KiB | 837.23 KiB |
f92cfa9 | 23.75 KiB | 855.38 KiB | 831.62 KiB |
2609f7a | 23.75 KiB | 867.04 KiB | 843.29 KiB |
e18d392 | 23.75 KiB | 866.68 KiB | 842.93 KiB |
e64d3d4 | 23.75 KiB | 855.37 KiB | 831.62 KiB |
Previous results on branch: fix/flushing-request-times-out
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
54cc390 | 1229.28 ms | 1247.92 ms | 18.64 ms |
3965892 | 1229.72 ms | 1249.86 ms | 20.13 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
54cc390 | 23.75 KiB | 874.30 KiB | 850.55 KiB |
3965892 | 23.75 KiB | 874.30 KiB | 850.55 KiB |
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.
LGTM with small feedback items.
📜 Description
Fix an edge case leading to flush continuing to time out when it timed out once before the HTTP transport finished sending.
💡 Motivation and Context
Came up while investigating #5421
💚 How did you test it?
Unit tests.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.