fix(click): issue with environment sniffing for click instrumentation

[lannuttia]

Description

This change attempts to make the click environment sniffing more robust by using loaded modules to sniff out whether it is being ran as part of a server or not.

Fixes #3846

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This has been validated with automated tests that are included in this pull request.

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• 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
• Documentation has been updated

[lannuttia]

After having thought about this some, this might not actually work if the asgi or wsgi instrumentation have not been loaded yet. I'll need to know whether this can be guaranteed or not at the time _command_invoke_wrapper is invoked.

[xrmx]

I don't think we can assume that other packages are even installed so I think this should be orthogonal to the current one.

Said that I think you can assume that the other models will be available once the instrumentors are imported.

import sys

from opentelemetry.instrumentation.flask import FlaskInstrumentor

for module in sorted(x for x in sys.modules.keys() if x.startswith("opentelemetry")):
    print(module)

with opentelemetry-instrument python modules.py and the opentelemetry.instrumentation.wsgi module was there.

[xrmx]

Btw once we have an agreement could you please also update asynclick instrumentation?

[lannuttia]

I don't think I understand what you mean about not being able to assume other packages are even installed or this check being orthogonal to the current check.

The check that I'm suggesting here is just checking to see if the asgi or wsgi instrumentation is loaded at the time of the check. The check shouldn't be dependent on any package being installed since it's just checking a dict from the sys module in the python standard library for some string key.

If either the ASGI or WSGI instrumentation is loaded, then I would think we should be running in a server context and we probably do not want the click instrumentation. The check that I'm performing should just perform a more implementation agnostic check to know that click instrumentation should be bypassed. So if there are other ASGI or WSGI servers that decide to start using click one day, that doesn't require a change to the click instrumentation to accommodate that change and unbreak things for consumers.

With all that being said, I was able to confirm locally today that the ASGI instrumentation module was in fact loaded when the check was performed. I would assume that the same would be true for when the WSGI instrumentation module would be used. I'm happy enough with what I've seen to resolve this comment since that was my original concern.

If you still want further discussion on the other points you brought up, I'd be more than happy to break those out into other conversations since I know I probably didn't address your concerns there.

[xrmx]

What I mean is that the wsgi and asgi instrumentation are coming from a different package each, and click instrumentation is a different package again. And there's no guarantee that these packages will be installed at the same time. If the packages are not installed how can they be loaded in sys.modules?

[lannuttia]

Yeah, this solution is agnostic to what specifically is installed. The ASGI and/or WSGI instrumentation don't need to be installed for this to work. We're just checking for the presence of a string key in the sys.modules dict that should only be there if the ASGI and/or WSGI instrumentation is both installed and imported. If they aren't installed, then they won't be in the dict.

[xrmx]

Sure what I mean is that there might be other cases that the current code is covering but yours is not (a server not having asgi and wsgi in its sys.modules) and for that reason I would like to keep both mechanism in place.

[lannuttia]

One additional thing that I'd like to point out is I still don't think I'm super happy with how this works. I don't really like how I can't think of a way to test this behavior without mocking and effectively coupling the test to the implementation. I'm doing it this way primarily because I see it as being the shortest path to more robustly sniffing out if we shouldn't be running the click instrumentation.

I have no idea how large of a lift this would be but it might be nice to provide a way for instrumentation to disable other instrumentation. If we were to add something like that as part of the BaseInstrumentor class and determine what instrumentation should and should not be enabled based on something like a property of that class, I think that would lead to things being more testable. Then we don't have to couple the test here to the implementation by mocking sys.modules. We can have the test just ensure that the property is what we expect it to be and trust that whatever's responsible for honoring that, does.