feat(service): implement real auto-switch metrics and adapter lifecycle#28
Open
EffortlessSteven wants to merge 2 commits intomainfrom
Open
feat(service): implement real auto-switch metrics and adapter lifecycle#28EffortlessSteven wants to merge 2 commits intomainfrom
EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
- Add AutoSwitchMetrics struct with AtomicU64 counters (switches_total, detection_attempts, detection_failures, avg_detection_ms) - Add record_switch() for recording switch events with timing (EMA) - Add record_failure() for failed detection attempts - Add snapshot() returning HashMap<String, u64> metrics snapshot - Add AdapterDetectionHooks trait for adapter lifecycle callbacks (on_detection_start, on_detection_stop, adapter_state) - Add 16 new tests for metrics tracking, EMA convergence, snapshot keys, mixed success/failure accounting, hook lifecycle, and failure paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd AutoSwitchMetrics and AdapterDetectionHooks implementations
WalkthroughsDescription• Implement lock-free AutoSwitchMetrics struct with atomic counters - Tracks switches_total, detection_attempts, detection_failures - Computes exponential-moving-average detection time in microseconds - Provides snapshot() method returning HashMap with millisecond conversion • Add AdapterDetectionHooks trait for adapter lifecycle management - Callbacks for on_detection_start and on_detection_stop events - Default implementations return Ok(()) and AdapterState::Stopped - Enables setup/teardown of expensive resources (connections, sockets) • Add 16 comprehensive tests covering metrics and hooks - Zeroed initial state, counter increments, EMA convergence - Snapshot key completeness and millisecond conversion - Hook lifecycle, multi-simulator support, failure paths Diagramflowchart LR
A["AutoSwitchMetrics<br/>Lock-free counters"] -->|record_switch| B["EMA Detection Time<br/>Microseconds"]
A -->|record_failure| C["Failure Tracking"]
A -->|snapshot| D["HashMap<String, u64><br/>Millisecond conversion"]
E["AdapterDetectionHooks<br/>Trait"] -->|on_detection_start| F["Adapter Setup"]
E -->|on_detection_stop| G["Adapter Teardown"]
E -->|adapter_state| H["State Query"]
File Changes1. crates/flight-service/src/aircraft_auto_switch_service.rs
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
Adds new building blocks in flight-service’s aircraft auto-switch subsystem: a lock-free AutoSwitchMetrics counter aggregate and an AdapterDetectionHooks trait intended for adapter lifecycle callbacks, along with unit tests validating both APIs.
Changes:
- Introduce
AutoSwitchMetricsbacked byAtomicU64counters plus snapshot/debug helpers. - Add
AdapterDetectionHookstrait with default start/stop/state methods. - Add unit tests covering counter behavior, EMA behavior, snapshot completeness, and hook lifecycle/failure paths.
Comment on lines
+331
to
+342
| let sample_us = detection_duration.as_micros() as u64; | ||
| let _ = self.avg_detection_us.fetch_update( | ||
| Ordering::Relaxed, | ||
| Ordering::Relaxed, | ||
| |old| { | ||
| if old == 0 { | ||
| Some(sample_us) | ||
| } else { | ||
| // Integer EMA: new = old + (sample - old) / 10 | ||
| let diff = sample_us as i64 - old as i64; | ||
| Some((old as i64 + diff / 10) as u64) | ||
| } |
There was a problem hiding this comment.
record_switch computes the EMA using sample_us as i64 / old as i64. Casting u64→i64 can wrap for values > i64::MAX, producing a negative diff and corrupting the average. Consider doing the EMA math in i128/u128 (or using checked/saturating conversions) and only converting back to u64 after clamping, to avoid wraparound/truncation edge cases.
Comment on lines
+424
to
+435
| #[allow(unused_variables)] | ||
| pub trait AdapterDetectionHooks: Send + Sync { | ||
| /// Called when process detection triggers adapter initialisation. | ||
| /// | ||
| /// The adapter should perform any expensive setup here (connections, | ||
| /// allocations, background tasks). Returns `Ok(())` on success. | ||
| fn on_detection_start(&mut self, sim: BusSimId) -> std::result::Result<(), String> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Called when the process is lost and the adapter should tear down. | ||
| fn on_detection_stop(&mut self, sim: BusSimId) -> std::result::Result<(), String> { |
There was a problem hiding this comment.
#[allow(unused_variables)] on the whole trait suppresses warnings for all implementors. Since only the default method bodies ignore sim, prefer naming the parameter _sim in the default implementations (or using let _ = sim;) and remove the broad allow attribute so real unused-variable issues in implementors aren’t hidden.
Suggested change
| #[allow(unused_variables)] | |
| pub trait AdapterDetectionHooks: Send + Sync { | |
| /// Called when process detection triggers adapter initialisation. | |
| /// | |
| /// The adapter should perform any expensive setup here (connections, | |
| /// allocations, background tasks). Returns `Ok(())` on success. | |
| fn on_detection_start(&mut self, sim: BusSimId) -> std::result::Result<(), String> { | |
| Ok(()) | |
| } | |
| /// Called when the process is lost and the adapter should tear down. | |
| fn on_detection_stop(&mut self, sim: BusSimId) -> std::result::Result<(), String> { | |
| pub trait AdapterDetectionHooks: Send + Sync { | |
| /// Called when process detection triggers adapter initialisation. | |
| /// | |
| /// The adapter should perform any expensive setup here (connections, | |
| /// allocations, background tasks). Returns `Ok(())` on success. | |
| fn on_detection_start(&mut self, _sim: BusSimId) -> std::result::Result<(), String> { | |
| Ok(()) | |
| } | |
| /// Called when the process is lost and the adapter should tear down. | |
| fn on_detection_stop(&mut self, _sim: BusSimId) -> std::result::Result<(), String> { |
Comment on lines
+295
to
+326
| /// Lock-free aggregate metrics for the auto-switch subsystem. | ||
| /// | ||
| /// All counters use [`AtomicU64`] with relaxed ordering so they can be | ||
| /// updated from any task without acquiring an async lock. | ||
| pub struct AutoSwitchMetrics { | ||
| /// Total number of profile switches performed. | ||
| switches_total: AtomicU64, | ||
| /// Total detection attempts (successful + failed). | ||
| detection_attempts: AtomicU64, | ||
| /// Total detection failures. | ||
| detection_failures: AtomicU64, | ||
| /// Exponential-moving-average detection time stored as whole | ||
| /// microseconds (µs). Callers that want milliseconds can divide by 1000. | ||
| avg_detection_us: AtomicU64, | ||
| } | ||
|
|
||
| impl AutoSwitchMetrics { | ||
| /// Create a new, zeroed metrics instance. | ||
| pub fn new() -> Self { | ||
| Self { | ||
| switches_total: AtomicU64::new(0), | ||
| detection_attempts: AtomicU64::new(0), | ||
| detection_failures: AtomicU64::new(0), | ||
| avg_detection_us: AtomicU64::new(0), | ||
| } | ||
| } | ||
|
|
||
| /// Record a successful switch event with the given detection duration. | ||
| /// | ||
| /// Updates `switches_total`, `detection_attempts`, and the running EMA of | ||
| /// detection time. | ||
| pub fn record_switch(&self, detection_duration: Duration) { |
There was a problem hiding this comment.
AutoSwitchMetrics and AdapterDetectionHooks are introduced as pub, but they don’t appear to be used anywhere in this module (no calls to record_switch/record_failure, and no code invokes the hook callbacks). If the intent is to implement “real auto-switch metrics and adapter lifecycle”, it looks incomplete—either wire these into the service’s detection/switch paths or adjust the PR scope/visibility so the new APIs aren’t dead weight.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+295
to
+443
| /// Lock-free aggregate metrics for the auto-switch subsystem. | ||
| /// | ||
| /// All counters use [`AtomicU64`] with relaxed ordering so they can be | ||
| /// updated from any task without acquiring an async lock. | ||
| pub struct AutoSwitchMetrics { | ||
| /// Total number of profile switches performed. | ||
| switches_total: AtomicU64, | ||
| /// Total detection attempts (successful + failed). | ||
| detection_attempts: AtomicU64, | ||
| /// Total detection failures. | ||
| detection_failures: AtomicU64, | ||
| /// Exponential-moving-average detection time stored as whole | ||
| /// microseconds (µs). Callers that want milliseconds can divide by 1000. | ||
| avg_detection_us: AtomicU64, | ||
| } | ||
|
|
||
| impl AutoSwitchMetrics { | ||
| /// Create a new, zeroed metrics instance. | ||
| pub fn new() -> Self { | ||
| Self { | ||
| switches_total: AtomicU64::new(0), | ||
| detection_attempts: AtomicU64::new(0), | ||
| detection_failures: AtomicU64::new(0), | ||
| avg_detection_us: AtomicU64::new(0), | ||
| } | ||
| } | ||
|
|
||
| /// Record a successful switch event with the given detection duration. | ||
| /// | ||
| /// Updates `switches_total`, `detection_attempts`, and the running EMA of | ||
| /// detection time. | ||
| pub fn record_switch(&self, detection_duration: Duration) { | ||
| self.switches_total.fetch_add(1, Ordering::Relaxed); | ||
| self.detection_attempts.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| // EMA with α = 0.1, operating in integer microseconds. | ||
| let sample_us = detection_duration.as_micros() as u64; | ||
| let _ = self.avg_detection_us.fetch_update( | ||
| Ordering::Relaxed, | ||
| Ordering::Relaxed, | ||
| |old| { | ||
| if old == 0 { | ||
| Some(sample_us) | ||
| } else { | ||
| // Integer EMA: new = old + (sample - old) / 10 | ||
| let diff = sample_us as i64 - old as i64; | ||
| Some((old as i64 + diff / 10) as u64) | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| /// Record a failed detection attempt. | ||
| pub fn record_failure(&self) { | ||
| self.detection_attempts.fetch_add(1, Ordering::Relaxed); | ||
| self.detection_failures.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// Return a snapshot of all counters as a `HashMap<String, u64>`. | ||
| /// | ||
| /// Keys: `"switches_total"`, `"detection_attempts"`, | ||
| /// `"detection_failures"`, `"avg_detection_ms"`. | ||
| pub fn snapshot(&self) -> HashMap<String, u64> { | ||
| let mut map = HashMap::with_capacity(4); | ||
| map.insert( | ||
| "switches_total".into(), | ||
| self.switches_total.load(Ordering::Relaxed), | ||
| ); | ||
| map.insert( | ||
| "detection_attempts".into(), | ||
| self.detection_attempts.load(Ordering::Relaxed), | ||
| ); | ||
| map.insert( | ||
| "detection_failures".into(), | ||
| self.detection_failures.load(Ordering::Relaxed), | ||
| ); | ||
| map.insert( | ||
| "avg_detection_ms".into(), | ||
| self.avg_detection_us.load(Ordering::Relaxed) / 1000, | ||
| ); | ||
| map | ||
| } | ||
|
|
||
| /// Read individual counters. | ||
| pub fn switches_total(&self) -> u64 { | ||
| self.switches_total.load(Ordering::Relaxed) | ||
| } | ||
|
|
||
| pub fn detection_attempts(&self) -> u64 { | ||
| self.detection_attempts.load(Ordering::Relaxed) | ||
| } | ||
|
|
||
| pub fn detection_failures(&self) -> u64 { | ||
| self.detection_failures.load(Ordering::Relaxed) | ||
| } | ||
|
|
||
| /// Average detection time in microseconds. | ||
| pub fn avg_detection_us(&self) -> u64 { | ||
| self.avg_detection_us.load(Ordering::Relaxed) | ||
| } | ||
| } | ||
|
|
||
| impl Default for AutoSwitchMetrics { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Debug for AutoSwitchMetrics { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("AutoSwitchMetrics") | ||
| .field("switches_total", &self.switches_total()) | ||
| .field("detection_attempts", &self.detection_attempts()) | ||
| .field("detection_failures", &self.detection_failures()) | ||
| .field("avg_detection_us", &self.avg_detection_us()) | ||
| .finish() | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // AdapterDetectionHooks — trait for adapter lifecycle callbacks | ||
| // ============================================================================ | ||
|
|
||
| /// Trait for adapter detection lifecycle hooks. | ||
| /// | ||
| /// Implementors receive callbacks when the auto-switch service starts or stops | ||
| /// detection for a particular simulator. This allows adapters to perform | ||
| /// setup/teardown (e.g. opening a SimConnect handle, binding a UDP socket) | ||
| /// exactly when needed. | ||
| #[allow(unused_variables)] | ||
| pub trait AdapterDetectionHooks: Send + Sync { | ||
| /// Called when process detection triggers adapter initialisation. | ||
| /// | ||
| /// The adapter should perform any expensive setup here (connections, | ||
| /// allocations, background tasks). Returns `Ok(())` on success. | ||
| fn on_detection_start(&mut self, sim: BusSimId) -> std::result::Result<(), String> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Called when the process is lost and the adapter should tear down. | ||
| fn on_detection_stop(&mut self, sim: BusSimId) -> std::result::Result<(), String> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Return the current adapter state (informational). | ||
| fn adapter_state(&self) -> AdapterState { | ||
| AdapterState::Stopped | ||
| } | ||
| } |
There was a problem hiding this comment.
1. New apis not wired 🐞 Bug ⛯ Reliability
AutoSwitchMetrics and AdapterDetectionHooks are added as public building blocks but are not stored or invoked anywhere in AircraftAutoSwitchService, so they currently provide no runtime behavior and can mislead consumers/maintainers about feature completeness. If the PR goal is “real auto-switch metrics and adapter lifecycle”, integration is missing.
Agent Prompt
### Issue description
`AutoSwitchMetrics` and `AdapterDetectionHooks` are defined and tested, but the service never stores or invokes them. This leaves the PR’s stated feature (“real auto-switch metrics and adapter lifecycle”) incomplete.
### Issue Context
Current service metrics are sourced from `flight-session` (`SwitchMetrics`) and existing atomics; adapter start/stop is handled directly without hooks.
### Fix Focus Areas
- crates/flight-service/src/aircraft_auto_switch_service.rs[295-443]
- crates/flight-service/src/aircraft_auto_switch_service.rs[465-506]
- crates/flight-service/src/aircraft_auto_switch_service.rs[547-607]
- crates/flight-service/src/aircraft_auto_switch_service.rs[738-758]
- crates/flight-service/src/aircraft_auto_switch_service.rs[920-1015]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…sues - Fix EMA i64 overflow by computing in i128 with clamping - Remove broad #[allow(unused_variables)], use _sim prefixes instead - Wire AutoSwitchMetrics into AircraftAutoSwitchService - Invoke AdapterDetectionHooks during detection lifecycle - Add get_switch_metrics() and add_detection_hook() methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a lock-free \AutoSwitchMetrics\ struct and an \AdapterDetectionHooks\ trait to the auto-switch service, replacing conceptual TODOs with real implementations.
Changes
AutoSwitchMetrics (lock-free aggregate counters)
ecord_switch(duration)\ — record a successful switch with timing
ecord_failure()\ — record a failed detection attempt
AdapterDetectionHooks trait
Tests (16 new)
Validation