Skip to content

refactor(sdk): share aggregator shape across hotspots roll-ups#398

Merged
willwashburn merged 1 commit intomainfrom
claude/resolve-github-issue-0jdvd
May 9, 2026
Merged

refactor(sdk): share aggregator shape across hotspots roll-ups#398
willwashburn merged 1 commit intomainfrom
claude/resolve-github-issue-0jdvd

Conversation

@willwashburn
Copy link
Copy Markdown
Member

Closes #340.

Summary

Three small clean-ups in crates/relayburn-sdk/src/analyze/hotspots.rs:

  • Shared aggregator shape. aggregate_by_file, aggregate_by_bash, and aggregate_by_subagent had near-parallel "filter → accumulate by key → sort by cost desc" boilerplate. Extract a private generic aggregate(…) helper parameterized by key/init/accumulate/cost closures; each public function now declares only the bits that actually differ. aggregate_by_bash_verb keeps its own loop because it tracks distinct hashes and per-verb examples on top of the basic shape.
  • f64::total_cmp for monotonic cost ordering. Replace every partial_cmp(...).unwrap_or(Ordering::Equal) cost comparator in this file with total_cmp — drop-in for our positive-cost values and removes the silent-Equal fallback for NaN. Covers the file/bash/subagent aggregates and both sorts in the bash-verb path.
  • One pass over session_turns in attribute_hotspots. attribute_session already iterates the session's turns; have it accumulate the per-session grand total in that same pass via cost_for_turn and return it on SessionAttribution. The caller drops the second for t in &session_turns { … } loop. Behavior is unchanged — totals still route through the canonical cost_for_turn so source-specific reasoning billing (Codex included_in_output, separate reasoning tariffs, etc.) stays in lock-step with cost.rs.

No public-API changes. Existing analyze::hotspots tests cover deterministic sort, Codex reasoning, and the grand_total + unattributed = session_grand invariant.

Test plan

  • cargo test -p relayburn-sdk --lib analyze::hotspots (14 passed)
  • cargo test --workspace (775 passed, 0 failed)
  • cargo clippy -p relayburn-sdk --all-targets — no new warnings on the touched code

https://claude.ai/code/session_01Wmh6iHN5aYaPKhFdR9bA14


Generated by Claude Code

- Extract a generic `aggregate` helper for `aggregate_by_file`,
  `aggregate_by_bash`, `aggregate_by_subagent`. Each caller passes its
  key extractor, row initializer, and accumulator; the shared helper owns
  iteration, deduping, and the cost-desc sort.
- Switch cost comparators from
  `partial_cmp(...).unwrap_or(Ordering::Equal)` to `f64::total_cmp` so
  monotonic ordering is exact. Applies to file/bash/subagent aggregates
  plus the bash-verb path.
- Fold the per-session grand-total accumulation into
  `attribute_session`'s existing turn loop, eliminating the second pass
  over `session_turns` in `attribute_hotspots`.

Closes #340.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 9, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 954b4e4a-940a-4f00-967b-39dfbf91b72a

📥 Commits

Reviewing files that changed from the base of the PR and between 7da0599 and b489ef1.

📒 Files selected for processing (1)
  • crates/relayburn-sdk/src/analyze/hotspots.rs

📝 Walkthrough

Walkthrough

This PR consolidates hotspot analysis logic: session grand totals are computed once in the attribution loop and reused downstream, three parallel aggregation functions are unified under a generic helper, and float comparisons are switched to deterministic ordering for consistent output.

Changes

Hotspot Attribution and Aggregation Refactor

Layer / File(s) Summary
Session Attribution Data Shape
crates/relayburn-sdk/src/analyze/hotspots.rs
SessionAttribution struct gains a new grand_total: f64 field to hold session-level cost sums.
Grand Total Computation in Attribution
crates/relayburn-sdk/src/analyze/hotspots.rs
attribute_session accumulates each turn's cost_for_turn(...).total into a grand_total variable during the per-turn loop; empty-turns path initializes grand_total to 0.0.
Grand Total Reuse in Hotspots
crates/relayburn-sdk/src/analyze/hotspots.rs
attribute_hotspots derives each session's grand_cost from session_result.grand_total instead of recalculating it in a separate pass.
Generic Aggregation Helper
crates/relayburn-sdk/src/analyze/hotspots.rs
New internal generic aggregate helper iterates attributions, groups rows by extracted key, accumulates shared metrics (total_cost, total_count), and sorts by total_cost descending.
Refactored Aggregation Functions
crates/relayburn-sdk/src/analyze/hotspots.rs
aggregate_by_file, aggregate_by_bash, and aggregate_by_subagent are rewritten to use the aggregate helper, preserving file-target filtering, args-hash collapsing, and (unknown) fallback behavior.
Deterministic Float Comparisons
crates/relayburn-sdk/src/analyze/hotspots.rs
Bash-verb example and final row sorting switched total_cost comparators from partial_cmp(...).unwrap_or(...) to f64::total_cmp.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • AgentWorkforce/burn#386: Both PRs refactor hotspot aggregation/attribution inside relayburn-sdk analyze code; #386 modifies summary/hotspots aggregation while this PR refactors the underlying hotspots.rs logic.
  • AgentWorkforce/burn#291: Both PRs modify hotspots attribution and aggregation functions (attribute_hotspots, aggregate_by_file/bash/subagent, session grand-total handling).

Poem

🐰 Three loops become one, grand totals aligned,
Aggregation patterns in harmony bind,
Float comparisons settled with total_cmp grace,
Hotspot refactors now run a faster race! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main refactoring change: introducing a shared generic aggregator shape across hotspot rollup functions, which is the primary objective of the PR.
Description check ✅ Passed The description is directly related to the changeset, detailing the three clean-ups made to hotspots.rs and referencing the closed issue #340, providing context for the refactoring.
Linked Issues check ✅ Passed All primary objectives from issue #340 are addressed: generic aggregator shape introduced [#340], f64::total_cmp used for cost ordering [#340], and session_turns loop consolidated [#340].
Out of Scope Changes check ✅ Passed All changes align with issue #340 scope: generic aggregate helper extraction, f64::total_cmp migration, and session loop consolidation are the three stated objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/resolve-github-issue-0jdvd

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@willwashburn willwashburn merged commit c2613b9 into main May 9, 2026
12 checks passed
@willwashburn willwashburn deleted the claude/resolve-github-issue-0jdvd branch May 9, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rust refactor: hotspots aggregate_by_file/bash/subagent share a generic shape

2 participants