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

Split auto instrument & bootstrap into new package #1415

Closed

Conversation

NathanielRN
Copy link
Contributor

Description

As a follow up to #1306 where we made Auto Instrumentation configurable (which was important because once the Tracer Provider is configured you never can configure it again!) we took a dependency on opentelemtry-sdk.

This is okay for the auto instrumentation alone, but the actual instrumentation packages should not be forced to depend on the SDK as mentioned in #1252.

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

Unit tests are all passing.

Checklist:

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

@NathanielRN NathanielRN requested review from a team, owais and aabmass and removed request for a team November 25, 2020 04:04
@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch from 7142271 to 5499d30 Compare November 25, 2020 04:05
@NathanielRN
Copy link
Contributor Author

@NathanielRN
Copy link
Contributor Author

NathanielRN commented Nov 25, 2020

EDITED - (RESOLVED)

  1. I'm not sure how to edit the opentelemetry-instrument executable, I'm not sure how it got onto my machine and if it was when I downloaded the original package using pip install opentelemetry-instrumentation...

Thanks to @ocelotl I found out that the scripts are generated through the setuptools tool. I have this configured in opentelemetry-auto-instrumentation/setup.py which allows me to verify that 1) pip install opentelemetry-instrumentation does NOT install the script and 2) pip install opentelemetry-auto-insturmentation DOES install the script.

See below:

 enowell  ~/git/opentelemetry-python   move-auto-instrumentation  which opentelemetry-instrument
opentelemetry-instrument not found
 ✘ enowell  ~/git/opentelemetry-python   move-auto-instrumentation  pip install ~/git/opentelemetry-python/opentelemetry-instrumentation
Defaulting to user installation because normal site-packages is not writeable
Processing ./opentelemetry-instrumentation
Requirement already satisfied: opentelemetry-api==0.16.dev0 in /Users/enowell/Library/Python/3.8/lib/python/site-packages (from opentelemetry-instrumentation==0.16.dev0) (0.16.dev0)
Requirement already satisfied: wrapt<2.0.0,>=1.0.0 in /Users/enowell/Library/Python/3.8/lib/python/site-packages (from opentelemetry-instrumentation==0.16.dev0) (1.12.1)
Building wheels for collected packages: opentelemetry-instrumentation
  Building wheel for opentelemetry-instrumentation (setup.py) ... done
  Created wheel for opentelemetry-instrumentation: filename=opentelemetry_instrumentation-0.16.dev0-py3-none-any.whl size=6562 sha256=11bee1ff49cd4db1bc1c71a18ce48f8664551f20f55c2777e2504bcdf246f60b
  Stored in directory: /Users/enowell/Library/Caches/pip/wheels/f4/42/7d/3d835fd3752680cd34e087a1c5906712304497099bff446f9b
Successfully built opentelemetry-instrumentation
Installing collected packages: opentelemetry-instrumentation
  Attempting uninstall: opentelemetry-instrumentation
    Found existing installation: opentelemetry-instrumentation 0.13b0
    Uninstalling opentelemetry-instrumentation-0.13b0:
      Successfully uninstalled opentelemetry-instrumentation-0.13b0
Successfully installed opentelemetry-instrumentation-0.16.dev0
WARNING: You are using pip version 20.2.3; however, version 20.2.4 is available.
You should consider upgrading via the '/Library/Developer/CommandLineTools/usr/bin/python3 -m pip install --upgrade pip' command.
 enowell  ~/git/opentelemetry-python   move-auto-instrumentation  which opentelemetry-instrument
opentelemetry-instrument not found
 ✘ enowell  ~/git/opentelemetry-python   move-auto-instrumentation  pip install ~/git/opentelemetry-python/opentelemetry-auto-instrumentation
