refactor(site-explorer): pair iteration latency with failure logs#3748
Conversation
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 4 minutes. |
|
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)
Summary by CodeRabbit
WalkthroughSite Explorer now emits a completion event for each iteration, recording latency and logging failures. The legacy iteration-latency instrument and duplicate failure log were removed, with tests covering metrics, logs, and Prometheus naming. ChangesSite Explorer telemetry
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant run_single_iteration
participant SiteExplorer_emit_iteration_finished
participant SiteExplorerIterationFinished
participant carbide_instrument
run_single_iteration->>SiteExplorer_emit_iteration_finished: pass elapsed latency and optional error
SiteExplorer_emit_iteration_finished->>SiteExplorerIterationFinished: emit completion event
SiteExplorerIterationFinished->>carbide_instrument: record latency histogram
SiteExplorerIterationFinished->>carbide_instrument: emit ERROR log when error is present
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
chet
changed the title
|
@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.
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/site-explorer/src/metrics.rs`:
- Line 363: The metric selector assigned to metric_name must match the renamed
carbide_site_explorer_*_latency instrument view so custom histogram buckets are
used. Update the selector or instrument name consistently with the setup in
logging configuration, and add an end-to-end regression test covering the
exported metric path.
🪄 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: fcf090d0-42e5-46e2-8586-a1010ee95594
📒 Files selected for processing (2)
crates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rs
chet
changed the title
Site Explorer recorded every iteration's latency and separately wrote the ERROR record for a failed pass. `SiteExplorerIterationFinished` now owns the label-free histogram and emits the historical diagnostic only for post-lock failures. Success and the early lock-acquisition return remain metric-only, so normal replica contention keeps its existing caller-owned WARN without gaining a misleading ERROR. Tests preserve the Prometheus family, description, unit, bucket layout, and one-sample-per-terminal-path contract. This supports NVIDIA#3737 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 20 minutes. |
|
Hey @coderabbitai can you do a full_review when you get a chance? CICD isn't letting me continue because the CodeRabbit check "failed". Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 12 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
Site Explorer recorded every iteration's latency and separately wrote the ERROR record for a failed pass.
SiteExplorerIterationFinishednow owns the label-free histogram and emits the historical diagnostic only for post-lock failures. Success and the early lock-acquisition return remain metric-only, so normal replica contention keeps its existing caller-owned WARN without gaining a misleading ERROR.Tests preserve the Prometheus family, description, unit, bucket layout, and one-sample-per-terminal-path contract.
Related issues
Type of Change
Breaking Changes
Testing
A
check_valuestable covers successful and failed Event emissions. Separate tests pin the Prometheus family name, description, unit suffix, label-free shape, and the custom 30-second latency bucket. The existing lock-path test remains explicit.Verified with:
cargo test -p carbide-site-explorercargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docsAdditional Notes
CodeRabbit asked whether the Event's
_millisecondsmetric name would bypass the existing latency view. The framework removes that unit suffix before registering the OpenTelemetry instrument, so the registeredcarbide_site_explorer_iteration_latencyname still matches the view; the exposition and 30-second bucket tests cover both sides of that contract. The thread was resolved without a code change.Closes #3737