-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add request id to django and flask libraries and propagate event context to celery tasks #216
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
Conversation
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.
Pull Request Overview
This PR introduces request ID tracking across Django, Flask, and Celery, ensuring that request_id
is captured or generated at the start of web requests and propagated into event contexts and Celery tasks. It also restarts the Honeybadger events worker in each Celery forked process.
- Inject
request_id
from headers or middleware into Honeybadger event context for Flask and Django. - Pass
correlation_context
(containingrequest_id
) through payload creation and notices. - Update Celery plugin to embed and restore event context in task headers and restart the events worker thread after forking.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
honeybadger/tests/*.py | Add tests for correlation context, request ID propagation, and worker init behavior. |
honeybadger/payload.py | Support correlation_context in create_payload . |
honeybadger/notice.py | Add request_id field and include it in _correlation_context . |
honeybadger/core.py | Extract request_id from event context and pass to Notice . |
honeybadger/events_worker.py | Allow restarting the batch worker thread after process fork. |
honeybadger/contrib/flask.py | Generate or extract X-Request-ID and set event context. |
honeybadger/contrib/django.py | Extract or generate request_id and set event context. |
honeybadger/contrib/celery.py | Propagate event context into task headers; restart events worker; use signals instead of polling. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
request_id = ( | ||
getattr(request, "id", None) | ||
or getattr(request, "request_id", None) | ||
or request.headers.get("X-Request-ID", None) |
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.
Since we don't have control of the contents of the X-Request-ID header, and since it is going to be passed along to our API, this value should be truncated to a reasonable length and characters outside of the set used in UUIDs ([a-z0-9-]+) should be stripped.
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.
Good call. I decided to sanitize at the points of collection, as that felt like the best place since we store it in the event, use it for sampling (in the other PR), and it goes into the notice.
honeybadger/contrib/flask.py
Outdated
@@ -201,6 +202,12 @@ def _initialize_honeybadger(self, config): | |||
def _handle_request_started(self, sender, *args, **kwargs): | |||
from flask import request | |||
|
|||
request_id = request.headers.get("X-Request-ID") |
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.
Ditto the previous comment about sanitizing this value.
@@ -80,6 +80,8 @@ def notify( | |||
merged_ctx = {**base, **(context or {})} | |||
merged_tags = list({*tag_ctx, *(tags or [])}) | |||
|
|||
request_id = self._get_event_context().get("request_id", None) |
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.
Perhaps it would be best to centralize the sanitization here. :)
This PR adds request ID tracking for both Django and Flask.
We inject the
request_id
into the event_context to be propagated to all emitted events. For Django, we first check the request object for anyid
orrequest_id
attributes, in case some middleware has already created a request_id. This also inserts thecorrelation_context
into error notices.I also updated the Celery plugin to automatically rehydrate any parent event context into the worker before it starts processing tasks.
There is a bit of code reworking the Celery worker. I had an issue where events would not be sent in forked workers. I initially thought the only solution was to route events from worker processes back to the main process (where the batch worker was still alive), but that was overcomplicating things. Really, each worker process just needed its own working batch worker thread. I also realized that calling
honeybadger.event
in a worker did not work with my previous implementation.When Celery forks worker processes, they inherit a copy of the batch worker object but the actual thread dies during the fork. So we had workers trying to queue events to a dead thread. Instead of building a complex IPC system to send events back to the main process, the fix was to just restart the batch worker thread in each worker process. Now every worker has its own live thread that can actually send events, and they all work independently without needing to communicate with the parent process at all.