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

POC: support target dependencies for instrumentations #1741

Closed

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 3, 2021

Description

This is a POC solution for the problem discussed in #1729 and #1563

Details

Instead of specifying instrumented libraries (target dependencies) as regular dependencies, instrumentation packages can specify such dependencies as extras with the label target. For example, opentelemetry-instrumentation-aiohttp-client can specify aoihttp as a target dependencies instead of a regular one:

[options.extras_require]
target = 
    aiohttp ~= 3.0
  • This will make opentelemetry-instrumentation-aiohttp-client installable without automatically installing or updating aiohttp.
  • opentelemetry-instrument command will be able to tell that the instrumentor needs aiohttp ~= 3.0. It'll only load the instrumentor if this requirement is satisfied i.e, aiohttp ~= 3.0 is installed as well.
  • If the target package is not installed, the instrumentation package will not be loaded. This will prevent the instrumentor from running and causing import errors.
  • This still allows users to automatically install the target dependencies if they want by suffixing [target] to the instrumentation package name opentelemetry-instrumentation-aiohttp-client[target]
    - Purely a packaging change. Does not require any interface or code changes to instrumentors.
  • Mostly a packaging change. Requires minimal code changes to instrumentations i.e adding a simple package_name method.

The instrument command will not require instrumentors to specify target dependencies. If an instrumentor does not specify any target dependencies, the instrument command will always load and apply the instrumentor.

Changes required to be made to instrumentations

Every instrumentation will need two changes.

  1. Move the target dependencies from regular deps to extra_require under label target.
  2. Each instrumentor would need to define a package_name() -> str method. This method would return the Python package name of the instrumentor. For example RequestsInstrumentor().package_name() would return opentelemetry-instrumentation-requests.

This demo contrib PR showcases what changes every instrumentation package would need: https://github.com/open-telemetry/opentelemetry-python-contrib/pull/397/files

Considerations

  • Do we enforce all instrumentors to always specify "target" dependencies like this? Instrumentors that don't specify target dependencies can still be used manually but the opentelemetry-instrument command will ignore them. Can we think of any cases where specifying target dependencies may not be viable or desirable?
  • I'm not sure about using the name target to represent instrumented libraries. I just picked something to allow me to move forward with the POC. What would be a better name for such deps?
  • We need the package_name method in order to fetch the instrumentation distribution using pkg_resource and validate it's dependencies. Can we somehow automatically detect an instrumentation's package name without it having to explicitly specify it as a method? Such auto-detection needs to be highly reliable and have acceptable performance.
  • Instead of making the instrumentors define a package_name() method that returns the Python package name of the instrumentor, we can have the instrumentors define a package_name() -> str method that just returns the package name of the instrumentor itself. We can implement it either way and it has almost no impact on the implementation. The question is what makes more sense for instrumentation authors.
class RequestsInstrumention:
    def package_name(self):
         return 'opentelemetry-instrumentation-requests'

vs

class RequestsInstrumention:
    def target_dependencies(self):
         return ['requests ~= 2.0']

Future iterations

  • Evaluate lazily applying instrumentors as suggested here. This would require code changes to all instrumentors but can be built on top of this POC.
  • Bootstrap command can be updated to support optionally installing target dependencies as well.

@owais owais force-pushed the instrumentation-runtime-checks branch from 10b6e4f to 05d6132 Compare April 3, 2021 06:59
@srikanthccv
Copy link
Member

What does this mean for individual instrumentation packages? Right now if we try to install opentelemetry-instrumentation-lib and lib with incompatibility pip throws an error. By making it optional target which users might not be aware/opt-in wouldn't this lead to undesirable outcome when I am just interested in instrumenting one or two libs?

@owais
Copy link
Contributor Author

owais commented Apr 3, 2021

@lonewolf3739 Good point. It would mean the instrumentation would be a noop when used with opentelemetry-instrument command. For people manually instrumenting in code, I think the outcome will be undefined. Things may or may not work if a user installs an incompatible version. We'd have to mention compatible versions in each instrumentation's documentation. We'd also always mention opentelemetry-instrumentation-lib[target] in all examples depicting installation of individual instrumentations.

That said, I think I can easily add some safe-guards for this in base instrumentor. Will look into it and update the PR later today.

@owais
Copy link
Contributor Author

owais commented Apr 3, 2021

@lonewolf3739 the following patch builds on top of this PR and supports the same runtime conflict checking for individual instrumentations when instrumenting manually with code

https://github.com/owais/opentelemetry-python/pull/2/files

This however requires minimal changes to instrumentors. Each instrumentor will need to define a package_name method that returns the instrumentors Python package name. For example, FalconInstrumentor would need to add the following method:

def package_name(self):
    return "opentelemetry-instrumentation-falcon"

Alternatively, we can return the list of extra dependencies from instrumentor. So instead of instrumentors having to define a package_name() -> str method, they'd define target_dependencies() -> Collection[str]. Something like:

def target_dependencies(self) -> Collection[str]:
    return ['falcon ~= 2.0']

In both cases, the value returned could be stored in a python file and injected into setup.py programmatically just like how we handle version numbers today.

@owais
Copy link
Contributor Author

owais commented Apr 3, 2021

@lonewolf3739 updated the PR description.

@owais owais force-pushed the instrumentation-runtime-checks branch 2 times, most recently from a8b401b to 58a6dab Compare April 3, 2021 19:30
@owais owais force-pushed the instrumentation-runtime-checks branch from 58a6dab to 7612edf Compare April 4, 2021 10:14
@owais owais closed this Apr 28, 2021
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.

2 participants