-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Warn about time adjustment in tags #2052
Warn about time adjustment in tags #2052
Conversation
I spent a few hours trying to figure out how on earth can a span for 301 redirect follow can happen before the first request that triggered said redirect finished. The reason was that tracing with futures is hard and redirect following span outlived its parent, which triggered an adjustment. This adjustment may or may not be a good idea, but let's at least be clear it happened, so people can save some time in the future: |
6003a87
to
fa840cb
Compare
Codecov Report
@@ Coverage Diff @@
## master #2052 +/- ##
==========================================
- Coverage 98.21% 97.39% -0.82%
==========================================
Files 195 207 +12
Lines 9602 10307 +705
==========================================
+ Hits 9431 10039 +608
- Misses 134 224 +90
- Partials 37 44 +7
Continue to review full report at Codecov.
|
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.
I don't think we should use span.tags for that, but rather span.warnings, which we are already using for some messages about clock skew adjustment.
The URL doesn't seem to be necessary.
Please add a unit test to check for the expected outcome.
fa840cb
to
d563c91
Compare
Time adjustment is confusing to many people, as you can see in jaegertracing#961. This change adds a warning if we do any time adjustments, so that it's at least clear that adjustments happened. Signed-off-by: Ivan Babrou <github@ivan.computer>
d563c91
to
a8cb126
Compare
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!
Any traction on this? |
Time adjustment is confusing to many people, as you can see in #961.
This change adds a warning if we do any time adjustments, so that it's at least clear that adjustments happened.