Feature/signpost metric sessions & summary/average API #16
Conversation
way to get latest session from a start point
WalkthroughAdds CI refinements and dependency bumps, introduces a SqliteExporterHandle and RequestSummaryFromSignpost flow to request averaged metrics, adds a MetricsDb.session_from_signpost API and a new internal query module, and adds an examples/summary.rs demonstration program. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handle as SqliteExporterHandle
participant Worker as ExporterWorker
participant DB as MetricsDb
Client->>Handle: request_average_metrics(signpost_key, keys)
activate Handle
Handle->>Worker: send RequestSummaryFromSignpost { signpost_key, keys, tx }
deactivate Handle
activate Worker
Worker->>DB: session_from_signpost(signpost_key)
activate DB
DB-->>Worker: Session (start, end)
deactivate DB
loop for each key
Worker->>DB: metrics_for_key(key, session)
DB-->>Worker: Vec<Metric>
end
Worker->>Worker: compute averages & build HashMap
Worker-->>Handle: send Result<HashMap> via oneshot tx
deactivate Worker
Handle-->>Client: return Result<HashMap<String,f64>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/rust.yml(2 hunks)Cargo.toml(1 hunks)examples/summary.rs(1 hunks)src/lib.rs(10 hunks)src/metrics_db.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
examples/summary.rs (2)
src/lib.rs (5)
db(206-206)new(156-170)new(444-458)average(277-283)metrics(517-517)src/metrics_db.rs (3)
new(35-41)new(51-55)metrics(78-81)
src/metrics_db.rs (1)
src/lib.rs (2)
setup_db(74-84)db(206-206)
src/lib.rs (3)
src/metrics_db.rs (13)
metrics(78-81)metric_key_for_key(133-140)metric_keys(105-108)query(70-70)query(124-127)query(128-128)query(136-136)query(183-183)session_from_signpost(63-74)end_query(72-72)new(35-41)new(51-55)metrics_for_key(113-131)src/models.rs (1)
query(93-93)examples/summary.rs (1)
average(24-30)
🪛 actionlint (1.7.8)
.github/workflows/rust.yml
21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/rust.yml (1)
26-27: Run tests with all features for parity.
cargo buildandcargo clippyexercise every feature, butcargo teststill runs with the default feature set. Adding--all-features(and optionally--all-targets) keeps coverage consistent and catches feature-gated test regressions earlier.src/metrics_db/query.rs (1)
62-76: Use the existingsession.durationfield.Line 73 recalculates the session duration as
session.end_time - session.start_time, butSessionalready has adurationfield (seeSession::newin src/metrics_db.rs line 39) that computes this value. Reusing it avoids redundant computation and potential inconsistencies.Apply this diff:
let session = session_from_signpost(db, signpost)?; let mut results = HashMap::new(); for key in keys { let value = average_for_session(db, &key, &session)?; results.insert(key, value); } - let duration_secs = session.end_time - session.start_time; - results.insert("session.duration".to_string(), duration_secs); + results.insert("session.duration".to_string(), session.duration.as_secs_f64()); Ok(results) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/rust.yml(2 hunks)src/lib.rs(11 hunks)src/metrics_db.rs(7 hunks)src/metrics_db/query.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib.rs (1)
src/metrics_db/query.rs (5)
query(11-11)query(24-24)query(41-44)query(45-45)metrics_summary_for_signpost_and_keys(62-76)
src/metrics_db.rs (2)
src/lib.rs (4)
setup_db(78-88)new(160-174)new(423-437)db(210-210)src/metrics_db/query.rs (6)
query(11-11)query(24-24)query(41-44)query(45-45)session_from_signpost(17-28)metrics_for_key(30-48)
src/metrics_db/query.rs (2)
src/lib.rs (5)
db(210-210)metrics(496-496)new(160-174)new(423-437)metrics_summary_for_signpost_and_keys(252-258)src/metrics_db.rs (6)
query(155-155)session_from_signpost(65-67)metrics(71-74)new(35-41)new(53-57)metrics_for_key(106-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
🔇 Additional comments (4)
src/metrics_db.rs (2)
126-131: LGTM! Defensive clamping prevents negative derivatives.The change correctly clamps the derivative to
0.0whendt <= 0, preventing negative or undefined values from timestamp inversion or duplicate timestamps.
193-193: LGTM! Cleaner idiom for divisibility check.Using
is_multiple_of(200)is more readable and idiomatic than the previous modulo-based check.src/lib.rs (2)
352-389: LGTM! Addresses the previous review concern.The handler correctly flushes pending metrics (line 357) before querying the database, ensuring that the summary includes the most recent samples. Error handling is robust, with separate paths for flush errors and query errors.
Based on past review comments.
521-527: LGTM! Handle return enables communication after installation.The updated signature allows callers to request averages after installing the exporter, which aligns well with the new
SqliteExporterHandleAPI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/metrics_db.rs (1)
126-131: Consider logging when time delta is non-positive.The guard prevents division by zero and handles negative time deltas by returning
0.0, which is good defensive programming. However, these cases indicate data quality issues (identical or out-of-order timestamps) that could be difficult to debug without visibility. Consider adding a trace or warning log whendt <= 0is encountered.Apply this diff to add logging:
+use tracing::trace; + pub fn deriv_metrics_for_key( &mut self, key_name: &str, session: Option<&Session>, ) -> Result<Vec<DerivMetric>> { let m = self.metrics_for_key(key_name, session)?; let new_values: Vec<_> = m .windows(2) .map(|v| { let dt = v[1].timestamp - v[0].timestamp; let new_value = if dt > 0.0 { (v[1].value - v[0].value) / dt } else { + trace!( + "Non-positive time delta (dt={}) for key '{}' between timestamps {} and {}, clamping derivative to 0.0", + dt, key_name, v[0].timestamp, v[1].timestamp + ); 0.0 };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/lib.rs(11 hunks)src/metrics_db.rs(7 hunks)src/metrics_db/query.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/metrics_db/query.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/metrics_db.rs (2)
src/lib.rs (4)
setup_db(84-94)new(166-180)new(417-431)db(216-216)src/metrics_db/query.rs (6)
query(11-11)query(24-24)query(44-47)query(48-48)session_from_signpost(17-31)metrics_for_key(33-51)
src/lib.rs (1)
src/metrics_db/query.rs (5)
query(11-11)query(24-24)query(44-47)query(48-48)metrics_summary_for_signpost_and_keys(68-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (13)
src/metrics_db.rs (6)
2-11: LGTM!The import additions (
Path,Duration,error,trace) appropriately support the new query module and enhanced CSV import logging.
43-43: LGTM!The new internal
querymodule provides good separation of concerns by centralizing query and derivation logic.
64-67: LGTM!The new
session_from_signpostAPI is well-designed and properly delegates to the centralized query module. Error handling is appropriate.
111-111: LGTM!Good refactoring to centralize query logic in the
querymodule.
193-193: LGTM!Using
is_multiple_ofis more idiomatic than the modulo check.
23-23: LGTM!Documentation improvements and more idiomatic string interpolation in the derivative key format.
Also applies to: 34-34, 134-134
src/lib.rs (7)
59-67: LGTM!The new error variants (
ExporterUnavailable,ZeroLengthSession,NoMetricsForKey) appropriately support the new signpost-based session querying and handle edge cases.
114-119: LGTM!The
RequestSummaryFromSignpostevent variant is well-designed for async query requests using a oneshot channel pattern.
121-145: LGTM!The new
SqliteExporterHandleandrequest_average_metricsAPI are well-designed:
- Clean handle pattern for continued communication with the exporter
- Proper error handling: channel failures correctly map to
ExporterUnavailable(addressing the past review comment about error mapping)- Non-panicking design: uses
map_errand?instead ofunwrap()(addressing past concern)
246-252: LGTM!Appropriate delegation to the centralized
querymodule for computing metrics summaries.
346-383: LGTM!The
RequestSummaryFromSignpostevent handler is excellently implemented:
- Correctly flushes pending metrics before querying (addressing the past review comment about ensuring up-to-date results)
- Comprehensive error handling covering flush failures, query failures, and channel send failures
- Appropriate logging for all error paths
- Clean control flow
515-521: LGTM!Good API improvement: returning the
SqliteExporterHandlefrominstall()enables callers to query metrics while the recorder is active.
77-77: LGTM!Minor improvements including the new
querymodule import, error construction refinement, and documentation enhancements.Also applies to: 91-91, 412-412, 433-436
1 participant
Summary by CodeRabbit
New Features
Chores
Bug Fixes