feat: add Django Ninja exception recording to instrument_django()#1746
feat: add Django Ninja exception recording to instrument_django()#1746Br1an67 wants to merge 1 commit intopydantic:mainfrom
Conversation
0f27bc5 to
6fb28cd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| from opentelemetry.trace import get_current_span | ||
|
|
||
| original_on_exception = api.on_exception |
There was a problem hiding this comment.
- We need this to work without requiring passing arguments, for the
logfire runcommand. If you think it's useful to be able to specifyapiinstead of instrumenting everything globally, then that can be an option, but it's not essential. In this case, that means patchingNinjaAPI.on_exceptionon the class. - Please make this part of
instrument_djangoinstead of adding another method and requiring users to call both. - Please mention this in the django docs.
| if getattr(api.on_exception, _LOGFIRE_INSTRUMENTED, False): | ||
| return | ||
|
|
||
| from opentelemetry.trace import get_current_span |
| span.record_exception(exc, escaped=False) | ||
| return response | ||
|
|
||
| patched_on_exception._logfire_instrumented = True # type: ignore[attr-defined] |
There was a problem hiding this comment.
use constant for consistency
| try: | ||
| response = original_on_exception(request, exc) | ||
| except Exception: | ||
| if span.is_recording(): # pragma: no branch |
There was a problem hiding this comment.
test these branches by setting the head sample rate to 0
pyproject.toml
Outdated
| aiohttp-server = ["opentelemetry-instrumentation-aiohttp-server >= 0.55b0"] | ||
| celery = ["opentelemetry-instrumentation-celery >= 0.42b0"] | ||
| django = ["opentelemetry-instrumentation-django >= 0.42b0", "opentelemetry-instrumentation-asgi >= 0.42b0"] | ||
| django-ninja = ["django-ninja >= 1.0.0"] |
There was a problem hiding this comment.
this extra isn't needed, we assume that users instrumenting a package have that package. same goes for catching the import error.
…go() 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
8989dc6 to
9072f0c
Compare
|
Thanks for the detailed review! I've restructured the PR completely based on your feedback:
Deleted the separate |
| except Exception: | ||
| if span.is_recording(): | ||
| span.record_exception(exc, escaped=True) |
There was a problem hiding this comment.
🟡 Wrong exception recorded when a Django Ninja exception handler itself raises
In patched_on_exception, the except Exception block always records the original exc parameter, but the exception that actually escapes may be different.
Root Cause
Django Ninja's on_exception (ninja/main.py) can raise a different exception than exc when a registered exception handler itself fails:
def on_exception(self, request, exc):
handler = self._lookup_exception_handler(exc)
if handler is None:
raise exc # same as exc — OK
return handler(request, exc) # handler could raise something else!In the patched version at logfire/_internal/integrations/django.py:68-70:
except Exception:
if span.is_recording():
span.record_exception(exc, escaped=True) # always records exc
raise # re-raises the *actual* exceptionIf handler(request, exc) raises a ValueError, the code records the original exc (e.g. HttpError) on the span with escaped=True, but the raise propagates the ValueError. The trace shows the wrong exception type and message.
The fix is to capture and record the actually-raised exception:
except Exception as raised_exc:
if span.is_recording():
span.record_exception(raised_exc, escaped=True)
raiseImpact: Users inspecting spans would see a misleading exception type/message that doesn't match the actual error that propagated.
| except Exception: | |
| if span.is_recording(): | |
| span.record_exception(exc, escaped=True) | |
| except Exception as raised_exc: | |
| if span.is_recording(): | |
| span.record_exception(raised_exc, escaped=True) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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, |
There was a problem hiding this comment.
🚩 Behavioral change: instrument_ninja=True by default for all existing instrument_django() callers
The instrument_ninja parameter defaults to True at logfire/_internal/main.py:1606. This means all existing users calling logfire.instrument_django() will now automatically get Django Ninja's NinjaAPI.on_exception monkey-patched at the class level if django-ninja is installed. While the ImportError is silently caught at logfire/_internal/integrations/django.py:56, users who DO have Django Ninja installed but don't use it with Logfire may be surprised by the class-level patching. This is likely intentional (as stated in the PR description and docstring), but worth a reviewer confirming this opt-out vs opt-in decision.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Integrates Django Ninja exception recording into
instrument_django()via a newinstrument_ninja=Trueparameter (enabled by default).Django Ninja catches exceptions before they propagate to Django's middleware, preventing OpenTelemetry's Django instrumentation from recording them. This patch hooks
NinjaAPI.on_exceptionat the class level to record exceptions on the current span.Closes #1742
Changes
logfire/_internal/integrations/django.py: Added_instrument_django_ninja()which patchesNinjaAPI.on_exceptionon the class. Silently skipped ifdjango-ninjais not installed. Uses_LOGFIRE_INSTRUMENTEDconstant for double-instrumentation guard.logfire/_internal/main.py: Addedinstrument_ninja: bool = Trueparameter toinstrument_django(). Removed separateinstrument_django_ninja()method.instrument_djangosignature withinstrument_ninjaparameter.head_sample_rate=0branches.django-ninjaoptional extra (users instrumenting Django Ninja are expected to have it installed).Key Design Decisions
logfire run)escaped=Truewhen Django Ninja re-raises unhandled exceptions,escaped=Falsewhen handledinstrument_django()flow