Enforce one distro is installed #571

NathanielRN · 2021-07-09T00:32:09Z

Description

Small PR to enforce that only one distro be installed when trying to run auto instrumentation.

Additionally, checks that the Configurator that is loaded automatically is a derived class of BaseConfigurator. The idea is that Configurators should come from distros and every distro should provide one. (Even if they just subclass the opentelemetry-sdk package Configurator which does a pretty good configuration).

Follow up to: #551

Type of change

Please delete options that are not relevant.

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?

Unfortunately I can't add unit tests because if you import functions from sitecustomize.py it will try to run the whole file and auto instrument which we can't do in a testing environment for now...

But I did confirm locally that an error is thrown only if multiple distros are installed:

$ OTEL_RESOURCE_ATTRIBUTES=service.name=my_auto_app \
OTEL_EXPORTER_OTLP_INSECURE=True \
OTEL_EXPORTER_OTLP_ENDPOINT=localhost:4317 \
LISTEN_ADDRESS=127.0.0.1:8080 \
opentelemetry-instrument python3 auto-instrumentation/flask/application.py

Multiple distros were found: (opentelemetry.distro,opentelemetry.distro_aws,opentelemetry.distro_test). Only one should be installed.
Failed to auto initialize opentelemetry
Traceback (most recent call last):
  File "/Users/enowell/git/opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py", line 132, in initialize
    distro = _load_distros()
  File "/Users/enowell/git/opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py", line 45, in _load_distros
    raise RuntimeError("Cannot Auto Instrument with multiple distros installed.")
RuntimeError: Cannot Auto Instrument with multiple distros installed.
 * Serving Flask app "create_flask_app" (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: on
 * Running on http://127.0.0.1:8080/ (Press CTRL+C to quit)

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
~~- [ ] Unit tests have been added~~ See comment about testing above.
Documentation has been updated

NathanielRN · 2021-07-09T00:33:03Z

@owais Here is a PR to enforce only 1 distro be installed on the system! Would appreciate your feedback 😄

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

owais · 2021-07-09T09:12:46Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

@@ -96,7 +113,14 @@ def _load_configurators():
            )
            continue
        try:
-            entry_point.load()().configure()  # type: ignore
+            configurator: BaseConfigurator = entry_point.load()()
+            if not isinstance(configurator, BaseConfigurator):


Do we want to enforce nominal typing at runtime here? I don't see this adding a ton of benefit but these things keep adding up to the "startup" cost of the SDK which can be critical in environments like AWS Lambda with really low resources.

If we consider startup costs should we remove the type check for BaseDistro?

opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

Lines 38 to 42 in 753e228

if not isinstance(distro, BaseDistro):

logger.debug(

"%s is not an OpenTelemetry Distro. Skipping",

entry_point.name,

)

I guess it has the benefit of checking a Distro and we know that Distro's have Configurators. I'll remove this but let me know if you think we should remove the BaseDistro check too.

I'm not sure. What benefit do we get from this message? it's also logging on debug level but we are paying the cost of isinstance always. i personally feel we should remove this. This isn't something that is going to be very helpful to end users IMO. If anyone publishes a package with a distro entrypoint that doesn't satisfy the interface, an attribute/type error would probably be good enough I think

Okay I agree with you! I will remove it since I'm changing those lines anyways.

owais · 2021-07-09T19:20:06Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+            raise RuntimeError("Cannot Auto Instrument with multiple distros installed.")
+
+        return first_distro.load()()
+    except IndexError:


this would be StopIteration I think.

Right, thank you! I changed it and configured that's what we wanted in my tests 😄

opentelemetry-instrument python3 auto-instrumentation/flask/application.py Initializing Auto Instrumentation without using a distro. * Serving Flask app "create_flask_app" (lazy loading) * Environment: production WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead. * Debug mode: on * Running on http://127.0.0.1:8080/ (Press CTRL+C to quit)

ocelotl · 2021-07-09T22:31:05Z

Hm, not sure if this is the approach we previously discussed. I don't think we should enforce only one distro being installed, but we should have a default one and let the user select one with an environment variable.

