feat: add VOUCH_LOG_FORMAT=json structured logging#113
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements structured JSON logging via a new VOUCH_LOG_FORMAT=json, centralizes startup configuration in src/vouch/logging_config.py, applies it at CLI, JSONL, and stdio entrypoints, and adds tests plus a CHANGELOG entry. ChangesStructured JSON Logging Configuration
Sequence Diagram(s)sequenceDiagram
participant CLI
participant JSONLServer
participant StdioServer
participant LoggingConfig
participant stderr
CLI->>LoggingConfig: call configure_logging()
JSONLServer->>LoggingConfig: call configure_logging()
StdioServer->>LoggingConfig: call configure_logging()
LoggingConfig->>stderr: attach StreamHandler(sys.stderr) with chosen formatter
LoggingConfig->>stderr: emit formatted log lines (JSON/text)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_logging_config.py (1)
7-35: ⚡ Quick winAdd explicit coverage for
VOUCH_LOG_LEVELthreshold behavior.The current tests validate format switching, but not whether
VOUCH_LOG_LEVELactually filters records as intended.Suggested test addition
+def test_log_level_from_env_filters_lower_levels(monkeypatch, capsys): + monkeypatch.setenv("VOUCH_LOG_FORMAT", "json") + monkeypatch.setenv("VOUCH_LOG_LEVEL", "ERROR") + from vouch.logging_config import configure_logging + configure_logging(force=True) + + logger = logging.getLogger("vouch.test") + logger.warning("should be filtered") + logger.error("should appear") + + captured = capsys.readouterr() + lines = [ln for ln in captured.err.strip().splitlines() if ln] + assert lines, "Expected at least one log line" + last = json.loads(lines[-1]) + assert last["level"] == "ERROR" + assert last["message"] == "should appear"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_logging_config.py` around lines 7 - 35, Add a new test in tests/test_logging_config.py that verifies VOUCH_LOG_LEVEL actually filters log records: use monkeypatch.setenv("VOUCH_LOG_LEVEL", "<LEVEL>") with configure_logging(force=True) from vouch.logging_config, then emit messages via logging.getLogger("vouch.test").warning(...) and .error(...) and capture output with capsys to assert that messages below the threshold (e.g., WARNING when level is ERROR) are not present and messages at/above the threshold are present; repeat for at least two thresholds (e.g., "ERROR" and "WARNING") to cover both filtered and allowed cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/logging_config.py`:
- Around line 21-30: The JSON formatter's format method currently only
serializes a fixed set of fields and drops LogRecord extras (e.g., event, actor,
object_ids); update the format method (the formatter class's format function) to
merge user-provided extra fields from record.__dict__ into the obj before
json.dumps by copying record.__dict__, removing standard LogRecord attributes
(like name, msg, args, levelname, levelno, pathname, lineno, funcName, created,
msecs, relativeCreated, thread, threadName, processName, exc_info, stack_info)
and any private keys, then update obj with the remaining extras so all
structured context fields are preserved in the JSON output.
---
Nitpick comments:
In `@tests/test_logging_config.py`:
- Around line 7-35: Add a new test in tests/test_logging_config.py that verifies
VOUCH_LOG_LEVEL actually filters log records: use
monkeypatch.setenv("VOUCH_LOG_LEVEL", "<LEVEL>") with
configure_logging(force=True) from vouch.logging_config, then emit messages via
logging.getLogger("vouch.test").warning(...) and .error(...) and capture output
with capsys to assert that messages below the threshold (e.g., WARNING when
level is ERROR) are not present and messages at/above the threshold are present;
repeat for at least two thresholds (e.g., "ERROR" and "WARNING") to cover both
filtered and allowed cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a605936-bddc-432b-8ae7-0e338e9f2da5
📒 Files selected for processing (6)
CHANGELOG.mdsrc/vouch/cli.pysrc/vouch/jsonl_server.pysrc/vouch/logging_config.pysrc/vouch/server.pytests/test_logging_config.py
| def format(self, record: logging.LogRecord) -> str: | ||
| obj = { | ||
| "time": self.formatTime(record, "%Y-%m-%dT%H:%M:%S"), | ||
| "level": record.levelname, | ||
| "logger": record.name, | ||
| "message": record.getMessage(), | ||
| } | ||
| if record.exc_info: | ||
| obj["exc_info"] = self.formatException(record.exc_info) | ||
| return json.dumps(obj, separators=(",", ":")) |
There was a problem hiding this comment.
Preserve structured context fields in JSON logs.
At Line 22, the formatter only serializes fixed fields, so LogRecord extras like event, actor, and object_ids are dropped in VOUCH_LOG_FORMAT=json output.
Suggested patch
class _JsonFormatter(logging.Formatter):
"""Emit one JSON object per log record."""
def format(self, record: logging.LogRecord) -> str:
obj = {
"time": self.formatTime(record, "%Y-%m-%dT%H:%M:%S"),
"level": record.levelname,
"logger": record.name,
"message": record.getMessage(),
}
+ for key in ("event", "actor", "object_ids"):
+ if key in record.__dict__:
+ obj[key] = record.__dict__[key]
if record.exc_info:
obj["exc_info"] = self.formatException(record.exc_info)
- return json.dumps(obj, separators=(",", ":"))
+ return json.dumps(obj, separators=(",", ":"), default=str)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/logging_config.py` around lines 21 - 30, The JSON formatter's
format method currently only serializes a fixed set of fields and drops
LogRecord extras (e.g., event, actor, object_ids); update the format method (the
formatter class's format function) to merge user-provided extra fields from
record.__dict__ into the obj before json.dumps by copying record.__dict__,
removing standard LogRecord attributes (like name, msg, args, levelname,
levelno, pathname, lineno, funcName, created, msecs, relativeCreated, thread,
threadName, processName, exc_info, stack_info) and any private keys, then update
obj with the remaining extras so all structured context fields are preserved in
the JSON output.
922ceff to
7e0f6b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_logging_config.py (1)
7-22: ⚡ Quick winJSON test bypasses the env-driven configuration path.
On Line 8 you set
VOUCH_LOG_FORMAT, but on Line 11 the test directly uses_JsonFormatter, so it won’t catch regressions inconfigure_logging()selection/handler wiring.Proposed update
def test_json_format_emits_valid_json(monkeypatch, capsys): monkeypatch.setenv("VOUCH_LOG_FORMAT", "json") - from vouch.logging_config import _JsonFormatter - handler = logging.StreamHandler() - handler.setFormatter(_JsonFormatter()) - record = logging.LogRecord( - name="vouch.test", level=logging.WARNING, - pathname="", lineno=0, msg="test message", - args=(), exc_info=None, - ) - line = handler.format(record) + from vouch.logging_config import configure_logging + root = logging.getLogger() + old_handlers = root.handlers[:] + old_level = root.level + try: + configure_logging(force=True) + root.warning("test message") + captured = capsys.readouterr() + line = captured.err.strip().splitlines()[-1] + finally: + root.handlers.clear() + root.handlers.extend(old_handlers) + root.setLevel(old_level) obj = json.loads(line) assert obj["level"] == "WARNING" assert obj["message"] == "test message" assert "time" in obj assert "logger" in obj🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_logging_config.py` around lines 7 - 22, The test test_json_format_emits_valid_json currently instantiates _JsonFormatter directly and thus bypasses the env-driven wiring; change it to set VOUCH_LOG_FORMAT to "json", call configure_logging() (the function that selects and wires handlers), obtain a logger (e.g., logging.getLogger("vouch.test")), emit a log at WARNING, capture the output from the configured handler, and then json.loads that output to assert "level", "message", "time", and "logger" keys; reference symbols: test_json_format_emits_valid_json, configure_logging, and _JsonFormatter to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_logging_config.py`:
- Around line 40-41: In the except json.JSONDecodeError handler, replace the use
of the bare assert (the line that currently triggers an assertion failure for
"Expected non-JSON output") with an explicit test failure by raising an
AssertionError or calling pytest.fail so the test fails under python -O; update
the test in tests/test_logging_config.py accordingly.
- Line 4: Remove the redundant imports in tests/test_logging_config.py: delete
the unused top-level "import os" statement and remove the local "import logging"
within the test body (the module already imports logging at top-level), ensuring
no duplicate imports remain and import-order lint (F401) is resolved; update any
references if needed to rely on the module-level logging import.
---
Nitpick comments:
In `@tests/test_logging_config.py`:
- Around line 7-22: The test test_json_format_emits_valid_json currently
instantiates _JsonFormatter directly and thus bypasses the env-driven wiring;
change it to set VOUCH_LOG_FORMAT to "json", call configure_logging() (the
function that selects and wires handlers), obtain a logger (e.g.,
logging.getLogger("vouch.test")), emit a log at WARNING, capture the output from
the configured handler, and then json.loads that output to assert "level",
"message", "time", and "logger" keys; reference symbols:
test_json_format_emits_valid_json, configure_logging, and _JsonFormatter to
locate the code to modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c35cf31-7c30-46d2-ac34-535f6dbfbbc8
📒 Files selected for processing (6)
CHANGELOG.mdsrc/vouch/cli.pysrc/vouch/jsonl_server.pysrc/vouch/logging_config.pysrc/vouch/server.pytests/test_logging_config.py
✅ Files skipped from review due to trivial changes (3)
- src/vouch/jsonl_server.py
- src/vouch/server.py
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vouch/cli.py
- src/vouch/logging_config.py
b9998a9 to
8714c63
Compare
|
@plind-junior , review please |
|
@plind-junior worth flagging before merge: the acceptance criteria on #97 calls for level, event, actor, object_ids per log line, but this formatter only emits {time, level, logger, message} and drops anything passed via the stdlib extra= argument (CodeRabbit's first review noted the same thing). So actor and object_ids won't actually appear in the output even if callers attach them. |
|
PR should target test branch |
8714c63 to
d4df5d8
Compare
All done man! |
f19cbc3 to
b10e69f
Compare
|
@plind-junior , please check |
All conflicts resolved |
|
@plind-junior ?? |
b10e69f to
a3cec83
Compare
|
Rebased onto upstream/test and expanded the implementation:
|
|
Additional advantages of this approach over the previous implementation:
|
|
Fix the conflict |
…LEVEL/FILE, and LoggingConfig return type Combines the best of both PR vouchdev#113 and vouchdev#114 into a superior implementation: From vouchdev#114 (Khaostica): - Scoped vouch logger namespace instead of root logger - _STDLIB_RECORD_FIELDS explicit whitelist for extra= merging - event key with caller override via extra={event: ...} - sort_keys=True, default=str in json.dumps - propagate=False in json mode to prevent double-emit - _selected_format() helper Added on top: - time field in JSON output (ISO 8601) - VOUCH_LOG_LEVEL env var: sets vouch logger level (default WARNING) - VOUCH_LOG_FILE env var: appends logs to file alongside stderr - Sensitive field redaction: keys containing secret/token/password/key are replaced with ***REDACTED*** so credentials never reach log sinks - LoggingConfig named tuple return type for testability and introspection - force=True parameter for clean reconfiguration in tests - CONTRIBUTING.md: documents all env vars, logging guidelines, and managed-handler pattern - 11 tests covering all new behaviour Fixes vouchdev#97
15d11e5 to
7d3b1af
Compare
…LEVEL/FILE, and LoggingConfig return type Combines the best of both PR vouchdev#113 and vouchdev#114 into a superior implementation: From vouchdev#114 (Khaostica): - Scoped vouch logger namespace instead of root logger - _STDLIB_RECORD_FIELDS explicit whitelist for extra= merging - event key with caller override via extra={event: ...} - sort_keys=True, default=str in json.dumps - propagate=False in json mode to prevent double-emit - _selected_format() helper Added on top: - time field in JSON output (ISO 8601) - VOUCH_LOG_LEVEL env var: sets vouch logger level (default WARNING) - VOUCH_LOG_FILE env var: appends logs to file alongside stderr - Sensitive field redaction: keys containing secret/token/password/key are replaced with ***REDACTED*** so credentials never reach log sinks - LoggingConfig named tuple return type for testability and introspection - force=True parameter for clean reconfiguration in tests - CONTRIBUTING.md: documents all env vars, logging guidelines, and managed-handler pattern - 11 tests covering all new behaviour Fixes vouchdev#97
|
Rebased onto latest upstream/test — all conflicts resolved, 265 tests passing, make check clean. Also updated tests/test_logging.py (Khaostica's existing tests) to match our expanded return type — configure_logging() now returns a LoggingConfig named tuple instead of a plain string, so assertions updated from All 265 tests passing. Make check clean. |
|
I guess this i perfect now, and ready to get merged @plind-junior |
ReviewSummary: Implements What works
Suggestions
VerdictRequest changes — The logging implementation itself ( |
|
Convert it to "ready to merge" once the issue has been resolved |
Got it |
Implements VOUCH_LOG_FORMAT=json for issue vouchdev#97. Scoped to logging only. - JsonFormatter: vouch logger namespace, event key with caller override, _STDLIB_RECORD_FIELDS whitelist, sort_keys=True, default=str, time field - Sensitive field redaction: keys containing secret/token/password/key replaced with ***REDACTED*** in JSON output - VOUCH_LOG_LEVEL: sets vouch logger level (default WARNING) - VOUCH_LOG_FILE: appends logs to file alongside stderr; bad path emits a warning to stderr and is skipped rather than crashing the server - LoggingConfig named tuple: reflects effective installed state on early-return (inspects installed handlers, not requested env) - _VouchManagedHandler subclass: host handlers never removed - Text mode is a true no-op unless VOUCH_LOG_FILE is set - force=True parameter for clean reconfiguration in tests - CONTRIBUTING.md: env var table and logging guidelines - 18 tests covering all behaviour; 429 passing total Fixes vouchdev#97
7d3b1af to
a59d3f6
Compare
|
@plind-junior , all set now |
|
@plind-junior ,you may give this a check likewise too |
|
@plind-junior , can you review this now |
|
From now on, lets focus on building desktop version before v2.0.0 release |
3 participants
Summary
VOUCH_LOG_FORMATis referenced in ROADMAP andadapters/generic-mcp/README.mdbut was never implemented anywhere insrc/. Running vouch as a service made logs unparseable by log aggregators and monitoring tools.Changes
Add
src/vouch/logging_config.pywith aconfigure_logging()function that readsVOUCH_LOG_FORMATat process startup:"text"— human-readable format, no behavior change for existing users"json"— one JSON object per log line:{"time", "level", "logger", "message"}Also supports
VOUCH_LOG_LEVEL(defaults toWARNING) for controlling verbosity.configure_logging()is called from all three entrypoints:cli()group incli.py— covers allvouchCLI commandsrun_stdio()inserver.py— MCP stdio serverrun_jsonl()injsonl_server.py— JSONL tool serverTesting
Two new tests in
tests/test_logging_config.py:test_json_format_emits_valid_json— verifies JSON output shapetest_text_format_is_default— verifies human-readable output when unset156 existing tests passing, no regressions.
Closes #97
Summary by CodeRabbit
New Features
Tests
Documentation