Remove time_ns from API #1602

ocelotl · 2021-02-12T05:00:14Z

This PR depends on #1599, I have marked this PR as a draft until #1599 gets merged to avoid trouble when reviewing.

Description

This removes time_ns from our API. We were providing this only to support Python versions older than 3.7 that did not include time_ns as a function of time. The problem with the current approach is that it provides a function that does not have the resolution that the new time_ns provides, it is misleading for our users to have a function named time_ns that does not provide actual nanosecond resolution. Having this function in our API is problematic, having a function that returns the time in nanoseconds is not part of the spec and makes us have to support it in future versions.

The solution here artificially adds a function named time_ns to time for Python versions older than 3.7. With this approach, our users can always do from time import time_ns and when we decide not to support Python 3.5 (which has reached end of life already) or Python 3.6 (which will reach end of life by the end of this year) then we can simply remove this solution. This will be completely transparent to our users because they will never import any time_ns function from an opentelemetry package.

Fixes #1594

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/
The method interfaces of opentelemetry-instrumentation/ have changed
The method interfaces of test/util have changed
Scripts in scripts/ that were copied over to the Contrib repo have changed
Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
- pyproject.toml
- isort.cfg
- .flake8
When a new .github/CODEOWNER is added
Major changes to project information, such as in:
- README.md
- CONTRIBUTING.md
Yes. - Link to PR: Sync with Remove time_ns from API opentelemetry-python-contrib#342
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

ocelotl · 2021-02-18T22:01:29Z

Right now we do this:

We provide a time_ns function. This is convenient for our users since they can use it to create timestamps regardless of the Python version they are using.
This time_ns function is part of the API. This is inconvenient for us because it only exists to fulfill the needs of Python<3.7 users, forces us to keep it in the long term and it has nothing to do with OpenTelemetry.
We do not tell our Python<3.6 users that the function they are using does not have the same resolution as the function in Python>3.7. This is inconvenient for our users because they can be mislead into thinking time_ns provides actual nanosecond resolution.

Now, this PR as it is right now "satisfies" the 3 points mentioned above:

It still provides a time_ns function so that our users can have a function that creates timestamps regardless of the Python version that they are using.
The time_ns function we provide in this PR is not part of the public API.
We let our users know that the function they are using does not have the right resolution.

I believe this PR to be a bit controversial since it uses a kind of a "hack" to fix this problem, it uses a sitecustomize.py file to add time_ns to time for Python versions previous than 3.6. I understand that others may prefer a less hackyish solution.

Of the 3 points mentioned before, I care mostly about 2 and 3. In the past SIG it was suggested another possible solution, to not provide a time_ns function at all in the public API, instead make it private and use it in our packages while raising the warning for users of Python<3.7. I am also ok with this solution because while it does not "satisfy" point 1 it "satisfies" points 2 and 3 which I consider the most important and it is not a controversial solution that relies on a "hack".

@lzchen, @codeboten, @aabmass and anyone who may have an opinion, please leave it in the comments below. If more prefer the more traditional approach of just making time_ns private and raise a warning I can reimplement this PR to do so.

owais · 2021-02-19T11:58:43Z

In the past SIG it was suggested another possible solution, to not provide a time_ns function at all in the public API, instead make it private and use it in our packages

This makes a lot of sense to me. We can just mark it as internal so we don't cover it under any API stability guarantees. I think this should be good enough for us to use the function internally. Anyone else deciding to use it would take on an intentional risk. Going out of the way with the sitecustomize trick isn't worth it IMHO.

srikanthccv

+1 for making time_ns as private instead of patching the time module.

lzchen · 2021-02-19T16:22:49Z

In the past SIG it was suggested another possible solution, to not provide a time_ns function at all in the public API, instead make it private and use it in our packages

+1

toumorokoshi

+1 to the other reviewers concerns about patching in this fashion. Definitely seems easy to make this a private function.

codeboten

Just looking through the change, there's a lot of changes that don't appear to be directly tied to removing time_ns from the API, is it possible to refactor this PR to only that code?

opentelemetry-api/setup.py

toumorokoshi

Thanks the changes look good, but I think the warning could be more succinct.

toumorokoshi · 2021-02-24T07:13:41Z

opentelemetry-api/src/opentelemetry/util/_time.py

+from logging import getLogger
+from sys import version_info
+
+if version_info.minor < 7:


IMO a safer way to do this is:

try: from time import time_ns except ImportError: def _time_ns....

This avoids anything funky with backported functions and the like.

Sorry, how can this avoid anything funky with backported functions? I prefer the if approach because it is not-exceptional for our users to use 3.6 or 3.5 so it should not be treated as an exception.

Sorry, how can this avoid anything funky with backported functions?

In the past, people have backported python features back to older versions of python. It's common for them to use the same module name, or modify an existing module to include the functionality they backported.

There's no guarantee that the version of python doesn't have some version of the function backported. Hence I was saying that it's more common to just check to see if the import fails, and if it doesn't, then import the function.

Ok, but we are not adding anything to any existing module now, I mean, we no longer try to patch time. _time_ns is always an attribute of opentelemetry.util regardless of the Python version used, only its implementation changes.

I'm more concerned about anyone patching the module. In the examples above, someone else could add time_ns, and if it exists, it would be nice to do so.

But this isn't a blocker comment, practically I don't think this will make much of a difference.

@toumorokoshi
Is this resolved?

toumorokoshi · 2021-02-24T07:14:54Z

opentelemetry-api/src/opentelemetry/util/_time.py

+
+if version_info.minor < 7:
+    getLogger(__name__).warning(  # pylint: disable=logging-not-lazy
+        "You are using Python version 3.%s. This version does not include a "


I think this is more verbose that it needs to be. It's a good code comment for maintainers, but for users it suffices to say "in this version of python, opentelemetry only supports millisecond-level granularity".

In addition, why give the context that this private function will be removed once opentelemetry-api doesn't support python 3.7? I feel like that's somewhat obvious, and also unnecessary information to the user since 3.7+ supports NS granularity.

Yes, you are right. I preferred to err on the safe side and be too verbose to make sure that our users never try using _time_ns. Refactoring this.

toumorokoshi · 2021-02-24T07:17:05Z

opentelemetry-api/src/opentelemetry/util/_time.py

+
+if version_info.minor < 7:
+    getLogger(__name__).warning(  # pylint: disable=logging-not-lazy
+        "You are using Python version 3.%s. This version does not include a "


In addition, why give the context that this private function will be removed once opentelemetry-api doesn't support python 3.7? I feel like that's somewhat obvious, and also unnecessary information to the user since 3.7+ supports NS granularity.

Fixes open-telemetry#1598 This refactors utils to make them private the most as possible

ocelotl force-pushed the issue_1594 branch from d08edb6 to ad0e72c Compare February 12, 2021 05:11

ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 12, 2021

ocelotl force-pushed the issue_1594 branch 5 times, most recently from 1de57e3 to f3a3278 Compare February 18, 2021 04:41

ocelotl changed the title ~~Issue 1594~~ Remove time_ns from API Feb 18, 2021

ocelotl marked this pull request as ready for review February 18, 2021 05:35

ocelotl requested review from a team, toumorokoshi and hectorhdzg and removed request for a team February 18, 2021 05:35

ocelotl mentioned this pull request Feb 18, 2021

Sync with Remove time_ns from API open-telemetry/opentelemetry-python-contrib#342

Merged

11 tasks

srikanthccv reviewed Feb 19, 2021

View reviewed changes

lzchen mentioned this pull request Feb 19, 2021

Refactor utils #1599

Merged

11 tasks

toumorokoshi suggested changes Feb 22, 2021

View reviewed changes

ocelotl self-assigned this Feb 22, 2021

ocelotl force-pushed the issue_1594 branch 3 times, most recently from 2f13261 to 8e0c8ed Compare February 22, 2021 23:20

codeboten reviewed Feb 23, 2021

View reviewed changes

opentelemetry-api/setup.py Outdated Show resolved Hide resolved

ocelotl requested a review from toumorokoshi February 23, 2021 23:26

toumorokoshi suggested changes Feb 24, 2021

View reviewed changes

ocelotl force-pushed the issue_1594 branch from c84ec7c to 6679805 Compare February 24, 2021 16:47

ocelotl requested a review from toumorokoshi February 24, 2021 16:59

ocelotl added 22 commits March 9, 2021 10:54

Refactor utils

260efe5

Fixes open-telemetry#1598 This refactors utils to make them private the most as possible

Remove time_ns

aff5743

Add injector

7076277

Add sitecustomize when installing

44f2d51

Fix

fd4e645

Fix SHA

bf06c3e

Update warning

1ef61aa

Update SHA

91c562e

Add FIXME

c4b0721

Fix lint

e54c00a

Fix lint

5f8e4b6

Fix lint

34b2530

Fix yml

0cc73d8

Refactor to make time_ns private

ec44554

Work around lint

d03b81d

Revert setup.py changes

9025860

Refactor warning message

a9e0d23

Update SHA

c7a2f9c

Make warning friendlier

062ef75

Update SHA

93549bb

Fix warning

e8a1c52

Use precision instead of granularity

da8e95a

ocelotl force-pushed the issue_1594 branch from f734148 to da8e95a Compare March 9, 2021 16:57

Add CHANGELOG entry

709e74a

ocelotl requested a review from codeboten March 9, 2021 17:00

Update SHA

d3694fc

codeboten approved these changes Mar 10, 2021

View reviewed changes

alrex added 2 commits March 9, 2021 17:27

Merge branch 'main' into issue_1594

42ea461

Update CHANGELOG.md

67d521f

codeboten merged commit 5cc7ec5 into open-telemetry:main Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove time_ns from API #1602

Remove time_ns from API #1602

ocelotl commented Feb 12, 2021 •

edited

Loading

ocelotl commented Feb 18, 2021

owais commented Feb 19, 2021 •

edited

Loading

srikanthccv left a comment

lzchen commented Feb 19, 2021

toumorokoshi left a comment

codeboten left a comment

toumorokoshi left a comment

toumorokoshi Feb 24, 2021

ocelotl Feb 24, 2021

toumorokoshi Mar 2, 2021

ocelotl Mar 2, 2021

toumorokoshi Mar 5, 2021

lzchen Mar 9, 2021

toumorokoshi Feb 24, 2021

toumorokoshi Feb 24, 2021

ocelotl Feb 24, 2021

toumorokoshi Feb 24, 2021

Remove time_ns from API #1602

Remove time_ns from API #1602

Conversation

ocelotl commented Feb 12, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

ocelotl commented Feb 18, 2021

owais commented Feb 19, 2021 • edited Loading

srikanthccv left a comment

Choose a reason for hiding this comment

lzchen commented Feb 19, 2021

toumorokoshi left a comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocelotl commented Feb 12, 2021 •

edited

Loading

owais commented Feb 19, 2021 •

edited

Loading