Refactor logging configuration: streamline JSON formatting #37
Conversation
…ontext handling, and improve code clarity
WalkthroughRefactors logging context handling in services/logging_config.py by simplifying JsonFormatter attribute merging via an excluded set, changing RequestLoggerAdapter extra/context merge to preserve existing keys, updating get/set context access patterns, removing a main-guarded setup block, and making minor formatting/logic tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application Code
participant Adp as RequestLoggerAdapter.process
participant Log as Logger/Handler
participant Fmt as JsonFormatter.format
participant Sink as Output (e.g., stdout)
App->>Adp: logger.info(msg, extra=...)
note right of Adp: Merge order (no overwrite):<br/>1) kwargs["extra"]<br/>2) adapter extras<br/>3) per-request context (_log_context.get())
Adp->>Log: emits LogRecord
Log->>Fmt: format(record)
note right of Fmt: base.update(_log_context.get())<br/>Record attrs merged excluding named set
Fmt-->>Log: JSON string
Log-->>Sink: write
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| extra: Dict[str, Any] = kwargs.setdefault("extra", {}) | ||
| extra.update({k: v for k, v in self.extra.items() if k not in extra}) | ||
| extra.update({k: v for k, v in _log_context.get().items() if k not in extra}) | ||
| return msg, kwargs |
There was a problem hiding this comment.
Avoid mutating caller-supplied extra
Line [131] grabs kwargs["extra"] and updates it in place, so any dict the caller passes (or reuses) now comes back polluted with adapter/context entries. That’s a behavioral regression: reuse of the same extra dict across log calls will leak stale request context into later logs, and even within the same call the caller sees unexpected mutations. Please copy the provided mapping before merging:
- extra: Dict[str, Any] = kwargs.setdefault("extra", {})
- extra.update({k: v for k, v in self.extra.items() if k not in extra})
- extra.update({k: v for k, v in _log_context.get().items() if k not in extra})
+ provided_extra = kwargs.get("extra") or {}
+ extra: Dict[str, Any] = dict(provided_extra)
+ for k, v in self.extra.items():
+ extra.setdefault(k, v)
+ for k, v in _log_context.get().items():
+ extra.setdefault(k, v)
+ kwargs["extra"] = extra📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extra: Dict[str, Any] = kwargs.setdefault("extra", {}) | |
| extra.update({k: v for k, v in self.extra.items() if k not in extra}) | |
| extra.update({k: v for k, v in _log_context.get().items() if k not in extra}) | |
| return msg, kwargs | |
| provided_extra = kwargs.get("extra") or {} | |
| extra: Dict[str, Any] = dict(provided_extra) | |
| for k, v in self.extra.items(): | |
| extra.setdefault(k, v) | |
| for k, v in _log_context.get().items(): | |
| extra.setdefault(k, v) | |
| kwargs["extra"] = extra | |
| return msg, kwargs |
🤖 Prompt for AI Agents
In services/logging_config.py around lines 131 to 134, the code mutates the
caller-supplied kwargs["extra"] in place which leaks adapter/context entries
into caller-owned dicts; instead create a fresh dict by copying the provided
mapping (e.g. new_extra = dict(kwargs.get("extra", {})) or use
dict(kwargs["extra"]) if present), update that new dict with self.extra and
_log_context entries, and assign it back to kwargs["extra"] so the original
caller dict is not modified.
Summary by CodeRabbit