span: use ns time directly rather than converting #964
Conversation
|
Wait, actually, I thought it was smart to do this but… it somehow breaks the API. I mean users can pass their own start/stop time and so far we expected this to be in seconds. I don't think a lot of people do that, but if we now expect it to be nanoseconds, it'll break their spans. Damn. Should we just give up on this? |
|
oh, right... yeah, probably need to not do this for now. |
|
Ok 😢 I'll rework it so it does not break the API. |
jd
force-pushed
the
use-time-ns
branch
3 times, most recently
from
a2324cc
to
3eaba5b
Compare
|
This works and slightly breaks less the API: you still pass seconds as arguments, however the WDYT? |
There was a problem hiding this comment.
this seems fine to me, wonder what can break from making this change for users.
@palazzem mind giving this a quick once over?
ddtrace/span.py
Outdated
| @@ -83,7 +82,7 @@ def __init__( | |||
| self.metrics = {} | |||
|
|
|||
| # timing | |||
| self.start = start or time.time() | |||
| self.start = time_ns() if start is None else (start * 1e9) | |||
There was a problem hiding this comment.
I wonder if we can handle this gracefully, like (start * 1e9) if not is_already_ns(start) else start.
This comment adds a little unnecessary complexity here, just pondering out loud.
There was a problem hiding this comment.
Yeah, though there's no way to be sure it's already ns rather than guessing than a big number might need ns.
Another approach I'm just thinking about is that we could keep seconds but use float instead of int so if somebody passes a float it can pass nanoseconds and we can use nanoseconds by default too.
|
@jd is this a PR we want to continue working on? |
|
@brettlangdon yes, it should be mergeable now. It improves things without breaking the API. We can break the API later if we think it's worth it. |
|
The only thing I think that could cause an issue here is if someone is using |
|
You're right. Is it worth possibly breaking this? Considering we're in |
|
Or we hold for 1.0.0 and break then. |
|
We are 0.x but we still want to be stable. |
This improves span precision. Note that time_ns is only available on Python 3.
|
This should be ready to be merged. It does not break the API anymore and make sure we use nanosecond precision when possible. |
This improves span precision. Note that time_ns is only available on Python 3.