perf(logging): render log lines with less work per field#3396
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (6)
Summary by CodeRabbit
WalkthroughChangesThe change gates admin UI log streaming by configuration, transfers span data on closure, and reworks logfmt field rendering and quoting classification with focused tests and Criterion benchmarks. Admin-gated logging
Logfmt rendering and benchmarks
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CarbideConfig
participant run
participant setup_logging
participant LogStreamLayer
CarbideConfig->>run: provide enable_admin_ui
run->>setup_logging: pass enable_admin_ui
setup_logging->>LogStreamLayer: install when enabled
LogStreamLayer-->>setup_logging: broadcast events and retain history
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/logfmt/benches/logfmt_layer.rs (1)
97-108: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider adding a "needs quoting" case to
bench_quoting_scan.Both benchmarked values are clean ASCII that never trip
ASCII_NEEDS_QUOTING, so only the "no quoting needed" fast path (which must scan the full value) is measured. Since quoting classification presumably short-circuits on match, adding a value that requires quoting (e.g. one containing=or whitespace) would round out coverage of the perf claim ("clean-ASCII quoting scans ~5x faster") with the contrasting case.🤖 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 `@crates/logfmt/benches/logfmt_layer.rs` around lines 97 - 108, The bench_quoting_scan benchmark only measures clean values that do not require quoting. Add a third benchmark input containing a quoting-triggering character such as '=' or whitespace, and call value_needs_quoting through black_box with a descriptive benchmark name to cover the short-circuit path alongside the existing clean ASCII cases.crates/logfmt/Cargo.toml (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
local-timefeature Keepenv-filter—crates/logfmt/src/lib.rsusestracing_subscriber::EnvFilter—butlocal-timeis not exercised here, so trim it unless another path depends on it.🤖 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 `@crates/logfmt/Cargo.toml` around lines 33 - 36, Remove the unused "local-time" feature from the tracing-subscriber dependency in Cargo.toml, retaining "env-filter" because it is used by tracing_subscriber::EnvFilter in lib.rs.crates/logfmt/src/lib.rs (1)
787-793: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the quote/escape decision to prevent silent divergence.
record_strreproduces the exact branch already expressed inrender_value(Line 759). While the inlining is well-justified for avoiding an intermediateString, maintaining two independent copies of the quoting contract risks them drifting apart under future edits, producing inconsistent log rendering between&strandDebug/error paths. An#[inline]helper accepting&strwould preserve the allocation-reuse behaviour while keeping a single source of truth.♻️ Illustrative extraction
+/// Renders a value the way `Kvp` emits it, from a borrowed slice. `#[inline]` +/// so `record_str` still renders straight into its quoted form. +#[inline] +fn render_value_str(value: &str) -> String { + if value_needs_quoting(value) { + format!("\"{}\"", value.escape_debug()) + } else { + value.to_string() + } +}fn record_str(&mut self, field: &Field, value: &str) { - let rendered = if value_needs_quoting(value) { - format!("\"{}\"", value.escape_debug()) - } else { - value.to_string() - }; + let rendered = render_value_str(value); self.fields.push(RenderedField::new(field, rendered)); }🤖 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 `@crates/logfmt/src/lib.rs` around lines 787 - 793, Consolidate the duplicated string quoting and escaping logic used by record_str and render_value into a shared #[inline] helper accepting &str. Preserve record_str’s allocation-reuse behavior while making both paths use the same quoting contract and removing the independent conditional branches.
🤖 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.
Nitpick comments:
In `@crates/logfmt/benches/logfmt_layer.rs`:
- Around line 97-108: The bench_quoting_scan benchmark only measures clean
values that do not require quoting. Add a third benchmark input containing a
quoting-triggering character such as '=' or whitespace, and call
value_needs_quoting through black_box with a descriptive benchmark name to cover
the short-circuit path alongside the existing clean ASCII cases.
In `@crates/logfmt/Cargo.toml`:
- Around line 33-36: Remove the unused "local-time" feature from the
tracing-subscriber dependency in Cargo.toml, retaining "env-filter" because it
is used by tracing_subscriber::EnvFilter in lib.rs.
In `@crates/logfmt/src/lib.rs`:
- Around line 787-793: Consolidate the duplicated string quoting and escaping
logic used by record_str and render_value into a shared #[inline] helper
accepting &str. Preserve record_str’s allocation-reuse behavior while making
both paths use the same quoting contract and removing the independent
conditional branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2550059-0005-4503-b291-170f30e97608
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/api-core/src/logging/setup.rscrates/api-core/src/logging/stream.rscrates/api-core/src/run.rscrates/logfmt/Cargo.tomlcrates/logfmt/benches/logfmt_layer.rscrates/logfmt/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/logfmt/src/lib.rs`:
- Around line 653-665: Add a CI workflow job for the crates/logfmt benchmark
target that runs with the bench-hooks feature enabled, ensuring the
feature-gated bench_hooks module is compiled and exercised. Configure the job to
invoke the appropriate cargo benchmark command for crates/logfmt with --features
bench-hooks, matching existing CI conventions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9877d454-285b-4cc7-9d17-e2a979823434
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/api-core/src/logging/setup.rscrates/api-core/src/logging/stream.rscrates/api-core/src/run.rscrates/logfmt/Cargo.tomlcrates/logfmt/benches/logfmt_layer.rscrates/logfmt/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
logfmt rendered every field of every line into its own heap String, sorted the formatted strings, then rewrote them into the output buffer, and debug values were formatted twice. The quoting decision built a `char::escape_debug` iterator for every character of every value. On top of that, carbide-api installed the admin UI's live log-stream layer unconditionally, so every INFO line was visited twice -- a BTreeMap, a timestamp and target String, a mutex, and a 128MiB ring buffer -- even with the admin UI disabled. Field values now render once at record time and the write phase just copies each rendered key and value into the reused buffer; the sort reproduces the exact rendered-token ordering, pinned by tests for the key-prefix, escaped-key, and duplicate-key edges. Quoting classifies ASCII with a byte-range match, keeping the original probe for non-ASCII. The log-stream layer installs only when the admin UI is enabled, and closing spans move their field map out instead of cloning it. - One render per field; copy-only write phase; rendered-token-order sort. - ASCII byte-match quoting classifier; non-ASCII decisions unchanged. - Admin log-stream layer gated on `enable_admin_ui`. Output bytes are a contract: all seventeen golden tests pass unchanged, and a sweep of every Unicode scalar pins the new classifier to the old one. Benched at about -14% time and -43% allocations per line, with the clean-ASCII quoting scan several times faster. This supports NVIDIA#3219 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
1 participant
logfmt rendered every field of every log line into its own heap String, sorted the formatted strings, rewrote them into the output buffer, and double-formatted debug values -- and carbide-api installed the admin UI's live log-stream layer (BTreeMap, timestamp/target Strings, a mutex, a 128MiB ring buffer per line) even with the admin UI disabled.
Now each field renders once at record time:
=-in-key edges (the last also keeps the comparator total).enable_admin_uiis set, and closing spans move their field map out instead of cloning it.Output bytes are a contract: all seventeen pre-existing golden tests pass unchanged. Benched at about -14% time and -43% allocations per line, with the clean-ASCII quoting scan several times faster.
This supports #3219