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

Make necessary metrics API symbols private #2626

Closed
wants to merge 2 commits into from

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 20, 2022

Fixes #2622

Note for reviewers

This PR is big but it is mostly just renaming public modules to make them private. For example, if there is a/b.py, this PR adds a/_b.py, then it adds a blank a/b.py and imports in a/b.py all the public symbols that are in a/_b.py.

The idea behind this is to make public only the intended public symbols, here is an example:

# a/_b.py

from re import match


def function(a, b):
    return match(a, b)
# a/b.py
from a._b import function

__all__ = ["function"]

In this way we don't expose re.match as part of our public API, because to import that the user would have to do this: from a._b import match.

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 20, 2022
@ocelotl ocelotl self-assigned this Apr 20, 2022
@ocelotl ocelotl requested a review from a team April 20, 2022 16:35
@ocelotl ocelotl marked this pull request as draft April 20, 2022 20:31
@ocelotl ocelotl marked this pull request as ready for review April 20, 2022 23:21
@aabmass
Copy link
Member

aabmass commented Apr 21, 2022

@ocelotl I just thought of something. Any idea how this effects the docs? Do they show the same structure as before?

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 21, 2022

@ocelotl I just thought of something. Any idea how this effects the docs? Do they show the same structure as before?

It makes a difference in the displayed paths:

This is how the documentation looks in main:

main

This is how the documentation looks in this PR:

pr

Notice that in the second image there is an underscore before instrument in the Instrument class path.

I tried fixing this issue as it says here by making this change:

diff --git a/opentelemetry-api/src/opentelemetry/_metrics/_instrument.py b/opentelemetry-api/src/opentelemetry/_metrics/_instrument.py
index ec8161a7a..1e881a1c7 100644
--- a/opentelemetry-api/src/opentelemetry/_metrics/_instrument.py
+++ b/opentelemetry-api/src/opentelemetry/_metrics/_instrument.py
@@ -54,6 +54,9 @@ class Instrument(ABC):
         # FIXME check that the unit contains only ASCII characters
 
 
+Instrument.__module__ = "opentelemetry._metrics.instrument"
+
+
 class _ProxyInstrument(ABC, Generic[InstrumentT]):
     def __init__(self, name, unit, description) -> None:
         self._name = name

Nevertheless, I go this error when running tox -e docs:

sphinx.errors.SphinxWarning: /home/ocelotl/github/ocelotl/opentelemetry-python/.tox/docs/lib/python3.9/site-packages/opentelemetry/_metrics/___init__.py:docstring of opentelemetry._metrics.___init__:4:py:class reference target not found: Instrument

I am ok with this PR as it is now, I don't see this as being too critical. We can add a workaround in a separate PR, the most important part of this PR is that we don't have unnecessary symbols that are exposed in the public API.

@lzchen
Copy link
Contributor

lzchen commented Apr 27, 2022

Did the original issue require us to simply add a _ in front of the module names? Were we not making the classes themselves private?

@aabmass
Copy link
Member

aabmass commented Apr 28, 2022

@ocelotl I gave this a shot with the other approach I mentioned and got the docs working after a lot of pain #2651

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 28, 2022

@ocelotl I gave this a shot with the other approach I mentioned and got the docs working after a lot of pain #2651

Yes! excellent, @aabmass, reviewing 💪

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 29, 2022

Closing in favor of #2651

@ocelotl ocelotl closed this Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make necessary API metrics symbols private
3 participants