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.

Sign up for GitHub

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

Jump to bottom

Allow customizing instrumentation loading #1751

Closed
wants to merge 3 commits into from
Closed

Allow customizing instrumentation loading #1751

wants to merge 3 commits into from

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 6, 2021 •
edited
Loading

Description

This feature allows distros to customize instrumentation initialization when using automatically instrumenting with the opentelemetry-instrument command.

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

  • Tested manually
  • Added test for newly introduced method

Does This PR Require a Contrib Repo Change?

  • Yes.
  • No.

Checklist:

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

@owais owais requested review from a team, ocelotl, lzchen and codeboten and removed request for a team April 6, 2021 08:41
@owais owais changed the title Instrumentation loader Allow customizing instrumentation loading Apr 6, 2021
@owais owais changed the title Allow customizing instrumentation loading POC: Allow customizing instrumentation loading Apr 6, 2021
@owais owais force-pushed the instrumentation-loader branch from 0f1b149 to 1574908 Compare April 6, 2021 11:08
@lzchen
Copy link
Contributor

lzchen commented Apr 6, 2021 •
edited
Loading

Is this ready for review?
Could you include a description of what the change is and what we are trying to achieve/link to an issue?

@owais
Copy link
Contributor Author

owais commented Apr 6, 2021

No, not ready yet. I wanted to get initial feedback on the approach before preparing it for actual review. Context here: https://cloud-native.slack.com/archives/C01PD4HUVBL/p1617697664091300

@owais owais added the WIP Work in progress label Apr 6, 2021
@owais
Copy link
Contributor Author

owais commented Apr 6, 2021

Updated description.

@ocelotl
Copy link
Contributor

ocelotl commented Apr 6, 2021

I'm having a bit of a hard time understanding this. The idea is for allowing instrumentations to receive configurable options, right?

A few questions:

  1. How is that it is not possible or practical to do so using environment variables?
  2. Would passing command line options to opentelemetry-instrument be another possibility?

codeboten
codeboten reviewed Apr 6, 2021
View reviewed changes
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.

I wonder if this is something we should define another entrypoint for or if it would be better to have a configuration object that includes a loader be returned from the distro configuration.

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py Outdated
instrumentation_loader(entry_points)


def _default_instrumentation_loader(entry_points):
Copy link
Contributor

Choose a reason for hiding this comment

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

If we go this route, we should define an interface for what makes a loader

@owais owais force-pushed the instrumentation-loader branch from 1574908 to 1304cd5 Compare April 7, 2021 01:58
@owais
Copy link
Contributor Author

owais commented Apr 7, 2021

I wonder if this is something we should define another entrypoint for or if it would be better to have a configuration object that includes a loader be returned from the distro configuration.

I like this idea. We already have a way to let distributions customize behavior. No need to introduce more. Updated the implementation.

One implication of using distro here is that loading instrumentations now depends on a Distro being installed while as before they'd be loaded even without one. This means users will have to at least install opentelemetry-distro for opentelemetry-instrument command to work. To avoid this, I'd added a DefaultDistro implementation that is used if no distro is found.

@codeboten

@owais
Copy link
Contributor Author

owais commented Apr 7, 2021 •
edited
Loading

@ocelotl

I'm having a bit of a hard time understanding this. The idea is for allowing instrumentations to receive configurable options, right?

Yes but it's mainly for distributions to configure instrumentations according to their needs. Flask accepts a "span name callback" for example which can be used to customize the name FlaskInstrumentor generated spans should use. lightstep/otel-launcher-python might want to specify such a custom callback. There is no easy way for a distro to do that today.

This also enables other use cases that were not possible before like distributions not wanted to support a specific version of an instrumentation. Let's say instrumentation X v0.25 has a known but that is not acceptable for a distribution, it can inspect the entrypoint and skip loading it or even load a fork instead. I'm just thinking out load :) but the point is that this gives distributions control over an important aspect of the opentelemetry-instrument command.

How is that it is not possible or practical to do so using environment variables?
Would passing command line options to opentelemetry-instrument be another possibility?

It is possible but not very nice and it may requires changes to instrumentations themselves. For example, we can specify excluded urls for Flask as an env var but we cannot specify a "span name callback" or "response callback" function via env vars or CLI args. Well, technically we can but then flask instrumentor will have to read a string and dynamically import a function from the path described by the string. That's not very convenient for an instrumentor to implement.

@owais owais force-pushed the instrumentation-loader branch from 1304cd5 to 983ad4e Compare April 7, 2021 02:14
@owais owais changed the title POC: Allow customizing instrumentation loading Allow customizing instrumentation loading Apr 7, 2021
@owais owais added auto-instrumentation related to auto-instrumentation of the sdk and removed WIP Work in progress labels Apr 7, 2021
@owais owais force-pushed the instrumentation-loader branch 8 times, most recently from ac376cc to 27b0839 Compare April 8, 2021 02:10
@owais
Load instrumentors via Distro
fb35915 
This commit makes the following changes:

- Introduces a new `load_instrumentor(EntryPoint) -> None:` with a
default implementation method to the `BaseDistro` class.
- The default implementation loads the insrumentor from the provided
entry point and calls applies it without any arguments. (same as before)
- sitecustomize now calls Distro's `load_instrumentor` method to load
and activate an instrumentor instead of doing it directly.
- Added a new `DefaultDistro` implementation which is used if not distro
is found by entry points.
@owais owais force-pushed the instrumentation-loader branch from 27b0839 to fb35915 Compare April 8, 2021 02:12
@owais owais requested a review from codeboten April 13, 2021 05:48
@ocelotl
Copy link
Contributor

ocelotl commented Apr 19, 2021

@ocelotl

I'm having a bit of a hard time understanding this. The idea is for allowing instrumentations to receive configurable options, right?

Yes but it's mainly for distributions to configure instrumentations according to their needs. Flask accepts a "span name callback" for example which can be used to customize the name FlaskInstrumentor generated spans should use. lightstep/otel-launcher-python might want to specify such a custom callback. How can the There is no easy way for a distro to do that today.

This also enables other use cases that were not possible before like distributions not wanted to support a specific version of an instrumentation. Let's say instrumentation X v0.25 has a known but that is not acceptable for a distribution, it can inspect the entrypoint and skip loading it or even load a fork instead. I'm just thinking out load :) but the point is that this gives distributions control over an important aspect of the opentelemetry-instrument command.

How is that it is not possible or practical to do so using environment variables?
Would passing command line options to opentelemetry-instrument be another possibility?

It is possible but not very nice and it may requires changes to instrumentations themselves. For example, we can specify excluded urls for Flask as an env var but we cannot specify a "span name callback" or "response callback" function via env vars or CLI args. Well, technically we can but then flask instrumentor will have to read a string and dynamically import a function from the path described by the string. That's not very convenient for an instrumentor to implement.

Actually, we have been using entry points to allow for environment variables to point to functions or classes (they can point to any Python object). So, any callback can be specified with an environment variable in this way.

@owais owais mentioned this pull request Apr 28, 2021
Merged
12 tasks
@owais owais closed this Apr 28, 2021
@owais
Copy link
Contributor Author

owais commented Apr 30, 2021

Moved PR to contrib: open-telemetry/opentelemetry-python-contrib#480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocelotl ocelotl Awaiting requested review from ocelotl

@lzchen lzchen Awaiting requested review from lzchen

@codeboten codeboten Awaiting requested review from codeboten

Assignees
No one assigned
Labels
auto-instrumentation related to auto-instrumentation of the sdk
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@owais @lzchen @ocelotl @codeboten