-
Notifications
You must be signed in to change notification settings - Fork 214
feat: add Django Ninja exception recording to instrument_django() #1746
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Behavioral change: The Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| **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(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Wrong exception recorded when a Django Ninja exception handler itself raises
In
patched_on_exception, theexcept Exceptionblock always records the originalexcparameter, but the exception that actually escapes may be different.Root Cause
Django Ninja's
on_exception(ninja/main.py) can raise a different exception thanexcwhen a registered exception handler itself fails:In the patched version at
logfire/_internal/integrations/django.py:68-70:If
handler(request, exc)raises aValueError, the code records the originalexc(e.g.HttpError) on the span withescaped=True, but theraisepropagates theValueError. The trace shows the wrong exception type and message.The fix is to capture and record the actually-raised exception:
Impact: Users inspecting spans would see a misleading exception type/message that doesn't match the actual error that propagated.
Was this helpful? React with 👍 or 👎 to provide feedback.