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

Adding opentelemetry-distro package and entrypoint #1482

Merged
merged 19 commits into from
Jan 19, 2021

Conversation

codeboten
Copy link
Contributor

Description

As per the discussion in the SIG meeting on 12/03, providing an entrypoint that is loaded before the configurator would reduce the amount of work needed by users in creating distro's for otel python. This PR proposes that change by introducing an opentelemetry_distro entrypoint, as well as a distro that configures the OTLP exporter by default.

Fixes #1481

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

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

@codeboten codeboten requested a review from a team as a code owner December 15, 2020 01:23
@codeboten codeboten requested review from owais and lzchen and removed request for a team December 15, 2020 01:23
@codeboten codeboten changed the title Distro Adding opentelemetry-distro package and entrypoint Dec 15, 2020
@@ -33,7 +33,7 @@
EXPORTER_OTLP = "otlp"
EXPORTER_OTLP_SPAN = "otlp_span"
EXPORTER_OTLP_METRIC = "otlp_metric"
_DEFAULT_EXPORTER = EXPORTER_OTLP
_DEFAULT_EXPORTER = "none"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is changing the default in the SDK configurator not to make a decision for users as to what exporter to configure

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove the line completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@lzchen
Copy link
Contributor

lzchen commented Dec 15, 2020

Curious what is the execution order of things in sitecustomize.py? Is it always run first? If users were to auto-instrument using one of the console scripts, do the scripts run first or sitecustomize.py?

opentelemetry-distro/setup.cfg Outdated Show resolved Hide resolved
@codeboten codeboten requested a review from lzchen January 18, 2021 23:17
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

I think the nomenclature here is a bit confusing. We have a Distro class that does configuration. Actually, what we have here is not only a way to perform configuration but a hook that allows users to register some code to be executed before the instrumentors are executed, right?

I don't want to over-generalize this, but maybe users will also need to run stuff after the instrumentors? Is this an opportunity to provide a more general-purpose hook mechanism? I understand if you don't want to spend much time in these abstract design or philosophical questions, just mentioning this just in case this brings in you any valuable idea.

Any way, I believe the Distro name is not very appropriate, maybe something more precise like BeforeInstrumentors would fit better? 🤷‍♂️

opentelemetry-distro/setup.cfg Outdated Show resolved Hide resolved
opentelemetry-distro/setup.cfg Outdated Show resolved Hide resolved
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

[options.entry_points]
opentelemetry_distro =
distro = opentelemetry.distro:OpenTelemetryDistro
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default Opentelemetry distro right? What would other distros be called?
azure = opentelemetry.distro:AzureDistro something like this?

Copy link
Contributor Author

@codeboten codeboten Jan 19, 2021

Choose a reason for hiding this comment

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

i'd originally called it otel_distro, but based on @ocelotl's feedback changed it to distro. I don't feel strongly one way or the other on this. I would expect the distro to match the vendor, as you suggested, azure would identify the Azure distro. in that case maybe this would make more sense as otel

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think it's best to prefix everything with an "opentelemetry" identifier just in case there are other libraries using a similar name within the entry points.

@lzchen
Copy link
Contributor

lzchen commented Jan 19, 2021

@ocelotl
I think the purpose of distros is to run vendor specific logic before instantiation. I'm not sure if there will be the existence of after hooks so this is most likely a different concept than that. Regardless, distro is a commonly used term to describe something like this, see Java

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Nice. Just one non-blocking comment.

@codeboten
Copy link
Contributor Author

@ocelotl: The term distro was proposed in a few different SIG meetings (maintainers and spec meetings that i know of), it's a concept that is already being promoted both in the opentelemetry community and by vendors.

This PR is moving code that was doing some default configuration out of the SDK and into a distro that can be used as-is by users. This also serves as an example for how others could implement their own distro, with whatever defaults makes sense for them.

@ocelotl
Copy link
Contributor

ocelotl commented Jan 19, 2021

@ocelotl: The term distro was proposed in a few different SIG meetings (maintainers and spec meetings that i know of), it's a concept that is already being promoted both in the opentelemetry community and by vendors.

This PR is moving code that was doing some default configuration out of the SDK and into a distro that can be used as-is by users. This also serves as an example for how others could implement their own distro, with whatever defaults makes sense for them.

Ok, sounds good 👍 Approving 😎

@codeboten codeboten merged commit d994e75 into open-telemetry:master Jan 19, 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.

Provide Distro mechanism
4 participants