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

Client should handle system clock changes better #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wiedi
Copy link

@wiedi wiedi commented Feb 13, 2020

This should fix #77

@andrewhsu
Copy link

@ocelotl if you can take a look at this one

@ocelotl
Copy link
Contributor

ocelotl commented Apr 26, 2020

I'm not sure this should be the solution (see below). TLDR: we can use it as it is but at least we should add a warning.

Surely, this will avoid the exception from being raised but the OpenTracing API seems to permit having negative span durations. The duration of a span is the difference between its start_time and its finish_time. Since both start_time and finish_time can be passed down as arguments, this means that whoever calls these methods could use a combination that causes span.duration to be negative if that is desired. Even when I have no practical reason on who may want this to happen, this certainly could.

This does not seem to be the place to check for negative durations, the place where span.duration is set seems more likely. I have tried the code in util._time_to_micros and it seems to handle negative values well, so the exception reported in #77 should be happening in the call to Span (which is in lightstep.collector_pb2.py which is not Python code currently). So, since the problem can't be tracked down more, I would suggest to at least raise a warning, since the most likely situation is the one reported in #77, and probably the end user would at least like to know this is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client doesn't account for adjustments to system clock.
3 participants