NathanielRN · 2021-07-09T22:34:24Z

@ocelotl I think that is Option 1 in issue #551. The votes were in favor of Option 2 when I brought it up again so I added an implementation of it in this PR. I think it's the right way to go, but maybe we can discuss it more in the issue?

ocelotl · 2021-07-09T22:44:15Z

@ocelotl I think that is Option 1 in issue #551. The votes were in favor of Option 2 when I brought it up again so I added an implementation of it in this PR. I think it's the right way to go, but maybe we can discuss it more in the issue?

Ah, yes. Sorry, I forgot to vote. Also, detecting the error when running opentelemetry-instrument is too late. The error should be raised when another additional distro is installed.

NathanielRN · 2021-07-09T22:51:43Z

The error should be raised when another additional distro is installed.

Hm I'm not familiar with how to make packages exclusive from each other at install time... I don't know if you can do that at pip install time. Even if you can, there's always a possibility that a user just unzips the 2nd distro package's contents in the python3.*/ folder and I'm pretty sure you can't throw an error/detect it at that time 😕

ocelotl · 2021-07-09T22:56:36Z

Also, this approach is very inconvenient for someone who may want to switch between more than one distro repeatedly (for testing purposes, for example).

srikanthccv · 2021-07-10T09:12:50Z

I have a dumb question, why do we limit to only one distro?

opentelemetry-instrumentation/README.rst

lzchen · 2021-07-12T17:40:16Z

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

