Repository navigation
refactor(sort): demote algorithm-internal info! logs to debug - #367
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR systematically demotes algorithm-internal logging from 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-sort/src/memory_probe.rs (1)
52-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale log-level guidance for memory probe target.
This comment still says
RUST_LOG=fgumi_lib::sort::memory_probe=info, but probe logging now gates on debug. Update it to...=debugto matchenabled().🤖 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/fgumi-sort/src/memory_probe.rs` around lines 52 - 55, The comment in the memory probe module still suggests using `RUST_LOG=fgumi_lib::sort::memory_probe=info` but the probe logging is gated at debug via the `enabled()` check; update the comment string to `RUST_LOG=fgumi_lib::sort::memory_probe=debug` so the guidance matches the actual behavior (refer to the `memory_probe` module comment and the `enabled()` call that controls logging).
🤖 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.
Outside diff comments:
In `@crates/fgumi-sort/src/memory_probe.rs`:
- Around line 52-55: The comment in the memory probe module still suggests using
`RUST_LOG=fgumi_lib::sort::memory_probe=info` but the probe logging is gated at
debug via the `enabled()` check; update the comment string to
`RUST_LOG=fgumi_lib::sort::memory_probe=debug` so the guidance matches the
actual behavior (refer to the `memory_probe` module comment and the `enabled()`
call that controls logging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1ca1cc6-e922-4656-8163-5bda473a96a5
📒 Files selected for processing (5)
crates/fgumi-sort/src/external.rscrates/fgumi-sort/src/memory_probe.rscrates/fgumi-sort/src/pipeline.rscrates/fgumi-sort/src/worker_pool.rssrc/lib/commands/sort.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
- Coverage 90.95% 90.89% -0.06%
==========================================
Files 78 78
Lines 51101 51101
==========================================
- Hits 46480 46450 -30
- Misses 4621 4651 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The sort engine emitted ~80 info! lines at the default log level, mixing user-relevant "what ran" output (config snapshot, summary, phase-timing report) with algorithm-internal chatter (per-phase markers, in-memory fast-path decisions, k-way merge thread budgets, memory-probe samples, worker-pool stats). Downstream library consumers worked around the volume by raising their own default filter (see fg-labs/mako#5); this fixes the cause instead. Split the existing info! sites into two buckets: Kept at info (user-visible "what ran"): - commands/sort.rs Sort::execute config snapshot (Input, Output, Sort order, Cell tag, Max memory, Threads, Temp compression level, Write index, Temp directories) and the trailing Summary block. - commands/sort.rs Sort::verify config snapshot (Input, Expected order, Cell tag) and the trailing Verification Summary block. - fgumi-sort/src/external.rs final Sort Phase Timing report and the two "Wrote BAM index" outcome events. Demoted to debug (algorithm-internal): - "Starting Sort" / "Starting Sort Verification" banners (the config snapshot already announces the operation). - "Auto memory: using …" lines adjacent to the memory-fallback warn!s (they read as debug context for the warning, not standalone events). - Per-phase markers ("Phase 1: …", "Phase 2: Merging N chunks …", "Consolidating N temp files …", "Consolidation complete …"). - Algorithm-layer startup duplicates ("Starting raw-bytes sort", "Memory limit", "Threads") that duplicate the header config block at a different layer. - Algorithm-choice noise ("All records fit in memory …", "Using queryname sort with X comparator"). - Intermediate progress already covered by the summary ("Read N records total", "Sort complete: N records processed"). - K-way merge internals ("Starting k-way merge …", "Merge complete: 0 records merged", "Merging from N sources …", "Merge thread budget …", "Pool-integrated merge: …", "Starting parallel/buffered parallel merge of N chunks"). - "Indexing enabled: …" pre-stage announcement (the final "Wrote BAM index: …" stays at info). - "async sort reader enabled: …" (the user passed --async-reader; the algorithm-layer confirmation is debug detail). - All four memory_probe.rs info! samples and the matching log_enabled!(target, Level::Info) gate, which is now Level::Debug so the cheap-path short-circuit still works at the new level. - Both worker_pool.rs info! sites ("Pool stats: …", per-line "{line}" forwarder). The fgumi CLI's -v flag already jumps info -> debug, so anyone who used it for "extra detail" keeps the same observable behavior; only the default level becomes quieter. Closes #364
71b82b2 to
e1ebd5c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This branch was previously deployed
1 participant
Summary
fgumi_lib::commands::sortemitted ~80info!lines at the default log level, mixing user-relevant "what ran" output (config snapshot, summary, phase-timing report) with algorithm-internal chatter (per-phase markers, in-memory fast-path decisions, k-way merge thread budgets, memory-probe samples, worker-pool stats). Downstream library consumers worked around the volume by raising their own default filter (see fg-labs/mako#5); this fixes the cause instead.Kept at
info(user-visible "what ran")commands/sort.rsSort::executeconfig snapshot (Input, Output, Sort order, Cell tag, Max memory, Threads, Temp compression level, Write index, Temp directories) and the trailing=== Summary ===block.commands/sort.rsSort::verifyconfig snapshot (Input, Expected order, Cell tag) and the trailing=== Verification Summary ===block.fgumi-sort/src/external.rsfinal=== Sort Phase Timing ===report and the twoWrote BAM index: …outcome events.Demoted to
debug(algorithm-internal)Starting Sort/Starting Sort Verificationbanners (the config snapshot already announces the operation).Auto memory: using …lines adjacent to the memory-fallbackwarn!s (they read as debug context for the warning, not standalone events).Phase 1: …,Phase 2: Merging N chunks …,Consolidating N temp files …,Consolidation complete …).Starting raw-bytes sort,Memory limit,Threads) that duplicate the header config block at a different layer.All records fit in memory …,Using queryname sort with X comparator).Read N records total,Sort complete: N records processed).Starting k-way merge …,Merge complete: 0 records merged,Merging from N sources …,Merge thread budget …,Pool-integrated merge: …,Starting parallel/buffered parallel merge of N chunks).Indexing enabled: …pre-stage announcement (the finalWrote BAM index: …stays at info).async sort reader enabled: …(the user passed--async-reader; the algorithm-layer confirmation is debug detail).memory_probe.rsinfo!samples and the matchinglog_enabled!(target, Level::Info)gate, which is nowLevel::Debugso the cheap-path short-circuit still works at the new level.worker_pool.rsinfo!sites (Pool stats: …, per-line{line}forwarder).memory_probe.rsthat documented the gate asInfo/RUST_LOG=…=infowas updated toDebug/…=debug.Compatibility
The fgumi CLI's
-vflag already jumpsinfo → debug, so anyone who used it for "extra detail" keeps the same observable behavior; only the default level becomes quieter. Log lines are not a documented API surface, but anyone scraping stdout/stderr for the demoted phrases will see fewer of them at the default level.Out of scope
warn!/error!/debug!calls.-vdebug toggle is the escape hatch).env_loggerdefault keeps emitting[<ts> <LEVEL> <module>] <msg>).Test plan
cargo build --release(workspace)cargo nextest run -p fgumi-sort— 396/396 passcargo nextest run -E 'test(/sort/)'— 56/56 passcargo ci-fmtcargo ci-lintcargo ci-tag-literalsCloses #364