Conversation
There was a problem hiding this comment.
Pull request overview
Adds correlation ID tracking and structured exception logging middleware for Application Insights integration.
Changes:
- New
CorrelationIdMiddlewarethat propagates/generates correlation IDs via context vars and response headers - New
ExceptionLoggingMiddlewarethat logs unhandled exceptions with structured context to Application Insights - Updated logging configuration with correlation ID filter, duplicate suppression filter, and formatter changes
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
manage_breast_screening/core/middleware/exception_logging.py |
New middleware for correlation ID and exception logging |
manage_breast_screening/core/tests/middleware/test_exception_logging.py |
Tests for the new middleware classes |
manage_breast_screening/core/services/application_insights_logging.py |
Updated exception method signature, logger name, and OTEL env var |
manage_breast_screening/core/apps.py |
Renamed app config from Notifications to Core |
manage_breast_screening/config/settings.py |
Registered middleware and logging filters/formatters |
manage_breast_screening/core/tests/services/test_application_insights_logging.py |
Updated test for new exception method signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| class SuppressHandled500s: | ||
| """ | ||
| Django's `django.request` logger logs every unhandled view exception at `ERROR` level | ||
| automatically. `ExceptionLoggingMiddleware.process_exception` then logs the same exception | ||
| again. Both reach Application Insights, creating duplicates in the `exceptions` / `traces` | ||
| tables and inflating alert counts. | ||
| """ | ||
|
|
||
| def filter(self, record): | ||
| request = getattr(record, "request", None) | ||
|
|
||
| if request and getattr(request, "_exception_logged", False): | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
The SuppressHandled500s filter class has no test coverage. It should be tested to verify it suppresses records when _exception_logged is set and allows records through otherwise.
| ApplicationInsightsLogging, | ||
| ) | ||
|
|
||
| app_insights_logger = ApplicationInsightsLogging() |
There was a problem hiding this comment.
ApplicationInsightsLogging() is instantiated at module level, which triggers __init__ (including setting OTEL_SERVICE_NAME env var) at import time. This is a side effect that runs whenever the module is imported, including during tests and management commands. Consider lazy initialization or moving the instantiation into a function.
swebberuk
force-pushed
the
DTOSS-12264-app-insights-2
branch
3 times, most recently
from
0fb80bf to
eee4dcd
Compare
Added middleware for correlation ID and exception logging Registered middleware in settings.py Added core/apps.py
swebberuk
force-pushed
the
DTOSS-12264-app-insights-2
branch
from
eee4dcd to
8ec9869
Compare
|
|
Close. Use #1153 instead. |
Added middleware for correlation ID and exception logging
Registered middleware in settings.py
Added core/apps.py
Description
Jira link
Review notes
Review checklist