From 9072f0c1547ff83a0ecd11d2acfde1bb593487de Mon Sep 17 00:00:00 2001 From: Br1an67 <932039080@qq.com> Date: Tue, 3 Mar 2026 23:07:21 +0800 Subject: [PATCH] feat: integrate Django Ninja exception recording into instrument_django() Restructure the Django Ninja integration per review feedback: - Move ninja patching into instrument_django() with instrument_ninja=True default - Patch NinjaAPI.on_exception at the class level (not per-instance) so it works without arguments for `logfire run` - Add double-instrumentation guard using _LOGFIRE_INSTRUMENTED constant - Correctly set escaped=True/False based on whether exception is re-raised - Silently skip if django-ninja is not installed - Remove separate instrument_django_ninja() method, public API, type stubs - Remove django-ninja optional extra (users are expected to have it) - Add tests with head_sample_rate=0 to verify is_recording() branches --- logfire-api/logfire_api/_internal/main.pyi | 2 +- logfire/_internal/integrations/django.py | 39 +++++- logfire/_internal/main.py | 7 + pyproject.toml | 3 + .../django_test_app/views.py | 19 +++ .../django_test_site/urls.py | 3 + tests/otel_integrations/test_django_ninja.py | 128 ++++++++++++++++++ uv.lock | 27 +++- 8 files changed, 223 insertions(+), 5 deletions(-) create mode 100644 tests/otel_integrations/test_django_ninja.py diff --git a/logfire-api/logfire_api/_internal/main.pyi b/logfire-api/logfire_api/_internal/main.pyi index 740a0aac5..cb87886e0 100644 --- a/logfire-api/logfire_api/_internal/main.pyi +++ b/logfire-api/logfire_api/_internal/main.pyi @@ -698,7 +698,7 @@ class Logfire: Args: **kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` method, for future compatibility. """ - def instrument_django(self, capture_headers: bool = False, is_sql_commentor_enabled: bool | None = None, request_hook: Callable[[trace_api.Span, HttpRequest], None] | None = None, response_hook: Callable[[trace_api.Span, HttpRequest, HttpResponse], None] | None = None, excluded_urls: str | None = None, **kwargs: Any) -> None: + def instrument_django(self, capture_headers: bool = False, is_sql_commentor_enabled: bool | None = None, request_hook: Callable[[trace_api.Span, HttpRequest], None] | None = None, response_hook: Callable[[trace_api.Span, HttpRequest, HttpResponse], None] | None = None, excluded_urls: str | None = None, instrument_ninja: bool = True, **kwargs: Any) -> None: """Instrument `django` so that spans are automatically created for each web request. Uses the diff --git a/logfire/_internal/integrations/django.py b/logfire/_internal/integrations/django.py index d2f24d08c..ed2d85f1d 100644 --- a/logfire/_internal/integrations/django.py +++ b/logfire/_internal/integrations/django.py @@ -3,7 +3,7 @@ from typing import Any, Callable from django.http import HttpRequest, HttpResponse -from opentelemetry.trace import Span +from opentelemetry.trace import Span, get_current_span from logfire._internal.utils import maybe_capture_server_headers @@ -16,6 +16,8 @@ " pip install 'logfire[django]'" ) +_LOGFIRE_INSTRUMENTED = '_logfire_instrumented' + def instrument_django( *, @@ -24,6 +26,7 @@ def instrument_django( excluded_urls: str | None, request_hook: Callable[[Span, HttpRequest], None] | None, response_hook: Callable[[Span, HttpRequest, HttpResponse], None] | None, + instrument_ninja: bool, **kwargs: Any, ) -> None: """Instrument the `django` module so that spans are automatically created for each web request. @@ -38,3 +41,37 @@ def instrument_django( response_hook=response_hook, **kwargs, ) + if instrument_ninja: + _instrument_django_ninja() + + +def _instrument_django_ninja() -> None: + """Patch NinjaAPI.on_exception at the class level to record exceptions on spans. + + Django Ninja catches exceptions before they propagate to Django's middleware, + which prevents OpenTelemetry's Django instrumentation from recording them. + """ + try: + from ninja import NinjaAPI + except ImportError: + return + + if getattr(NinjaAPI.on_exception, _LOGFIRE_INSTRUMENTED, False): + return + + original_on_exception = NinjaAPI.on_exception + + def patched_on_exception(self: Any, request: HttpRequest, exc: Exception) -> HttpResponse: + span = get_current_span() + try: + response = original_on_exception(self, request, exc) + except Exception: + if span.is_recording(): + span.record_exception(exc, escaped=True) + raise + if span.is_recording(): + span.record_exception(exc, escaped=False) + return response + + patched_on_exception._logfire_instrumented = True # type: ignore[attr-defined] + NinjaAPI.on_exception = patched_on_exception # type: ignore[assignment] diff --git a/logfire/_internal/main.py b/logfire/_internal/main.py index 472910163..4c9edfb17 100644 --- a/logfire/_internal/main.py +++ b/logfire/_internal/main.py @@ -1603,6 +1603,7 @@ def instrument_django( request_hook: Callable[[trace_api.Span, HttpRequest], None] | None = None, response_hook: Callable[[trace_api.Span, HttpRequest, HttpResponse], None] | None = None, excluded_urls: str | None = None, + instrument_ninja: bool = True, **kwargs: Any, ) -> None: """Instrument `django` so that spans are automatically created for each web request. @@ -1631,6 +1632,11 @@ def instrument_django( excluded_urls: A string containing a comma-delimited list of regexes used to exclude URLs from tracking. + instrument_ninja: Set to `True` (the default) to also instrument Django Ninja's + `NinjaAPI.on_exception` so that exceptions caught by Django Ninja are recorded + on OpenTelemetry spans. Requires `django-ninja` to be installed; silently + skipped if not available. + **kwargs: Additional keyword arguments to pass to the OpenTelemetry `instrument` method, for future compatibility. @@ -1644,6 +1650,7 @@ def instrument_django( request_hook=request_hook, response_hook=response_hook, excluded_urls=excluded_urls, + instrument_ninja=instrument_ninja, **{ 'tracer_provider': self._config.get_tracer_provider(), 'meter_provider': self._config.get_meter_provider(), diff --git a/pyproject.toml b/pyproject.toml index 2de689058..4b00597c9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -111,6 +111,7 @@ dev = [ "fastapi != 0.124.3, != 0.124.4", "Flask >= 3.0.3", "django >= 4.2.16", + "django-ninja >= 1.0.0", "dirty-equals >= 0.8.0", "pytest >= 8.3.4", "pytest-django >= 4.6.0", @@ -312,6 +313,8 @@ filterwarnings = [ "ignore:Call to 'get_connection' function with deprecated usage of input argument/s.*:DeprecationWarning", # OpenTelemetry event logger stuff being used by Pydantic AI "ignore:You should use `\\w*(L|l)ogger\\w*` instead. Deprecated since version 1.39.0 and will be removed in a future release.:DeprecationWarning", + # django-ninja uses deprecated asyncio.iscoroutinefunction on Python 3.14+ + "ignore:'asyncio.iscoroutinefunction' is deprecated.*:DeprecationWarning", ] DJANGO_SETTINGS_MODULE = "tests.otel_integrations.django_test_project.django_test_site.settings" diff --git a/tests/otel_integrations/django_test_project/django_test_app/views.py b/tests/otel_integrations/django_test_project/django_test_app/views.py index 40bf509ea..bcd5ee7a1 100644 --- a/tests/otel_integrations/django_test_project/django_test_app/views.py +++ b/tests/otel_integrations/django_test_project/django_test_app/views.py @@ -1,5 +1,9 @@ from django.core.exceptions import BadRequest from django.http import HttpRequest, HttpResponse +from ninja import NinjaAPI +from ninja.errors import HttpError + +ninja_api = NinjaAPI(urls_namespace='ninja') def detail(_request: HttpRequest, item_id: int) -> HttpResponse: @@ -8,3 +12,18 @@ def detail(_request: HttpRequest, item_id: int) -> HttpResponse: def bad(_request: HttpRequest) -> HttpResponse: raise BadRequest('bad request') + + +@ninja_api.get('/good/') +def ninja_good(request: HttpRequest) -> dict[str, str]: + return {'message': 'ok'} + + +@ninja_api.get('/error/') +def ninja_error(request: HttpRequest) -> dict[str, str]: + raise HttpError(400, 'ninja error') + + +@ninja_api.get('/unhandled/') +def ninja_unhandled(request: HttpRequest) -> dict[str, str]: + raise RuntimeError('unhandled ninja error') diff --git a/tests/otel_integrations/django_test_project/django_test_site/urls.py b/tests/otel_integrations/django_test_project/django_test_site/urls.py index 40b47fe80..e4d46faec 100644 --- a/tests/otel_integrations/django_test_project/django_test_site/urls.py +++ b/tests/otel_integrations/django_test_project/django_test_site/urls.py @@ -1,7 +1,10 @@ from django.contrib import admin from django.urls import include, path # type: ignore +from tests.otel_integrations.django_test_project.django_test_app.views import ninja_api + urlpatterns = [ path('django_test_app/', include('django_test_app.urls')), + path('ninja/', ninja_api.urls), path('admin/', admin.site.urls), ] diff --git a/tests/otel_integrations/test_django_ninja.py b/tests/otel_integrations/test_django_ninja.py new file mode 100644 index 000000000..4e1bee8cf --- /dev/null +++ b/tests/otel_integrations/test_django_ninja.py @@ -0,0 +1,128 @@ +import pytest +from django.http import HttpResponse +from django.test import Client +from inline_snapshot import snapshot + +import logfire +from logfire.testing import TestExporter +from tests.otel_integrations.django_test_project.django_test_app.views import ninja_api + + +@pytest.fixture(autouse=True) +def _restore_ninja_api(): # pyright: ignore[reportUnusedFunction] + """Restore the original on_exception method after each test.""" + from ninja import NinjaAPI + + original = NinjaAPI.on_exception + yield + NinjaAPI.on_exception = original + + +def test_ninja_good_route(client: Client, exporter: TestExporter): + logfire.instrument_django() + response: HttpResponse = client.get('/ninja/good/') # type: ignore + assert response.status_code == 200 + + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 1 + # No exception events for successful requests + assert 'events' not in spans[0] or spans[0].get('events') == [] + + +def test_ninja_error_route_without_instrumentation(client: Client, exporter: TestExporter): + """Without instrument_ninja, handled exceptions are NOT recorded on spans.""" + logfire.instrument_django(instrument_ninja=False) + response: HttpResponse = client.get('/ninja/error/') # type: ignore + assert response.status_code == 400 + + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 1 + # No exception events — Django Ninja handled the exception before OTel could see it + assert spans[0].get('events') is None or spans[0].get('events') == [] + + +def test_ninja_error_route_with_instrumentation(client: Client, exporter: TestExporter): + """With instrument_ninja=True (default), handled exceptions ARE recorded on spans.""" + logfire.instrument_django() + response: HttpResponse = client.get('/ninja/error/') # type: ignore + assert response.status_code == 400 + + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 1 + + # Verify the exception was recorded on the span + events = spans[0].get('events', []) + exception_events = [e for e in events if e['name'] == 'exception'] + assert len(exception_events) == 1 + assert exception_events[0]['attributes']['exception.type'] == 'ninja.errors.HttpError' + assert exception_events[0]['attributes']['exception.message'] == 'ninja error' + assert exception_events[0]['attributes']['exception.escaped'] == 'False' + + +def test_ninja_unhandled_error_with_instrumentation(client: Client, exporter: TestExporter): + """Unhandled exceptions (RuntimeError) are also recorded on spans.""" + logfire.instrument_django() + client.raise_request_exception = False + response: HttpResponse = client.get('/ninja/unhandled/') # type: ignore + assert response.status_code == 500 + + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 1 + + events = spans[0].get('events', []) + exception_events = [e for e in events if e['name'] == 'exception'] + # At least one exception event should be recorded (could be 2: our hook + OTel middleware) + assert len(exception_events) >= 1 + assert any(e['attributes']['exception.type'] == 'RuntimeError' for e in exception_events) + assert any(e['attributes']['exception.message'] == 'unhandled ninja error' for e in exception_events) + # Our hook records escaped=True because Django Ninja re-raises unhandled exceptions + our_events = [ + e + for e in exception_events + if e['attributes']['exception.type'] == 'RuntimeError' and e['attributes'].get('exception.escaped') == 'True' + ] + assert len(our_events) >= 1 + + +def test_double_instrumentation(client: Client, exporter: TestExporter): + """Calling instrument_django twice should not double-wrap on_exception.""" + logfire.instrument_django() + logfire.instrument_django() + response: HttpResponse = client.get('/ninja/error/') # type: ignore + assert response.status_code == 400 + + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 1 + events = spans[0].get('events', []) + exception_events = [e for e in events if e['name'] == 'exception'] + # Should only record the exception once, not twice + assert len(exception_events) == 1 + + +def test_ninja_error_with_head_sample_rate_zero(client: Client, exporter: TestExporter): + """When head_sample_rate=0, spans are not recording and exceptions should not be recorded.""" + from logfire.sampling import SamplingOptions + + logfire.configure(sampling=SamplingOptions(head=0.0)) + logfire.instrument_django() + response: HttpResponse = client.get('/ninja/error/') # type: ignore + assert response.status_code == 400 + + # No spans should be exported when sampling rate is 0 + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 0 + + +def test_ninja_unhandled_error_with_head_sample_rate_zero(client: Client, exporter: TestExporter): + """When head_sample_rate=0, unhandled exceptions should still propagate correctly.""" + from logfire.sampling import SamplingOptions + + logfire.configure(sampling=SamplingOptions(head=0.0)) + logfire.instrument_django() + client.raise_request_exception = False + response: HttpResponse = client.get('/ninja/unhandled/') # type: ignore + assert response.status_code == 500 + + # No spans should be exported when sampling rate is 0 + spans = exporter.exported_spans_as_dict(parse_json_attributes=True) + assert len(spans) == 0 diff --git a/uv.lock b/uv.lock index 766efd1be..ddeb630cc 100644 --- a/uv.lock +++ b/uv.lock @@ -669,7 +669,7 @@ name = "cffi" version = "2.0.0" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "pycparser", version = "2.23", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10' and implementation_name != 'PyPy'" }, + { name = "pycparser", version = "2.23", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10' and implementation_name != 'PyPy' and platform_python_implementation != 'PyPy'" }, { name = "pycparser", version = "3.0", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.10' and implementation_name != 'PyPy'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/eb/56/b1ba7935a17738ae8453301356628e8147c79dbb825bcbc73dc7401f9846/cffi-2.0.0.tar.gz", hash = "sha256:44d1b5909021139fe36001ae048dbdde8214afa20200eda0f64c068cac5d5529", size = 523588, upload-time = "2025-09-08T23:24:04.541Z" } @@ -1426,6 +1426,21 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/96/ba/a6e2992bc5b8c688249c00ea48cb1b7a9bc09839328c81dc603671460928/django-6.0.2-py3-none-any.whl", hash = "sha256:610dd3b13d15ec3f1e1d257caedd751db8033c5ad8ea0e2d1219a8acf446ecc6", size = 8339381, upload-time = "2026-02-03T13:50:15.501Z" }, ] +[[package]] +name = "django-ninja" +version = "1.5.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "django", version = "4.2.28", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10'" }, + { name = "django", version = "5.2.11", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.10' and python_full_version < '3.12'" }, + { name = "django", version = "6.0.2", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.12'" }, + { name = "pydantic" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/02/84/27a5fceac29bd85eb8dc8a6697e93019a8742d626180f0d67b894e20a8a1/django_ninja-1.5.3.tar.gz", hash = "sha256:974803944965ad0566071633ffd4999a956f2ad1ecbed815c0de37c1c969592b", size = 3658996, upload-time = "2026-01-10T20:02:23.821Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/f4/b3/30600696c2532fcf026259f2f4980b364cb6847518bb4b3365d42a4a3afe/django_ninja-1.5.3-py3-none-any.whl", hash = "sha256:0a6ead5b4e57ec1050b584eb6f36f105f256b8f4ac70d12e774d8b6dd91e2198", size = 2365685, upload-time = "2026-01-10T20:02:21.484Z" }, +] + [[package]] name = "dnspython" version = "2.7.0" @@ -1531,7 +1546,7 @@ name = "exceptiongroup" version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "typing-extensions", marker = "python_full_version < '3.13'" }, + { name = "typing-extensions", marker = "python_full_version < '3.11'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/50/79/66800aadf48771f6b62f7eb014e352e5d06856655206165d775e675a02c9/exceptiongroup-1.3.1.tar.gz", hash = "sha256:8b412432c6055b0b7d14c310000ae93352ed6754f70fa8f7c34141f91c4e3219", size = 30371, upload-time = "2025-11-21T23:01:54.787Z" } wheels = [ @@ -3264,6 +3279,9 @@ django = [ { name = "opentelemetry-instrumentation-asgi" }, { name = "opentelemetry-instrumentation-django" }, ] +django-ninja = [ + { name = "django-ninja" }, +] dspy = [ { name = "openinference-instrumentation-dspy", version = "0.1.31", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10'" }, { name = "openinference-instrumentation-dspy", version = "0.1.33", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.10'" }, @@ -3343,6 +3361,7 @@ dev = [ { name = "django", version = "4.2.28", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10'" }, { name = "django", version = "5.2.11", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.10' and python_full_version < '3.12'" }, { name = "django", version = "6.0.2", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.12'" }, + { name = "django-ninja" }, { name = "dspy", marker = "python_full_version >= '3.10'" }, { name = "eval-type-backport" }, { name = "fastapi", version = "0.128.8", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.10'" }, @@ -3458,6 +3477,7 @@ docs = [ [package.metadata] requires-dist = [ + { name = "django-ninja", marker = "extra == 'django-ninja'", specifier = ">=1.0.0" }, { name = "executing", specifier = ">=2.0.1" }, { name = "httpx", marker = "extra == 'datasets'", specifier = ">=0.27.2" }, { name = "openinference-instrumentation-dspy", marker = "extra == 'dspy'", specifier = ">=0" }, @@ -3499,7 +3519,7 @@ requires-dist = [ { name = "tomli", marker = "python_full_version < '3.11'", specifier = ">=2.0.1" }, { name = "typing-extensions", specifier = ">=4.1.0" }, ] -provides-extras = ["system-metrics", "asgi", "wsgi", "aiohttp", "aiohttp-client", "aiohttp-server", "celery", "django", "fastapi", "flask", "httpx", "starlette", "sqlalchemy", "asyncpg", "psycopg", "psycopg2", "pymongo", "redis", "requests", "mysql", "sqlite3", "aws-lambda", "google-genai", "litellm", "dspy", "datasets", "variables"] +provides-extras = ["system-metrics", "asgi", "wsgi", "aiohttp", "aiohttp-client", "aiohttp-server", "celery", "django", "django-ninja", "fastapi", "flask", "httpx", "starlette", "sqlalchemy", "asyncpg", "psycopg", "psycopg2", "pymongo", "redis", "requests", "mysql", "sqlite3", "aws-lambda", "google-genai", "litellm", "dspy", "datasets", "variables"] [package.metadata.requires-dev] dev = [ @@ -3517,6 +3537,7 @@ dev = [ { name = "cryptography", specifier = ">=44.0.0" }, { name = "dirty-equals", specifier = ">=0.8.0" }, { name = "django", specifier = ">=4.2.16" }, + { name = "django-ninja", specifier = ">=1.0.0" }, { name = "dspy", marker = "python_full_version >= '3.10'", specifier = ">=3.1.0" }, { name = "eval-type-backport", specifier = ">=0.2.0" }, { name = "fastapi", specifier = "!=0.124.3,!=0.124.4" },