feat(swift-sdk): log tokio runtime metrics from rs-sdk-ffi and platform-wallet-ffi#3901
Conversation
📝 WalkthroughWalkthroughAdds an optional Changestokio-metrics feature rollout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
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 complete (commit 6c3e59c) |
9f0d110 to
3ff6011
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Dev-only Tokio runtime observability behind an opt-in tokio-metrics feature; no consensus, storage, or FFI ABI surface is touched. Two real but minor issues remain: an unconditional import that becomes unused when the feature is off (CI uses --all-features so it does not fail CI, but pollutes default cargo check), and a sampler ordering quirk where the first emitted interval is near-zero. No blocking issues.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/runtime.rs:25: Gate AtomicUsize/Ordering import behind the tokio-metrics feature
`use std::sync::atomic::{AtomicUsize, Ordering};` is unconditional, but both symbols are only referenced inside `#[cfg(feature = "tokio-metrics")]` blocks (lines 67/74 in `build_shared` and 91/98 in `new_isolated`); the inner `metrics` module re-imports them at line 181. Verified locally: `cargo check -p rs-sdk-ffi` (default features) emits `warning: unused imports: AtomicUsize and Ordering`. CI runs clippy with `--all-features` so it does not currently fail, but this still makes default `cargo build`/`cargo check` noisy for anyone working in this crate without enabling the feature. Codex's claim that this is blocking for the repo's `-D warnings` gate is incorrect — CI passes the all-features flag.
3ff6011 to
771cbaa
Compare
771cbaa to
6c3e59c
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Three carried-forward nitpicks remain valid against 9f0d110: an unconditional AtomicUsize/Ordering import in rs-sdk-ffi that becomes an unused-import warning under default features, and the same next()-before-sleep ordering in both tokio-metrics samplers (rs-sdk-ffi and rs-platform-wallet-ffi) that shifts/skews emitted metrics by one interval. No new findings from the latest delta (logging.rs only) and no FFI ABI changes. All findings are non-blocking; consensus/proof paths are untouched.
💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review against head 3ff6011. All three prior findings are still valid: the unconditional AtomicUsize/Ordering imports in rs-sdk-ffi remain ungated (warn when tokio-metrics is off), and both RuntimeMonitor samplers (rs-sdk-ffi and rs-platform-wallet-ffi) still advance the interval iterator before awaiting the one-second sleep, producing a near-zero first record and one-period-stale subsequent records. No new in-scope blocking issues in the latest delta; CodeRabbit had zero inline findings.
💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "6bd6216957124b17dc0ebc93db840777668b940b8305908809433b26f3d76067"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of HEAD 6c3e59c. All three prior findings from base 3ff6011 are FIXED: AtomicUsize/Ordering imports are properly gated behind #[cfg(feature = "tokio-metrics")] in rs-sdk-ffi/src/runtime.rs:25-26, and both rs-sdk-ffi and platform-wallet-ffi samplers correctly call tokio::time::sleep(...) before rt_intervals.next(). One new latest-delta suggestion from codex about restoring a BLOCK_ON_TOTAL counter to keep metric coverage aligned with the PR description; severity is suggestion-level since the feature is dev-only and the value can be approximated from event counts.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/runtime.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/runtime.rs:180-213: Emit total block_on count alongside in_flight
The PR description states `BlockOnGuard` tracks both concurrency and cumulative FFI calls, but the current metrics module only maintains `BLOCK_ON_IN_FLIGHT` and emits `in_flight` on the `kind = "block_on"` event. Without a monotonic `total`, correlating sampled elapsed call durations against overall call volume in `dash_sdk/metrics.log` requires counting log events, which is fragile when only a subset of events are retained or the log is rotated. Add a `BLOCK_ON_TOTAL: AtomicU64` and include it on the emitted event so the per-call records are self-contained.
Adds an optional, dev-only observability layer that surfaces tokio runtime health for the two FFI runtimes behind the unified iOS SDK.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
tokio-metricsfeature to enable periodic Tokio runtime monitoring (workers, tasks, busy ratio, queue depth, and polling stats) and per-block_ontiming/in-flight tracking across SDK packages.Bug Fixes
metrics.logfiles and failing file setup if any required log file can’t be opened.Chores
tokio-metricsdependency, forwarded the feature via the unified SDK, and enabled it automatically for iOSdev-iosbuilds.