+        return first_distro.load()()
+    except StopIteration:
+        logger.warning(
+            "Initializing Auto Instrumentation without using a distro."


A description of the error would be good.

Added the following description:

logger.warning( "Failed to find even one installed distro. Initializing Auto Instrumentation without using a distro." )

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

lzchen · 2021-07-12T17:44:33Z

@NathanielRN
Is the PR description/title still up to date? I don't see the changes pertaining to Configurator

NathanielRN · 2021-07-12T19:34:55Z

@lonewolf3739

I have a dumb question, why do we limit to only one distro?

It's not a dumb questions 😄 The goal is just to make it easier for the user, because if multiple distros are installed they probably will set conflicting values and produce confusing bugs.

i.e.

opentelemetry-distro-aws will set OTEL_PROPAGATORS=xray

opentelemetry-distro-microsoft will set OTEL_PROPAGATORS=azure

We mentioned this comment in #551 that this would be confusing.

But like @ocelotl said there are cases like testing multiple distros where this setup would be inconvenient. It probably warrants a SIG discussion at this point.

NathanielRN · 2021-07-12T19:50:10Z

@lzchen

Is the PR description/title still up to date? I don't see the changes pertaining to Configurator

Enforcing 1 Configurator is already true as of right now:

opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py

Lines 91 to 97 in 2ee2cf3

    
           if configured is not None: 
        
               logger.warning( 
        
                   "Configuration of %s not loaded, %s already loaded", 
        
                   entry_point.name, 
        
                   configured, 
        
               ) 
        
               continue

What I meant in this PR is that we enforce one Distro which means we enforce one Configurator. However I can see why that's confusing, I'll update the title.

srikanthccv · 2021-07-13T03:32:06Z

because if multiple distros are installed they probably will set conflicting values and produce confusing bugs.

The way I imagined this is each distro would add to existing values or creates one if it doesn't exist, for example, the propagator is initially set to jaeger, and the vendor distro such as azure would update it to jaeger,azure. And when there are conflicting cases where there can be only one value then the latest update value is considered (following how we do it resource merge etc...).

NathanielRN · 2021-08-12T16:21:25Z

We decided to go in a different direction for distro, follow the results here: open-telemetry/opentelemetry-python#2005

NathanielRN requested review from a team, ocelotl and lzchen and removed request for a team July 9, 2021 00:32

NathanielRN force-pushed the enforce-one-distro-and-configurator branch from b653174 to 228061f Compare July 9, 2021 00:35

NathanielRN mentioned this pull request Jul 9, 2021

Default configurators do more for distros open-telemetry/opentelemetry-python#1937

Merged

7 tasks

NathanielRN added 3 commits July 8, 2021 17:44

Enforce 1 distro+configurator for auto instrumentation

0e22758

Changelog for enforcing 1 distro+configurator

00f5532

Update documentation to reflect distro strictness

6f057bd

NathanielRN force-pushed the enforce-one-distro-and-configurator branch from 228061f to 6f057bd Compare July 9, 2021 00:45

owais reviewed Jul 9, 2021

View reviewed changes

NathanielRN added 4 commits July 9, 2021 11:42

Simplify distro loading logic

070c37e

Quick variable reference fixes

6f34f1d

Do not need BaseConfigurator import

eaae961

Remove type check for BaseDistro

68484fe

owais reviewed Jul 9, 2021

View reviewed changes

NathanielRN added 2 commits July 9, 2021 13:59

Use StopIteration because we use generator next now

afaab23

Better logic for stopiteration catch

40a5e1b

NathanielRN requested a review from owais July 9, 2021 21:02

NathanielRN mentioned this pull request Jul 9, 2021

Making it easier to create custom distros #551

Closed

lzchen reviewed Jul 12, 2021

View reviewed changes

opentelemetry-instrumentation/README.rst Outdated Show resolved Hide resolved

lzchen reviewed Jul 12, 2021

View reviewed changes

opentelemetry-instrumentation/README.rst Outdated Show resolved Hide resolved

lzchen reviewed Jul 12, 2021

View reviewed changes

opentelemetry-instrumentation/README.rst Outdated Show resolved Hide resolved

lzchen reviewed Jul 12, 2021

View reviewed changes

...etry-instrumentation/src/opentelemetry/instrumentation/auto_instrumentation/sitecustomize.py Outdated Show resolved Hide resolved

Merge branch 'main' into enforce-one-distro-and-configurator

0e4d8ab

Add small documentation and interface updates

cacaa0b

NathanielRN changed the title ~~Enforce one distro and configurator~~ Enforce one distro is installed Jul 12, 2021

NathanielRN requested a review from lzchen July 12, 2021 19:50

NathanielRN closed this Aug 12, 2021

NathanielRN deleted the enforce-one-distro-and-configurator branch August 12, 2021 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce one distro is installed #571

Enforce one distro is installed #571

NathanielRN commented Jul 9, 2021 •

edited

Loading

NathanielRN commented Jul 9, 2021

owais Jul 9, 2021

NathanielRN Jul 9, 2021

owais Jul 9, 2021

NathanielRN Jul 9, 2021

owais Jul 9, 2021

NathanielRN Jul 9, 2021

ocelotl commented Jul 9, 2021

NathanielRN commented Jul 9, 2021

ocelotl commented Jul 9, 2021

NathanielRN commented Jul 9, 2021

ocelotl commented Jul 9, 2021

srikanthccv commented Jul 10, 2021

lzchen Jul 12, 2021

NathanielRN Jul 12, 2021

lzchen commented Jul 12, 2021

NathanielRN commented Jul 12, 2021

NathanielRN commented Jul 12, 2021

srikanthccv commented Jul 13, 2021

NathanielRN commented Aug 12, 2021

	if not isinstance(distro, BaseDistro):
	logger.debug(
	"%s is not an OpenTelemetry Distro. Skipping",
	entry_point.name,
	)

Enforce one distro is installed #571

Enforce one distro is installed #571

Conversation

NathanielRN commented Jul 9, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

NathanielRN commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocelotl commented Jul 9, 2021

NathanielRN commented Jul 9, 2021

ocelotl commented Jul 9, 2021

NathanielRN commented Jul 9, 2021

ocelotl commented Jul 9, 2021

srikanthccv commented Jul 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen commented Jul 12, 2021

NathanielRN commented Jul 12, 2021

NathanielRN commented Jul 12, 2021

srikanthccv commented Jul 13, 2021

NathanielRN commented Aug 12, 2021

NathanielRN commented Jul 9, 2021 •

edited

Loading