fix(server): omit absent request-log ids instead of logging them empty (#301) - #633
fix(server): omit absent request-log ids instead of logging them empty (#301)#633chethanuk wants to merge 1 commit into
Conversation
RequestLogContext::emit and emit_cancelled rendered session_id and
correlation_id with .unwrap_or(""), collapsing None and Some("") onto the
same session_id= output. That is a lossy projection: an operator reading
the request log cannot tell a request that sent no session from one that
sent a blank session.
Drop .unwrap_or("") at both sites. tracing-core's impl<T: Value> Value for
Option<T> skips None, so an absent id omits the field entirely while
Some("x") still renders session_id=x byte-identically to before.
requested_model, selected_model and error keep unwrap_or("") on purpose:
emit_cancelled hardcodes selected_model = "", so they mean "absent implies
empty" by design.
Widen the test subscriber to capture event fields as well as the message,
and add a table test over the three id shapes (absent, empty, present).
The absent row failed before the fix with left: Some(""), right: None.
Refs NVIDIA-NeMo#301
Signed-off-by: ChethanUK <chethanuk@outlook.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughRequest logging now preserves absent session and correlation IDs as missing fields. Completed and cancelled request tests capture all tracing fields and verify absent, empty, and populated identifier values. The changelog documents the fix. ChangesRequest logging
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Request logs now omit absent session and correlation IDs while retaining explicitly empty and populated values, improving diagnostic clarity without an identified current-head merge risk. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. (1 skipped: 1 unsupported.)
Comment |
What
.unwrap_or("")onsession_id/correlation_idinRequestLogContext::emitandemit_cancelled, so an absent id omits the field instead of logging it as empty.Why
The terminal request log renders
session_id=for both "no session was sent" and "an empty session was sent" — an operator can't tell them apart. This is part 3 of #301; parts 1 and 2 landed in #308.Closes #301.
Notes for reviewers
requested_model,selected_model, anderrorkeepunwrap_or("")on purpose:emit_cancelledhardcodesselected_model = "", so those three mean "absent implies empty" by design. The only non-test producer ofsession_id(resolve_path) never returnsSome("")today, so the empty-string test row is a regression guard rather than a live bug — the user-visible change is that an absent session stops printingsession_id=.Test plan
cargo test -p switchyard-server request_log— new test fails today (left: Some(""), right: None), 4 passed after the fixcargo fmt --all --checkcargo clippy -p switchyard-server --all-targets -- -D warningscargo test --workspace— 586 passed, 0 failed, 1 ignoredcargo clippy --workspacefails oncrates/prefill-router(chunks_exact), pre-existing and untouched here — reproduces onorigin/mainalone.Summary by CodeRabbit
Bug Fixes
Documentation