Skip to content

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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+70 −51
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b97af6
fix(click): issue with environment sniffing for click instrumentation
lannuttia Oct 15, 2025
59fca1f
fix(asyncclick): disable asyncclick for long running processes like s…
lannuttia Oct 16, 2025
a0525b2
Merge branch 'main' into improve-click-environment-sniffing
lannuttia Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `opentelemetry-instrumentation-asyncclick`: fix issue where servers using asyncclick would not get a separate span per-request
- `opentelemetry-instrumentation-click`: fix issue where starting uvicorn via `python -m` would cause the click instrumentation to give all requests the same trace id
- `opentelemetry-instrumentation-flask`: Do not record `http.server.duration` metrics for excluded URLs.
([#3794](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3794))
- `opentelemetry-instrumentation-botocore`: migrate off the deprecated events API to use the logs API
Expand Down
21 changes: 10 additions & 11 deletions ...metry-instrumentation-asyncclick/src/opentelemetry/instrumentation/asyncclick/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,7 @@ async def hello():
import sys
from functools import partial
from logging import getLogger
from typing import (
TYPE_CHECKING,
Any,
Awaitable,
Callable,
Collection,
TypeVar,
)
from typing import TYPE_CHECKING, Any, Awaitable, Callable, Collection, TypeVar

import asyncclick
from typing_extensions import ParamSpec, Unpack
Expand All @@ -68,9 +61,7 @@ async def hello():
from opentelemetry.instrumentation.asyncclick.package import _instruments
from opentelemetry.instrumentation.asyncclick.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.instrumentation.utils import (
unwrap,
)
from opentelemetry.instrumentation.utils import unwrap
from opentelemetry.semconv._incubating.attributes.process_attributes import (
PROCESS_COMMAND_ARGS,
PROCESS_EXECUTABLE_NAME,
Expand Down Expand Up @@ -112,6 +103,14 @@ async def _command_invoke_wrapper(

ctx = args[0]

# we don't want to create a root span for long running processes like servers
# otherwise all requests would have the same trace id
if (
"opentelemetry.instrumentation.asgi" in sys.modules
or "opentelemetry.instrumentation.wsgi" in sys.modules
):
return await wrapped(*args, **kwargs)

span_name = ctx.info_name
span_attributes = {
PROCESS_COMMAND_ARGS: sys.argv,
Expand Down
31 changes: 31 additions & 0 deletions instrumentation/opentelemetry-instrumentation-asyncclick/tests/test_asyncclick.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import asyncio
import os
import sys
from typing import Any
from unittest import IsolatedAsyncioTestCase, mock

Expand Down Expand Up @@ -158,6 +159,36 @@ async def command_raises() -> None:
},
)

@mock.patch("sys.argv", ["command.py"])
def test_disabled_when_asgi_instrumentation_loaded(self):
@asyncclick.command()
async def command():
pass

with mock.patch.dict(
sys.modules,
{**sys.modules, "opentelemetry.instrumentation.asgi": mock.Mock()},
):
result = run_asyncclick_command_test(command)
self.assertEqual(result.exit_code, 0)

self.assertFalse(self.memory_exporter.get_finished_spans())

@mock.patch("sys.argv", ["command.py"])
def test_disabled_when_wsgi_instrumentation_loaded(self):
@asyncclick.command()
async def command():
pass

with mock.patch.dict(
sys.modules,
{**sys.modules, "opentelemetry.instrumentation.wsgi": mock.Mock()},
):
result = run_asyncclick_command_test(command)
self.assertEqual(result.exit_code, 0)

self.assertFalse(self.memory_exporter.get_finished_spans())

def test_uninstrument(self):
AsyncClickInstrumentor().uninstrument()

Expand Down
29 changes: 5 additions & 24 deletions ...n/opentelemetry-instrumentation-click/src/opentelemetry/instrumentation/click/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,11 @@ def hello():
import click
from wrapt import wrap_function_wrapper

try:
from flask.cli import ScriptInfo as FlaskScriptInfo
except ImportError:
FlaskScriptInfo = None


from opentelemetry import trace
from opentelemetry.instrumentation.click.package import _instruments
from opentelemetry.instrumentation.click.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.instrumentation.utils import (
unwrap,
)
from opentelemetry.instrumentation.utils import unwrap
from opentelemetry.semconv._incubating.attributes.process_attributes import (
PROCESS_COMMAND_ARGS,
PROCESS_EXECUTABLE_NAME,
Expand All @@ -74,20 +66,6 @@ def hello():
_logger = getLogger(__name__)


def _skip_servers(ctx: click.Context):
# flask run
if (
ctx.info_name == "run"
and FlaskScriptInfo
and isinstance(ctx.obj, FlaskScriptInfo)
):
return True
# uvicorn
if ctx.info_name == "uvicorn":
return True
return False


def _command_invoke_wrapper(wrapped, instance, args, kwargs, tracer):
# Subclasses of Command include groups and CLI runners, but
# we only want to instrument the actual commands which are
Expand All @@ -99,7 +77,10 @@ def _command_invoke_wrapper(wrapped, instance, args, kwargs, tracer):

# we don't want to create a root span for long running processes like servers
# otherwise all requests would have the same trace id
if _skip_servers(ctx):
if (
"opentelemetry.instrumentation.asgi" in sys.modules
or "opentelemetry.instrumentation.wsgi" in sys.modules
):
Copy link
Contributor Author

@lannuttia lannuttia Oct 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@xrmx xrmx Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@xrmx xrmx Oct 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lannuttia lannuttia Oct 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@xrmx xrmx Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@lannuttia lannuttia Oct 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@xrmx xrmx Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return wrapped(*args, **kwargs)

span_name = ctx.info_name
Expand Down
38 changes: 22 additions & 16 deletions instrumentation/opentelemetry-instrumentation-click/tests/test_click.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,12 @@
# limitations under the License.

import os
import sys
from unittest import mock

import click
import pytest
from click.testing import CliRunner

try:
from flask import cli as flask_cli
except ImportError:
flask_cli = None

from opentelemetry.instrumentation.click import ClickInstrumentor
from opentelemetry.test.test_base import TestBase
from opentelemetry.trace import SpanKind
Expand Down Expand Up @@ -168,24 +163,35 @@ def command_raises():
},
)

def test_uvicorn_cli_command_ignored(self):
@click.command("uvicorn")
def command_uvicorn():
@mock.patch("sys.argv", ["command.py"])
def test_disabled_when_asgi_instrumentation_loaded(self):
@click.command()
def command():
pass

runner = CliRunner()
result = runner.invoke(command_uvicorn)
with mock.patch.dict(
sys.modules,
{**sys.modules, "opentelemetry.instrumentation.asgi": mock.Mock()},
):
result = runner.invoke(command)
self.assertEqual(result.exit_code, 0)

self.assertFalse(self.memory_exporter.get_finished_spans())

@pytest.mark.skipif(flask_cli is None, reason="requires flask")
def test_flask_run_command_ignored(self):
@mock.patch("sys.argv", ["command.py"])
def test_disabled_when_wsgi_instrumentation_loaded(self):
@click.command()
def command():
pass

runner = CliRunner()
result = runner.invoke(
flask_cli.run_command, obj=flask_cli.ScriptInfo()
)
self.assertEqual(result.exit_code, 2)
with mock.patch.dict(
sys.modules,
{**sys.modules, "opentelemetry.instrumentation.wsgi": mock.Mock()},
):
result = runner.invoke(command)
self.assertEqual(result.exit_code, 0)

self.assertFalse(self.memory_exporter.get_finished_spans())

Expand Down