Skip to content

ENG-3006 Add request_id metadata to all logs#7691

Merged
erosselli merged 14 commits intomainfrom
erosselli/ENG-3006
Mar 19, 2026
Merged

ENG-3006 Add request_id metadata to all logs#7691
erosselli merged 14 commits intomainfrom
erosselli/ENG-3006

Conversation

@erosselli
Copy link
Contributor

@erosselli erosselli commented Mar 18, 2026

Ticket ENG-3006

Description Of Changes

Adds X-Request-ID header support so that all logs emitted during a single API request can be correlated. Clients can supply their own ID via the X-Request-ID request header, or the server generates a UUID automatically. The ID is returned in the X-Request-ID response header and injected into every log record.

How it works:

  • LogRequestMiddleware reads/generates the request ID and stores it in the existing RequestContext ContextVar
  • A Loguru patcher automatically injects request_id into every log record's extra dict — no changes needed to the ~2,900 existing logger.* call sites
  • Celery signal handlers (before_task_publish / task_prerun) propagate the request ID to worker processes
  • In JSON serialization mode (typical for production), the request_id appears as a searchable field in log aggregators (e.g. @record.extra.request_id in Datadog)

Code Changes

  • src/fides/api/request_context.py — Added request_id field to RequestContext dataclass with get/set helpers. Fixed pre-existing bug where set_request_context mutated a shared default object instead of creating a per-request copy.
  • src/fides/api/util/logger.py — Added _inject_request_context patcher that reads the ContextVar and injects request_id into every log record. Updated has_custom_extra filter to ignore patcher-injected keys.
  • src/fides/api/asgi_middleware.py — Updated LogRequestMiddleware to read/generate request ID, set ContextVar, and inject X-Request-ID response header. Added header_injecting_send helper to BaseASGIMiddleware.
  • src/fides/api/tasks/__init__.py — Added before_task_publish and task_prerun Celery signal handlers to propagate request ID to worker processes.

Steps to Confirm

  1. Make an API request without X-Request-ID header — confirm the response includes a generated X-Request-ID header
  2. Make an API request with X-Request-ID: test-123 — confirm test-123 appears in the response header and in log output
  3. Confirm that logs from different concurrent requests have different request_id values
  4. If JSON serialization is enabled, confirm request_id appears in the serialized log output under record.extra.request_id

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • No UX review needed
  • Followup issues:
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

@vercel
Copy link
Contributor

vercel bot commented Mar 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Mar 19, 2026 0:49am
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Mar 19, 2026 0:49am

Request Review

@erosselli erosselli marked this pull request as ready for review March 18, 2026 16:17
@erosselli erosselli requested a review from a team as a code owner March 18, 2026 16:17
@erosselli erosselli requested review from thabofletcher and removed request for a team March 18, 2026 16:17
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR adds X-Request-ID header support for correlating all log records emitted during a single API request. A request ID is read from (or generated for) each incoming request in LogRequestMiddleware, stored in the existing RequestContext ContextVar, and automatically injected into every Loguru record via a patcher — requiring zero changes at individual call sites. The ID is also propagated to Celery workers via before_task_publish/task_prerun signals and cleaned up via task_postrun. A pre-existing bug where set_request_context mutated the shared default RequestContext object is also fixed.

Key changes:

  • request_context.py — adds request_id field and get/set helpers; fixes shared-default mutation bug
  • asgi_middleware.py — reads/validates/generates request ID, sets ContextVar, wraps send to echo X-Request-ID in the response (including error responses)
  • util/logger.py — registers _inject_request_context patcher globally; updates has_custom_extra to ignore patcher-injected keys so text-mode formatting is unaffected
  • tasks/__init__.py — full propagation lifecycle: attach → restore → clear
  • Test concern: TestLoggerPatcher.test_patcher_injects_request_id and TestRequestIdMiddleware.test_request_id_in_log_output depend on the Loguru patcher being registered, but logger.configure(patcher=...) is only called inside setup(config), which is never invoked in the test environment. These tests are expected to fail until a test fixture registers the patcher.

Confidence Score: 3/5

  • The production code changes are solid; the test file has a fixture gap that will cause 3 tests to fail in CI until addressed.
  • The implementation across request_context.py, asgi_middleware.py, util/logger.py, and tasks/__init__.py is correct and well-structured. The primary blocker is that TestLoggerPatcher and test_request_id_in_log_output will fail because the Loguru patcher is never registered in the test context. This aligns with the PR's own note that CI pipelines haven't all passed yet. Once the patcher fixture is added, this PR should be ready to merge.
  • tests/api/middleware/test_request_id.py — needs a fixture to register the Loguru patcher before TestLoggerPatcher and TestRequestIdMiddleware.test_request_id_in_log_output run.

Important Files Changed

Filename Overview
src/fides/api/asgi_middleware.py Adds request ID generation/validation and response header injection to LogRequestMiddleware. Logic is sound — validation regex, header wrapping, and error-path header injection are all correct. Minor: \w in the regex is slightly over-permissive (matches Unicode letters, not just ASCII alphanumerics).
src/fides/api/request_context.py Adds request_id to the RequestContext dataclass and fixes the pre-existing mutation-of-shared-default bug in set_request_context. Clean and well-documented implementation.
src/fides/api/util/logger.py Registers _inject_request_context as a Loguru patcher and updates has_custom_extra to ignore patcher-injected keys. Import correctly moved to module top level. Logic is correct.
src/fides/api/tasks/init.py Adds before_task_publish, task_prerun, and task_postrun signal handlers to propagate and clean up request_id across Celery worker boundaries. All three signals (including the task_postrun cleanup) are present, preventing ID leakage between sequential tasks.
tests/api/middleware/test_request_id.py Good coverage of middleware, Celery signal, and context isolation behavior. However, TestLoggerPatcher and test_request_id_in_log_output rely on _inject_request_context being registered as a Loguru patcher, which never happens in the test context — these tests will fail.

Last reviewed commit: "fix test"

@erosselli
Copy link
Contributor Author

@greptile re-review

@erosselli
Copy link
Contributor Author

@greptile

@erosselli
Copy link
Contributor Author

@greptile

Copy link
Contributor

@thabofletcher thabofletcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! All tests pass from the PR description: I sent a over a doc with a few ideas

Merged via the queue into main with commit e0afc88 Mar 19, 2026
57 checks passed
@erosselli erosselli deleted the erosselli/ENG-3006 branch March 19, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants