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

No real nanosecond resolution provided by time_ns #1594

Closed
ocelotl opened this issue Feb 10, 2021 · 4 comments · Fixed by #1602
Closed

No real nanosecond resolution provided by time_ns #1594

ocelotl opened this issue Feb 10, 2021 · 4 comments · Fixed by #1602
Assignees
Labels
bug Something isn't working

Comments

@ocelotl
Copy link
Contributor

ocelotl commented Feb 10, 2021

Right now, we provide time_ns as a function that should return a timestamp with nanosecond precision:

try:
    time_ns = time.time_ns
# Python versions < 3.7
except AttributeError:

    def time_ns() -> int:
        return int(time.time() * 1e9)

Nevertheless, PEP 0564 explicitly compares time.time_ns against time.time and shows that they don't have the same resolution, the former being much better.

I think the purpose of the code above is to provide an uniform path where users can get a function that provides a timestamp with nanosecond resoultion regardless of the interpreter being run but it is misleading, we are not providing the same resolution for all Python versions.

I understand that the spec has precision requirements for timestamps but I don't think it is in the scope of OpenTelemetry to concern itself with providing this kind of functionality. Also, it is problematic to have a function named time_ns that can make an user believe the time is actually being measured with nanosecond precision when this is not the case.

I suggest we eliminate this function completely, it is out of the scope of OpenTelemetry, puts us in a difficult situation by offering it as part of our public API and it is something else that we have to maintain for the future.

@ocelotl ocelotl added the bug Something isn't working label Feb 10, 2021
@codeboten
Copy link
Contributor

codeboten commented Feb 11, 2021

Rename time_ns to timestamp that returns the max precision for the version of python we're using may be the better solution than estimating ns, I would be ok with this approach.

@codeboten
Copy link
Contributor

This should also have some clear documentation here.

@srikanthccv
Copy link
Member

@ocelotl IIRC you suggested using milliseconds for versions < 3.7 and nanos for >= 3.7 python versions. It could be possible that my span operations duration in ranges of tens of nanoseconds, then, wouldn't that be an issue for 3.5 and 3.6 versions? A quick look at the exporters reveals that they require nanoseconds timestamp for most of the transport mechanisms and micros in few formats. What is the major concern here is it the precision resolution or the time_ns shouldn't be part of public API? If precision resolution, What are the alternatives you are thinking of? I also wonder what is the case of other SIGs?

@xkortex
Copy link

xkortex commented Feb 15, 2021

Throwing in my unsolicited $0.02 into the ring, having been recently bit by something like this.

Playing devil's advocate a bit here, can you actually trust sub-microsecond timestamps on python <3.7 to actually be meaningfully comparable in terms of causality? It's more of an open question than a rhetorical, really, but my intuition is "not if you really depend on it for strict ordering". For that you really need a logical clock (e.g. Lamport clock). Also I believe on Windows you need to do some funky stuff to get better than 16ms resolution (not sure how much cross-platform you good folks are aiming for but I figure it's worth mentioning).

I suggest we eliminate this function completely, it is out of the scope of OpenTelemetry, puts us in a difficult situation by offering it as part of our public API and it is something else that we have to maintain for the future.

+1. Time is really thorny. Lots of potential footguns. Personally, I would aim for a "best effort" time resolution and a big "your mileage may vary" caveat, since some consumers may rely on the implicit, incorrect assumption that timestamp1 < timestamp2 implies event1 actually occurred before event2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants