-
-
Notifications
You must be signed in to change notification settings - Fork 772
fix: negative trace substraction when using SetTimeout #1038
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
jeevatkm
left a comment
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.
@nicolasbeauvais Thanks for the PR. I'm sorry for the delayed response, I was occupied with my personal stuff.
I added a one review comment.
|
Also, the same issue might exist in the v2 too. I will backport it afterwards, or you can also submit PR v2 if you're interested. |
|
Hey @jeevatkm, no worries, I hope you're doing good! I've set the default value directly at object initialization, let me know if that's ok for you. I can make a PR on the v2 branch once this one is approved. |
jeevatkm
left a comment
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.
@nicolasbeauvais Thanks for updating PR.
|
@nicolasbeauvais It seems this test case having an issue |
@jeevatkm Indeed, it seems the new test uncovered a race condition. I fixed it (at least locally), but please check the changes thoroughly I'm not an experienced Go dev ✌️ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #1038 +/- ##
==========================================
- Coverage 99.82% 99.79% -0.03%
==========================================
Files 18 18
Lines 3919 3398 -521
==========================================
- Hits 3912 3391 -521
+ Misses 5 4 -1
- Partials 2 3 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@nicolasbeauvais Thanks for fixing the test case. It is passed now, only missing coverage on the modified code. Do you mind checking it? |
jeevatkm
left a comment
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.
@nicolasbeauvais Thanks for updating the PR.
|
Still, patch coverage is failing because of missing coverage on the same line as before. I will merge this PR. Can you please look into this coverage part later on? |
In certain cases when using client or request SetTimeout it is possible that some trace value like
dnsDone,tlsHandshakeDoneorgotConnare zero, resulting in inaccurate results:This PR add additional checks in the
TraceInfomethod to avoid negative values.I'm not sure how to improve the tests for this, as aborting requests with SetTimeout will not always accurately stop the execution at the exact same point. I'm open to ideas.