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

Refactor distros #1966

Closed
wants to merge 2 commits into from
Closed

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jul 19, 2021

This is a proposed refactoring of the distro mechanism. This PR is being opened so that the general approach can be reviewed, formal tests are still pending.

It implements solution B.

I found a few issues with the current distro mechanism:

  1. There is redundancy between Distros and Configurators. Both perform configuration.
  2. We require a Distro to be installed for opentelemtetry-instrumentation to work. I disagree with this because opentelemetry-instrumentation is something that is (at least partially) defined in the spec, Distro is not.
  3. opentelemetry-instrumentation is directly coupled with Distro. Distro is a configuration mechanism that may not be preferred by the end user who may prefer to configure instead with a configuration file or a bash script, etc.

To solve these issues, this PR does the following, respectively:

  1. I find Configurator to have the most representative name, so this PR removes Distro and refactors Configurator.
  2. Having a Configurator is no longer required for opentelemetry-instrumentation to work.
  3. opentelemetry-instrumentation now provides 2 entry points, one for plugins that are called before instrumentation and other for plugins that are called after instrumentation. Configurator is decoupled from opentelemetry-instrumentation by making it a plugin that is called before instrumentation.

opentelemetry-configurator provides an entry point by itself. In this way several Configurators can be registered and several can be called in the order defined by the environment variable OTEL_PYTHON_CONFIGURATORS if there is more than one Configurator installed.

Configurators are classes. This approach makes it possible to share common configuration by just inheriting from another one and calling super.

@ocelotl ocelotl requested a review from a team as a code owner July 19, 2021 01:01
@ocelotl ocelotl self-assigned this Jul 19, 2021
@ocelotl ocelotl requested review from codeboten and aabmass and removed request for a team July 19, 2021 01:01
@ocelotl ocelotl force-pushed the refactor-distros branch 4 times, most recently from aaeab40 to 495552e Compare July 21, 2021 23:37
@ocelotl ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jul 21, 2021
@ocelotl ocelotl force-pushed the refactor-distros branch 2 times, most recently from c5534c3 to b512bf4 Compare July 23, 2021 13:33
@lzchen
Copy link
Contributor

lzchen commented Sep 9, 2021

As discussed in the SIG, @owais will be leading distro dicussions, closing this for now until we decide what direction we are heading for distros.

@lzchen lzchen closed this Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants