Improve package layout #37

reyang · 2019-06-27T21:39:09Z

Add the SDK package skeleton.
Instead using "opentelemetry.*" to discover namespace packages, use where="src" approach as demonstrated https://setuptools.readthedocs.io/en/latest/setuptools.html#find-namespace-packages.

The reason for introducing src folder:

Using wildcard approach would match tests/foo/bar/opentelemetry/ as well, which we want to avoid. The name filter in setuptools is implemented using fnmatchcase, which doesn't provide a good way to workaround this.
If we run setup.py under the opentelemetry-api folder, it will generate temporary folder structure build/lib/opentelemetry/, which will be accidentally included in the package.

reyang · 2019-06-27T21:41:44Z

opentelemetry-sdk/setup.py

+    description="OpenTelemetry Python SDK",
+    include_package_data=True,
+    long_description=open("README.rst").read(),
+    install_requires=[


opentelemetry-sdk will have dependency on opentelemetry-api, this will be a separate PR (we need to figure out how to install from local).

c24t

I was skeptical of src/, but this looks good and the setuptools docs confirm that this is the best way to do this.

You'll have to change the paths in tox.ini to get the tests to pass.

c24t · 2019-06-28T00:28:51Z

opentelemetry-sdk/src/opentelemetry/sdk/__init__.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from .version import __version__


Why do you need this?

I was thinking about troubleshooting scenario, where we want to see which version of the SDK customers are using, we do need to distinguish SDK versions versus API versions.
For example, if the queue for traces are full, I'd expect we log something like:

2019-06-27T22:36:59Z OpenTelemetry API loaded (version=1.0.0) 2019-06-27T22:37:15Z OpenTelemetry SDK loaded (version=1.0.3) 2019-06-27T22:38:30Z traces queue full, started to drop spans 2019-06-27T22:38:50Z traces queue recovered

I mean why do you need to import it here instead of in whatever module uses it (the trace queue in your example)?

I see, my intent was to save some typing, so we can import sdk.__version__ instead of sdk.version.__version__. Later we can probably rename version.py to package_info.py if we have more stuff to put here.

c24t · 2019-06-28T00:35:13Z

opentelemetry-sdk/setup.py

+
+package_info = {}
+with open(os.path.join(base_dir, "src", "opentelemetry", "sdk", "version.py")) as f:
+    exec(f.read(), package_info)


Why use package_info instead of read/exec-ing right into globals?

To prevent bad thing inside the version.py (later we might add more stuff such like SDK vendor info and rename the file to package_info.py) from polluting globals.

c24t · 2019-06-28T00:41:07Z

opentelemetry-sdk/src/opentelemetry/sdk/version.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+__version__ = "0.1.dev0"


Should you move the API version file out of internal/ to match?

Do you mean something like opentelemetry/api/version.py? We decided to put version.py in internal since there is no public interface. I'm open to suggestions.

I take #19 (review) to mean that we can't have a single top-level version file, but separate files under api/ and sdk/ should be fine.

reyang · 2019-06-28T01:04:20Z

tox.ini

@@ -8,5 +8,5 @@ deps =
  py37-mypy: mypy

 commands =
-  py37-lint: pylint opentelemetry-api/opentelemetry/
-  py37-mypy: mypy opentelemetry-api/opentelemetry/
+  py37-lint: pylint opentelemetry-api/src/opentelemetry/


@c24t do you think it's good idea to run lint on opentelemetry-api (which includes setup.py, tests, examples)?

Sure, if it ends up being easier to blacklist dirs that shouldn't be linted than to whitelist every one that should.

…y eventually

reyang · 2019-06-28T01:51:14Z

You'll have to change the paths in tox.ini to get the tests to pass.

I noticed that we got some trouble here. The reason why Travis + Tox work for us is because we have the file opentelemetry/__init__.py, while we shouldn't have it (since it is a namespace package).
If we want to fix this problem, we need to workaround this pylint bug.

For this PR, I reverted the __init__.py file back. Will create a separate issue to track it.

c24t · 2019-07-01T20:07:21Z

tox.ini

@@ -8,5 +8,5 @@ deps =
  py37-mypy: mypy

 commands =
-  py37-lint: pylint opentelemetry-api/opentelemetry/
-  py37-mypy: mypy opentelemetry-api/opentelemetry/
+  py37-lint: pylint opentelemetry-api/src/opentelemetry/


Sure, if it ends up being easier to blacklist dirs that shouldn't be linted than to whitelist every one that should.

c24t · 2019-07-01T20:09:01Z

opentelemetry-sdk/src/opentelemetry/sdk/__init__.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from .version import __version__


I mean why do you need to import it here instead of in whatever module uses it (the trace queue in your example)?

c24t · 2019-07-01T20:16:13Z

opentelemetry-sdk/src/opentelemetry/sdk/version.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+__version__ = "0.1.dev0"


I take #19 (review) to mean that we can't have a single top-level version file, but separate files under api/ and sdk/ should be fine.

improve package layout

20d02a4

reyang requested review from c24t, carlosalberto and Oberon00 as code owners June 27, 2019 21:39

reyang commented Jun 27, 2019

View reviewed changes

c24t reviewed Jun 28, 2019

View reviewed changes

update lint path

333b937

reyang commented Jun 28, 2019

View reviewed changes

workaround pylint first, will need to remove opentelemetry/__init__.p…

dd69d37

…y eventually

c24t approved these changes Jul 1, 2019

View reviewed changes

reyang merged commit 7e3f9a1 into master Jul 1, 2019

reyang deleted the layout branch July 1, 2019 21:31

c24t added a commit to c24t/opentelemetry-python that referenced this pull request Jul 2, 2019

Change sphinx conf path for open-telemetry#37

ccae3ba

c24t mentioned this pull request Jul 2, 2019

Update sphinx conf for package move #39

Merged

reyang mentioned this pull request Jul 3, 2019

Add "global tracer"/backend objects (#15) #29

Merged

reyang mentioned this pull request Jul 16, 2019

SDK namespace package #55

Merged

c24t mentioned this pull request Jul 17, 2019

Add single tox target for API and SDK tests #56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve package layout #37

Improve package layout #37

reyang commented Jun 27, 2019

reyang Jun 27, 2019

c24t left a comment

c24t Jun 28, 2019

reyang Jun 28, 2019

c24t Jul 1, 2019

reyang Jul 1, 2019

c24t Jun 28, 2019

reyang Jun 28, 2019 •

edited

Loading

c24t Jun 28, 2019

reyang Jun 28, 2019

c24t Jul 1, 2019

reyang Jun 28, 2019

c24t Jul 1, 2019

reyang commented Jun 28, 2019

c24t Jul 1, 2019

c24t Jul 1, 2019

c24t Jul 1, 2019

Improve package layout #37

Improve package layout #37

Conversation

reyang commented Jun 27, 2019

Choose a reason for hiding this comment

c24t 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

reyang Jun 28, 2019 • edited Loading

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

reyang commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Jun 28, 2019 •

edited

Loading