Add TRACE logging level for verbose payload logging across event writers #61
Conversation
oto-macenauer-absa
requested review from
ABMC831,
Zejnilovic and
petr-pokorny-absa
as code owners
WalkthroughAdds TRACE logging support and Logger.trace; introduces safe, PII‑redacting, size‑bounded serialization and a trace helper used by writers. EventBridge, Kafka, and Postgres writers now emit TRACE payload logs. Minor import restructuring in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Writer as Writer (EventBridge/Kafka/Postgres)
participant TraceUtil as trace_logging.log_payload_at_trace
participant Serializer as safe_serialize_for_log
participant Logger as logging.Logger (TRACE)
participant Backend as Backend Client
Caller->>Writer: write(topic, message)
Writer->>TraceUtil: log_payload_at_trace(logger, WriterName, topic, message)
TraceUtil->>Logger: isEnabledFor(TRACE_LEVEL)?
alt TRACE enabled
TraceUtil->>Serializer: safe_serialize_for_log(message)
alt serialization OK
Serializer-->>TraceUtil: safe_payload
TraceUtil->>Logger: trace("<Writer> payload topic=<topic> payload=<safe_payload>")
else serialization fails
TraceUtil->>Logger: trace("<Writer> payload topic=<topic> <unserializable>")
end
else TRACE disabled
Note over TraceUtil,Writer: skip serialization & TRACE logging
end
Writer->>Backend: send/produce/execute(topic, message)
Backend-->>Writer: result
Writer-->>Caller: (ok / err)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
oto-macenauer-absa
force-pushed
the
feature/trace-logging
branch
from
7898832 to
ad4d5d7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/logging_levels.py (1)
12-19: Fix idempotency guard; expose constant onloggingfor reuse.
hasattr(logging, "TRACE")is always False because the attribute is never set. This can re‑register on repeated imports. Switch to a stable check and set the constant for convenience.Apply this diff:
-if not hasattr(logging, "TRACE"): - logging.addLevelName(TRACE_LEVEL, "TRACE") +if logging.getLevelName(TRACE_LEVEL) != "TRACE": + logging.addLevelName(TRACE_LEVEL, "TRACE") + logging.TRACE = TRACE_LEVEL # convenience constant and future idempotency guard - - def trace(self: logging.Logger, message: str, *args, **kws): # type: ignore[override] + def trace(self: logging.Logger, message: str, *args, **kws): if self.isEnabledFor(TRACE_LEVEL): self._log(TRACE_LEVEL, message, args, **kws) # pylint: disable=protected-accesssrc/writer_kafka.py (1)
97-105: Narrow exception; avoid blind catch in logging path.Catching
Exceptiontrips BLE001 and can hide programming errors. Limit to JSON serialization errors.Apply this diff:
- if logger.isEnabledFor(TRACE_LEVEL): - try: - logger.trace( # type: ignore[attr-defined] - "Kafka payload topic=%s payload=%s", topic_name, json.dumps(message, separators=(",", ":")) - ) - except Exception: # pragma: no cover - defensive - logger.trace("Kafka payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined] + if logger.isEnabledFor(TRACE_LEVEL): + try: + logger.trace( # type: ignore[attr-defined] + "Kafka payload topic=%s payload=%s", + topic_name, + json.dumps(message, separators=(",", ":"), ensure_ascii=False), + ) + except (TypeError, ValueError, OverflowError): # pragma: no cover - defensive + logger.trace("Kafka payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined]src/writer_postgres.py (1)
277-285: Narrow exception; add safe JSON options.Mirror the narrower handling used elsewhere and avoid BLE001.
Apply this diff:
- if _logger.isEnabledFor(TRACE_LEVEL): - try: - _logger.trace( # type: ignore[attr-defined] - "Postgres payload topic=%s payload=%s", topic_name, json.dumps(message, separators=(",", ":")) - ) - except Exception: # pragma: no cover - defensive - _logger.trace("Postgres payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined] + if _logger.isEnabledFor(TRACE_LEVEL): + try: + _logger.trace( # type: ignore[attr-defined] + "Postgres payload topic=%s payload=%s", + topic_name, + json.dumps(message, separators=(",", ":"), ensure_ascii=False), + ) + except (TypeError, ValueError, OverflowError): # pragma: no cover - defensive + _logger.trace("Postgres payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined]src/writer_eventbridge.py (2)
29-32: Remove redundant side‑effect import.
from .logging_levels import TRACE_LEVELalready executes the module and registers the level; the extrafrom . import logging_levels # noqa: F401is unnecessary and triggers RUF100.Apply this diff:
-# Ensure TRACE level is registered -from . import logging_levels # noqa: F401 -from .logging_levels import TRACE_LEVEL +# Ensure TRACE level is registered +from .logging_levels import TRACE_LEVEL
75-83: Narrow exception; align JSON options.Limit the catch to JSON serialization errors and keep payload readable.
Apply this diff:
- if logger.isEnabledFor(TRACE_LEVEL): - try: - logger.trace( # type: ignore[attr-defined] - "EventBridge payload topic=%s payload=%s", topic_name, json.dumps(message, separators=(",", ":")) - ) - except Exception: # pragma: no cover - defensive serialization guard - logger.trace("EventBridge payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined] + if logger.isEnabledFor(TRACE_LEVEL): + try: + logger.trace( # type: ignore[attr-defined] + "EventBridge payload topic=%s payload=%s", + topic_name, + json.dumps(message, separators=(",", ":"), ensure_ascii=False), + ) + except (TypeError, ValueError, OverflowError): # pragma: no cover - defensive serialization guard + logger.trace("EventBridge payload topic=%s <unserializable>", topic_name) # type: ignore[attr-defined]src/event_gate_lambda.py (1)
46-51: Tighten import guard; register name in fallback.Use
ImportErrorinstead of broadException, drop the unusednoqa, and ensure the level name is registered even in fallback soLOG_LEVEL=TRACEworks.Apply this diff:
-# Register custom TRACE level before using LOG_LEVEL env var -try: - from .logging_levels import TRACE_LEVEL # noqa: F401 -except Exception: # pragma: no cover - defensive - TRACE_LEVEL = 5 # type: ignore +# Register custom TRACE level before using LOG_LEVEL env var +try: + from .logging_levels import TRACE_LEVEL +except ImportError: # pragma: no cover - defensive + TRACE_LEVEL = 5 # type: ignore + logging.addLevelName(TRACE_LEVEL, "TRACE")tests/test_trace_logging.py (2)
24-42: Tidy test doubles to silence arg/noqalint nits.Rename unused args and drop the unused
noqaonflush.Apply this diff:
- class FakeProducer: - def produce(self, *a, **kw): + class FakeProducer: + def produce(self, *_, **__): cb = kw.get("callback") if cb: cb(None, object()) - def flush(self, *a, **kw): # noqa: D401 + def flush(self, *_, **__): return 0
71-75: Same nit on DummyPsycopg2.connect.Apply this diff:
- class DummyPsycopg2: - def connect(self, **kwargs): # noqa: D401 + class DummyPsycopg2: + def connect(self, **_): return DummyConnection()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/event_gate_lambda.py(1 hunks)src/logging_levels.py(1 hunks)src/writer_eventbridge.py(2 hunks)src/writer_kafka.py(2 hunks)src/writer_postgres.py(2 hunks)tests/test_trace_logging.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/writer_kafka.py (1)
src/logging_levels.py (1)
trace(15-17)
tests/test_trace_logging.py (3)
src/writer_eventbridge.py (1)
write(55-108)src/writer_kafka.py (1)
write(81-134)src/writer_postgres.py (1)
write(260-311)
src/writer_eventbridge.py (1)
src/logging_levels.py (1)
trace(15-17)
src/writer_postgres.py (1)
src/logging_levels.py (1)
trace(15-17)
🪛 Ruff (0.12.2)
src/writer_kafka.py
103-103: Do not catch blind exception: Exception
(BLE001)
tests/test_trace_logging.py
26-26: Unused method argument: a
(ARG002)
31-31: Unused method argument: a
(ARG002)
31-31: Unused method argument: kw
(ARG002)
31-31: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
72-72: Unused method argument: kwargs
(ARG002)
72-72: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
src/writer_eventbridge.py
30-30: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
81-81: Do not catch blind exception: Exception
(BLE001)
src/event_gate_lambda.py
48-48: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
49-49: Do not catch blind exception: Exception
(BLE001)
src/writer_postgres.py
283-283: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
src/writer_kafka.py (1)
29-31: Import looks good.Side-effect import via
from .logging_levels import TRACE_LEVELis sufficient to register the level and method.src/writer_postgres.py (1)
34-36: TRACE registration import is correct.Ensures
logger.traceexists before use.
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
I did leave some comments to understand your solution, write down some recommendation easy to fix based on linter. I would think about comments from code rabbit since this way of logging can create new AquaSec issues, which we will have to solve soon.
oto-macenauer-absa
force-pushed
the
feature/trace-logging
branch
from
ad4d5d7 to
c53ec19
Compare
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/safe_serialization.py (1)
27-84: LGTM! Robust safe serialization implementation.The implementation is well-designed with solid defensive practices:
- Recursive redaction correctly handles nested dicts and lists (lines 37-44)
- Case-insensitive key matching prevents bypass via casing variations (line 66)
- UTF-8-safe truncation with
errors="ignore"prevents broken multibyte characters (line 79)- Sensible defaults for redaction keys and size limits via environment variables (lines 60, 63)
- Graceful error handling returns empty string on serialization failures (lines 83-84)
Optional: Minor code organization improvement
Line 82 could be moved into an
elseblock for slightly clearer control flow (currently falls through after try/except in the non-truncation case):if len(serialized.encode("utf-8")) > max_bytes: # Binary truncate to max_bytes and append marker truncated_bytes = serialized.encode("utf-8")[:max_bytes] # Ensure we don't break mid-multibyte character try: return truncated_bytes.decode("utf-8", errors="ignore") + "..." except UnicodeDecodeError: # pragma: no cover - defensive return "" - return serialized + else: + return serialized
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/event_gate_lambda.py(1 hunks)src/logging_levels.py(1 hunks)src/safe_serialization.py(1 hunks)src/trace_logging.py(1 hunks)src/writer_eventbridge.py(2 hunks)src/writer_kafka.py(2 hunks)src/writer_postgres.py(2 hunks)tests/test_safe_serialization.py(1 hunks)tests/test_trace_logging.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/logging_levels.py
- src/writer_eventbridge.py
- src/writer_postgres.py
- src/event_gate_lambda.py
🧰 Additional context used
🧬 Code graph analysis (4)
src/trace_logging.py (2)
src/safe_serialization.py (1)
safe_serialize_for_log(47-84)src/logging_levels.py (1)
trace(15-25)
tests/test_safe_serialization.py (1)
src/safe_serialization.py (1)
safe_serialize_for_log(47-84)
src/writer_kafka.py (1)
src/trace_logging.py (1)
log_payload_at_trace(29-48)
tests/test_trace_logging.py (3)
src/writer_eventbridge.py (1)
write(53-99)src/writer_kafka.py (1)
write(80-126)src/writer_postgres.py (1)
write(259-303)
🪛 Ruff (0.14.3)
src/safe_serialization.py
82-82: Consider moving this statement to an else block
(TRY300)
tests/test_safe_serialization.py
43-43: Possible hardcoded password assigned to: "password"
(S105)
51-51: Possible hardcoded password assigned to: "password"
(S105)
60-60: Possible hardcoded password assigned to: "Password"
(S105)
62-62: Possible hardcoded password assigned to: "Token"
(S105)
75-75: Possible hardcoded password assigned to: "password"
(S105)
90-90: Possible hardcoded password assigned to: "password"
(S105)
92-92: Possible hardcoded password assigned to: "secret"
(S105)
117-117: Possible hardcoded password assigned to: "password"
(S105)
118-118: Possible hardcoded password assigned to: "secret"
(S105)
140-140: Local variable parsed_or_truncated is assigned to but never used
Remove assignment to unused variable parsed_or_truncated
(F841)
146-146: Possible hardcoded password assigned to: "password"
(S105)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
172-172: Possible hardcoded password assigned to: "password"
(S105)
186-186: Possible hardcoded password assigned to: "password"
(S105)
195-195: Possible hardcoded password assigned to: "password"
(S105)
197-197: Possible hardcoded password assigned to: "token"
(S105)
217-217: Possible hardcoded password assigned to: "password"
(S105)
219-219: Possible hardcoded password assigned to: "secret"
(S105)
220-220: Possible hardcoded password assigned to: "secret"
(S105)
242-242: Possible hardcoded password assigned to: "password"
(S105)
244-244: Possible hardcoded password assigned to: "secret"
(S105)
tests/test_trace_logging.py
26-26: Unused method argument: a
(ARG002)
31-31: Unused method argument: a
(ARG002)
31-31: Unused method argument: kw
(ARG002)
31-31: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
72-72: Unused method argument: kwargs
(ARG002)
72-72: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (4)
src/writer_kafka.py (1)
29-29: LGTM! TRACE logging integration is clean.The import and call to
log_payload_at_tracecorrectly integrate TRACE-level payload logging before the Kafka write operation. The past review concerns about PII exposure and log volume are now addressed by the safe serialization utility (redaction + size capping viasafe_serialize_for_log).Also applies to: 96-96
src/trace_logging.py (1)
29-49: LGTM! Well-designed trace logging utility.The implementation is solid with good defensive practices:
- Early return optimization (lines 38-39) avoids serialization overhead when TRACE is disabled
- Exception handling (lines 47-48) provides a graceful fallback for unserializable payloads
- Clear, concise logging format with writer name and topic context
tests/test_safe_serialization.py (1)
28-244: Excellent test coverage for safe serialization.The test suite comprehensively validates:
- Redaction behavior (single/multiple keys, case-insensitive, nested structures, lists)
- Truncation mechanics (including multibyte UTF-8 safety)
- Environment-driven configuration defaults
- Error handling for unserializable objects
- Edge cases (empty messages, non-dict inputs, non-string values)
This thorough coverage provides strong confidence in the safe serialization utility.
tests/test_trace_logging.py (1)
10-86: LGTM! Effective TRACE logging validation.The test suite effectively validates TRACE-level payload logging across all three writers (EventBridge, Kafka, Postgres):
- Minimal mocks/stubs (MagicMock, FakeProducer, DummyPsycopg2) avoid real I/O while exercising the trace logging path
- Caplog integration correctly captures and verifies TRACE-level log output
- Each test confirms the writer-specific payload log message is emitted
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_trace_logging.py (2)
31-32: Remove the unusednoqadirective.The
# noqa: D401comment is unnecessary since D401 is not enabled in your Ruff configuration.Apply this diff:
- def flush(self, *a, **kw): # noqa: D401 + def flush(self, *a, **kw): return 0
71-73: Remove the unusednoqadirective.The
# noqa: D401comment is unnecessary since D401 is not enabled in your Ruff configuration.Apply this diff:
- def connect(self, **kwargs): # noqa: D401 + def connect(self, **kwargs): return DummyConnection()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/writer_postgres.py(8 hunks)tests/test_trace_logging.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/writer_postgres.py (2)
src/trace_logging.py (1)
log_payload_at_trace(29-48)src/writer_eventbridge.py (1)
init(34-44)
tests/test_trace_logging.py (3)
src/writer_postgres.py (1)
write(259-303)src/writer_eventbridge.py (1)
write(53-99)src/writer_kafka.py (1)
write(80-126)
🪛 Ruff (0.14.3)
src/writer_postgres.py
299-299: Use explicit conversion flag
Replace with conversion flag
(RUF010)
300-300: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_trace_logging.py
26-26: Unused method argument: a
(ARG002)
31-31: Unused method argument: a
(ARG002)
31-31: Unused method argument: kw
(ARG002)
72-72: Unused method argument: kwargs
(ARG002)
72-72: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (6)
tests/test_trace_logging.py (2)
10-22: LGTM!The EventBridge trace logging test is well-structured: it properly configures the TRACE level, injects the necessary state into the writer module, mocks the AWS client, and verifies that the payload appears in the captured logs.
44-86: LGTM!The Postgres trace logging test is well-designed: it constructs a complete dummy psycopg2 stack (connection, cursor, context managers), uses monkeypatch to inject it, configures the module state, and validates that the trace payload appears in the logs.
src/writer_postgres.py (4)
34-34: LGTM!The import of
log_payload_at_tracecorrectly integrates the trace logging functionality introduced in this PR.
52-52: LGTM!Renaming
_loggertologger(making it public) aligns with the pattern used in other writer modules and properly addresses the previous review question about why the logger was private.
56-77: LGTM!The
initfunction changes are well-executed: renaming the parameter tologger_instanceavoids shadowing the module-levelloggervariable, and all references are consistently updated from_loggertologger.
276-277: LGTM!The trace payload logging is correctly positioned before the database connection and follows the same pattern as the EventBridge and Kafka writers, ensuring consistent trace-level diagnostics across all backends.
oto-macenauer-absa
force-pushed
the
feature/trace-logging
branch
from
8ce7f4d to
e9a8712
Compare
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/writer_postgres.py (1)
298-301: Consider usinglogger.exceptionfor better diagnostics.The exception handler currently uses
logger.error, which omits the stack trace. Usinglogger.exceptionwould automatically include the full trace, making it easier to debug database connectivity or query issues.Apply this diff:
except (RuntimeError, PsycopgError) as e: # narrowed exception set err_msg = f"The Postgres writer with failed unknown error: {str(e)}" - logger.error(err_msg) + logger.exception(err_msg) return False, err_msg
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/event_gate_lambda.py(1 hunks)src/logging_levels.py(1 hunks)src/safe_serialization.py(1 hunks)src/trace_logging.py(1 hunks)src/writer_eventbridge.py(2 hunks)src/writer_kafka.py(2 hunks)src/writer_postgres.py(8 hunks)tests/test_safe_serialization.py(1 hunks)tests/test_trace_logging.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/logging_levels.py
- src/writer_eventbridge.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/trace_logging.py (2)
src/safe_serialization.py (1)
safe_serialize_for_log(47-84)src/logging_levels.py (1)
trace(15-25)
src/writer_kafka.py (1)
src/trace_logging.py (1)
log_payload_at_trace(29-48)
tests/test_safe_serialization.py (1)
src/safe_serialization.py (1)
safe_serialize_for_log(47-84)
tests/test_trace_logging.py (3)
src/writer_kafka.py (1)
write(82-142)src/writer_postgres.py (1)
write(259-303)src/writer_eventbridge.py (1)
write(53-99)
src/writer_postgres.py (3)
src/trace_logging.py (1)
log_payload_at_trace(29-48)src/writer_kafka.py (1)
init(46-79)src/writer_eventbridge.py (1)
init(34-44)
🪛 Ruff (0.14.3)
tests/test_safe_serialization.py
43-43: Possible hardcoded password assigned to: "password"
(S105)
51-51: Possible hardcoded password assigned to: "password"
(S105)
60-60: Possible hardcoded password assigned to: "Password"
(S105)
62-62: Possible hardcoded password assigned to: "Token"
(S105)
75-75: Possible hardcoded password assigned to: "password"
(S105)
90-90: Possible hardcoded password assigned to: "password"
(S105)
92-92: Possible hardcoded password assigned to: "secret"
(S105)
117-117: Possible hardcoded password assigned to: "password"
(S105)
118-118: Possible hardcoded password assigned to: "secret"
(S105)
145-145: Possible hardcoded password assigned to: "password"
(S105)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Possible hardcoded password assigned to: "password"
(S105)
185-185: Possible hardcoded password assigned to: "password"
(S105)
194-194: Possible hardcoded password assigned to: "password"
(S105)
196-196: Possible hardcoded password assigned to: "token"
(S105)
216-216: Possible hardcoded password assigned to: "password"
(S105)
218-218: Possible hardcoded password assigned to: "secret"
(S105)
219-219: Possible hardcoded password assigned to: "secret"
(S105)
241-241: Possible hardcoded password assigned to: "password"
(S105)
243-243: Possible hardcoded password assigned to: "secret"
(S105)
tests/test_trace_logging.py
26-26: Unused method argument: a
(ARG002)
31-31: Unused method argument: a
(ARG002)
31-31: Unused method argument: kw
(ARG002)
72-72: Unused method argument: kwargs
(ARG002)
72-72: Unused noqa directive (non-enabled: D401)
Remove unused noqa directive
(RUF100)
src/safe_serialization.py
82-82: Consider moving this statement to an else block
(TRY300)
src/writer_postgres.py
299-299: Use explicit conversion flag
Replace with conversion flag
(RUF010)
300-300: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (10)
src/writer_kafka.py (2)
29-29: LGTM!Clean import of the new trace logging helper, correctly used at line 97.
97-97: LGTM!Trace payload logging is correctly placed before the Kafka send operation and matches the pattern used in other writers.
src/event_gate_lambda.py (1)
28-42: LGTM!Import restructuring improves clarity by making writer module imports explicit. The UnsupportedAlgorithm import is correctly used in the exception handler at line 103.
tests/test_safe_serialization.py (1)
1-243: Excellent test coverage!The test suite comprehensively validates safe serialization behavior including redaction, truncation, environment defaults, and edge cases. Well-structured and thorough.
tests/test_trace_logging.py (1)
1-86: LGTM!The tests validate TRACE-level payload logging across all three writers (EventBridge, Kafka, Postgres) with appropriate mocks and stubs. Good integration test coverage.
src/trace_logging.py (1)
29-48: LGTM!Clean implementation with appropriate early-return optimization and defensive error handling. The integration with safe serialization ensures PII protection and size bounds.
src/writer_postgres.py (2)
52-77: LGTM!Logger refactor from private
_loggerto publicloggeris clean and consistent. The parameter rename tologger_instanceappropriately avoids shadowing the module variable.
276-276: LGTM!Trace payload logging correctly placed after skip checks and before database connection, consistent with the pattern in other writers.
src/safe_serialization.py (2)
27-44: LGTM!Clean recursive implementation correctly handles nested dictionaries and lists with case-insensitive key matching. Non-mutating design is appropriate.
47-84: Excellent implementation!Robust serialization with sensible defaults, UTF-8-safe truncation, and comprehensive error handling. The defensive approach ensures logging never crashes even with problematic payloads.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/writer_postgres.py (1)
299-299: Consider using explicit conversion flag in f-string.While
str(e)works correctly, f-strings idiomatically use the!sconversion flag for string conversion.Apply this diff:
- err_msg = f"The Postgres writer with failed unknown error: {str(e)}" + err_msg = f"The Postgres writer with failed unknown error: {e!s}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/writer_postgres.py(8 hunks)tests/test_trace_logging.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/writer_postgres.py (1)
src/trace_logging.py (1)
log_payload_at_trace(29-48)
tests/test_trace_logging.py (3)
src/writer_postgres.py (1)
write(259-303)src/writer_kafka.py (1)
write(82-142)src/writer_eventbridge.py (1)
write(53-99)
🪛 Ruff (0.14.3)
src/writer_postgres.py
299-299: Use explicit conversion flag
Replace with conversion flag
(RUF010)
tests/test_trace_logging.py
26-26: Unused method argument: a
(ARG002)
31-31: Unused method argument: a
(ARG002)
31-31: Unused method argument: kw
(ARG002)
72-72: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (8)
src/writer_postgres.py (4)
34-34: LGTM: Import and logger declaration.The import of
log_payload_at_traceis consistent with the trace logging infrastructure. The logger naming change from_loggertologger(removing the underscore prefix) follows Python conventions and improves API consistency.Also applies to: 52-52
56-77: LGTM: Init signature refactoring.Renaming the parameter from
loggertologger_instanceavoids shadowing the module-levelloggervariable and makes the assignment intent clearer. The docstring and implementation are properly updated.
88-88: LGTM: Logging calls updated consistently.All logging calls have been correctly updated to use the renamed
loggervariable. Line 300 properly useslogger.exceptionfor better error diagnostics, as addressed in previous reviews.Also applies to: 147-147, 227-227, 270-270, 273-273, 294-294, 300-300
276-276: LGTM: Trace logging integration.The trace logging call is correctly positioned after availability checks and before the actual database write, consistent with the pattern in other writers (EventBridge, Kafka). The trace helper handles PII redaction and size bounds as designed.
tests/test_trace_logging.py (4)
1-8: LGTM: Clean import structure.All necessary imports are present and properly organized. The test imports the custom TRACE_LEVEL and all three writer modules for comprehensive coverage.
10-21: LGTM: EventBridge trace logging test.The test properly configures the EventBridge writer state, mocks the client, and validates both the success of the write operation and the presence of trace payload logs. Clean and effective test design.
24-41: LGTM: Kafka trace logging test.The
FakeProducerstub correctly implements the minimal interface needed for testing, including callback invocation for success simulation and flush behavior. The test validates both write success and trace log emission. The unused argument warnings from Ruff are false positives—stub methods must match the real interface signature.
44-86: LGTM: Postgres trace logging test.The dummy psycopg2 implementation is comprehensive and clean, with proper context manager support. The test correctly monkeypatches dependencies, configures state, and validates both write success and trace log emission. The message payload matches the requirements of
postgres_test_write. The unusedkwargswarning is a false positive—connectmust accept multiple connection parameters.
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
Great, thanks for the changes. If you have time, I have one more recommendation to create some source structure. The project is getting bigger and we should probably create a file for writers/ and logging/ or utils/ keeping the event_gate_lambda.py script as main one.
I've create a follow up issue for the project structure: #77 |
This pull request introduces a new custom TRACE logging level (below DEBUG) for highly verbose payload logging, and integrates it into the EventBridge, Kafka, and Postgres writer modules. It also includes comprehensive tests to ensure TRACE-level logging works as intended across all three writers. The main themes are the addition of the TRACE level, its integration into the writers, and the new tests for this functionality.
Custom TRACE logging level:
TRACE_LEVEL(level 5) and a correspondingLogger.trace()method inlogging_levels.py, ensuring idempotent registration and compatibility with the standard logging module.event_gate_lambda.py,writer_eventbridge.py,writer_kafka.py, andwriter_postgres.pyto ensure the TRACE level is registered before use. [1] [2] [3] [4]Integration of TRACE logging in writers:
writer_eventbridge.py,writer_kafka.py, andwriter_postgres.py, added TRACE-level payload logging before sending messages, including defensive handling for unserializable payloads. [1] [2] [3]Testing:
test_trace_logging.pyto verify that TRACE-level logging is triggered and correctly logs payloads for EventBridge, Kafka, and Postgres writers, using mocks and monkeypatching as needed.Release notes
Related
Summary by CodeRabbit
New Features
Tests