Defaulting to user installation because normal site-packages is not writeable
Processing ./opentelemetry-auto-instrumentation
Requirement already satisfied: opentelemetry-api==0.16.dev0 in /Users/enowell/Library/Python/3.8/lib/python/site-packages (from opentelemetry-auto-instrumentation==0.16.dev0) (0.16.dev0)
Requirement already satisfied: opentelemetry-sdk==0.16.dev0 in /Users/enowell/Library/Python/3.8/lib/python/site-packages (from opentelemetry-auto-instrumentation==0.16.dev0) (0.16.dev0)
Building wheels for collected packages: opentelemetry-auto-instrumentation
  Building wheel for opentelemetry-auto-instrumentation (setup.py) ... done
  Created wheel for opentelemetry-auto-instrumentation: filename=opentelemetry_auto_instrumentation-0.16.dev0-py3-none-any.whl size=11560 sha256=231f1c696edd2a1f645e525bcc50061f3d146393ff77dccc02efcf166bd53f57
  Stored in directory: /Users/enowell/Library/Caches/pip/wheels/b6/08/17/98b9d7c43c6ea91184a03f4c7ef786a3087302ffc308051d3a
Successfully built opentelemetry-auto-instrumentation
Installing collected packages: opentelemetry-auto-instrumentation
  Attempting uninstall: opentelemetry-auto-instrumentation
    Found existing installation: opentelemetry-auto-instrumentation 0.16.dev0
    Uninstalling opentelemetry-auto-instrumentation-0.16.dev0:
      Successfully uninstalled opentelemetry-auto-instrumentation-0.16.dev0
Successfully installed opentelemetry-auto-instrumentation-0.16.dev0
WARNING: You are using pip version 20.2.3; however, version 20.2.4 is available.
You should consider upgrading via the '/Library/Developer/CommandLineTools/usr/bin/python3 -m pip install --upgrade pip' command.
 enowell  ~/git/opentelemetry-python   move-auto-instrumentation  which opentelemetry-instrument
/Users/enowell/Library/Python/3.8/bin/opentelemetry-instrument

This is done through the the console scripts entry point as outlined by setuptools.

  1. I also want to make sure we get the opentelemetry-bootstrap executable!

Same behavior as above.

Some help on that would really be appreciated!

NOTE: I do think if people had opentelemetry-instrument installed before they would have to manually delete it so that installing this package could replace it!

@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch from 5499d30 to e1b6509 Compare November 25, 2020 04:17
@@ -68,6 +72,7 @@ changedir =
test-core-api: opentelemetry-api/tests
test-core-sdk: opentelemetry-sdk/tests
test-core-proto: opentelemetry-proto/tests
test-core-auto_instrumentation: opentelemetry-auto-instrumentation/tests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why, but when I do test-core-auto-instrumentation it doesn't find any tests at all... I had to add an underscore _ for it to find the tests. Some matching issue I guess?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't expect the - vs _ to make a difference here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I can't explain why it doesn't work with the - but I can confirm that it does not :/ It was the difference maker here for some reason.

Even test-coree-auto-instrumentation worked just fine. But specifically test-core-auto-instrumentation cause tox to not find any tests.

How do you want to proceed? Is this a blocker?

@owais
Copy link
Contributor

owais commented Nov 25, 2020

Thanks. I've wanted to do something similar. I think the bootstrap command should be part of this package as well. Don't see a reason to keep the two commands in different packages. If we move bootstrap, opentelemetry-instrumentation will contain just shared utilities and machinery that make it easier to write an instrumentation meaning end users will never have to worry about it and the experience would look something like:

pip install openetelemtry-auto-instrumentation
opentelemetry-bootstrap -a=install
opentelemetry-instrument ./main.py

@owais
Copy link
Contributor

owais commented Nov 25, 2020

At this point, we could even see this new package as the official OpenTelemetry Python distribution and actually call it just opentelemetry. Even when users don't want either of the commands this package would ship, it could still act as a meta-package that installs everything else required for them to get started with manual instrumentation.

People who want a custom SDK would install individual components they need to use with their own SDK.

This in a way looks very similar to the following wrappers or distributions:

https://github.com/lightstep/otel-launcher-python
https://github.com/signalfx/splunk-otel-python

@NathanielRN
Copy link
Contributor Author

@owais Thats a really cool thought! I can definitely relate to wanting to just run a pip install opentelemetry command and get started with opentelemetry right away.

@codeboten and @lzchen what do you think of this? I can change it to be as so!

@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch from e1b6509 to d590521 Compare November 25, 2020 08:31
@NathanielRN NathanielRN changed the title Split auto instrumentation into a new package Split auto instrument & bootstrap into new package Nov 25, 2020
@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch 2 times, most recently from d9f44d3 to 89cbbf1 Compare November 25, 2020 08:34
@NathanielRN
Copy link
Contributor Author

@owais I've moved bootstrap into opentelemetry-auto-instrumentation.

The more I think about it the more I really like the idea of having this be the official opentelemetry package so I would vote for renaming it to be that! Looking forward to hearing more opinions on that.

Even on pypi it would be known as the opentelemetry package.

@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch 4 times, most recently from 19aa2a7 to e03cb5f Compare November 25, 2020 08:41
@lzchen
Copy link
Contributor

lzchen commented Nov 25, 2020

@owais
Although I do agree that this functionality is very useful and cool, there is nothing "official" about something if it is not properly defined in the specs. I'm not sure that we can offer this to customers as a feature to be marked that OT is officially supporting, especially if other languages are not doing the same. The fact that it is in a separate package makes it acceptable as a "plugin" package. I think also we should think about, what would be the use case for users to install each of these packages individually if they existed? opentelemetry-api by itself makes sense, users who want to use their own sdk. opentelemetry-sdk + opentelemetry-api makes sense, users want to use the OFFICIALLY supported/default sdk from openteletry. opentelemetry (which encompasses both the two + some bootstrap/autoinstrumentation logic), they install this official package just to get autoinstrumentation behaviour? Behaviour that is not defined anywhere else?

If we do want to push for this, we should introduce "auto-instrumentation" concept into the specs, instead of doing something special for Python.

@NathanielRN
Copy link
Contributor Author

@lzchen You're right that there is nothing in the specs regarding this, but when I searched the org I found this enhancement proposal for OpenTelemetry without Manual instrumentation.

I would argue that this "new" package today satisfies all the requirements set out in the proposal.

i.e.

  • No source code modification required
  • Permissive licensing
  • Vendors can wrap this wrapper
  • Works well with manual instrumentation
  • Standard of telemetry for auto instrumentation is the same
  • No hard dependencies on vendors

However, notably, the proposal suggests that all languages should have an entirely separate repo for auto instrumentation for that language.

So overall, I once again agree that it would be cool :)

But I agree that we should not mark it as OT official if it's not yet.

opentelemetry (which encompasses both the two + some bootstrap/autoinstrumentation logic)

I think this is exactly the goal of this new opentelemetry-auto-instrumentation package. We don't have to mark it as OT official because it's not, but we have it for now so might as well add it as "plugin" like you mentioned and maybe gather some useful feedback on it from people using it in the next release.

This PR should not solve all those questions for now. Instead it just cleans up the expectations and dependencies that come with downloading opentelemtry-instrumentation vs downloading opentelemtry-auto-instrumentation. One is for people developing Instrumentations, the other is for people trying a useful plugin for OTel Python (Which has been improved to be configurable as of #1306 and I know people are excited to try it out 😄 )

Considering that, would you and @owais considering approving this PR so we can try this "plugin" out in the next release?

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on, the scripts/build.sh will need to be updated as well

@@ -68,6 +72,7 @@ changedir =
test-core-api: opentelemetry-api/tests
test-core-sdk: opentelemetry-sdk/tests
test-core-proto: opentelemetry-proto/tests
test-core-auto_instrumentation: opentelemetry-auto-instrumentation/tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wouldn't expect the - vs _ to make a difference here

@NathanielRN NathanielRN force-pushed the move-auto-instrumentation branch from e03cb5f to ba8531a Compare November 25, 2020 17:54
@NathanielRN
Copy link
Contributor Author

Thanks for your comments! Updated scripts/build.sh!

@lzchen
Copy link
Contributor

lzchen commented Nov 25, 2020

This PR should not solve all those questions for now.

+1

@NathanielRN
Copy link
Contributor Author

Closing this because we went with #1405 as a temporary solution.

@NathanielRN NathanielRN deleted the move-auto-instrumentation branch November 26, 2020 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants