feat(metrics): optional metrics layer (off by default, Prometheus/OTLP-ready) - #734
Conversation
…P-ready) Adds an opt-in `metrics` feature that emits wa_* metrics through the `metrics` facade for rates and latency percentiles, complementing the tracing spans (PR #733). Off by default: no dependency and zero overhead - every emit is an inlined no-op and the duration Timer is a zero-sized type that reads no clock. The library only emits; the application installs a recorder (Prometheus, OTLP, ...). See examples/metrics.rs. wacore::telemetry provides typed helpers (counters/gauges plus a record-on-drop Timer), so call sites stay clean and labels are strictly low-cardinality categorical values (outcome/kind/result/reason) - never a JID, phone number or message id, which would explode the backend and leak PII. Counters: wa_recv_total{outcome}, wa_send_total{kind}, wa_retry_receipt_total{reason}, wa_iq_total{result}, wa_reconnect_total, wa_stream_error_total, wa_connect_total{outcome}, wa_appstate_sync_total{outcome}, wa_identity_change_total, wa_prekey_upload_total{outcome}. Histograms: wa_iq_duration_seconds, wa_connect_duration_seconds, wa_decrypt_duration_seconds, wa_send_duration_seconds, wa_appstate_sync_duration_seconds. Gauges: wa_connected (plus wa_pending_retries helper). Emitted at the same boundaries as the wa.* spans (connect/reconnect, recv/decrypt, send, IQ, app-state, retry, identity, prekey). Durations use the pluggable wacore::time::Instant so WASM/deterministic builds are unaffected. RetryReason gained a stable as_str() for the reason label. Verified: clippy --all-targets -- -D warnings clean both with and without --features metrics; fmt clean; cargo test --workspace --exclude e2e-tests green (1992 passed, 0 failed; additive and cfg-gated, no behavior change).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a feature-gated telemetry facade (wacore::telemetry), wires a workspace-level ChangesMetrics Instrumentation & Observability
Sequence Diagram(s)sequenceDiagram
participant App as Client Application
participant Lifecycle as Lifecycle
participant Message as Message Handler
participant Request as IQ Handler
participant Telemetry as wacore::telemetry
App->>Lifecycle: connect()
Lifecycle->>Telemetry: timer(CONNECT_DURATION)
Lifecycle->>Telemetry: connect("ok" | "fail")
App->>Message: process_session_enc_batch()
Message->>Telemetry: timer(DECRYPT_DURATION)
Message->>Telemetry: recv("decrypted")
App->>Request: send_and_wait_iq()
Request->>Telemetry: timer(IQ_DURATION)
Request->>Telemetry: iq(status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 4m 32s —— View job Claude PR Review — feat(metrics)Nice surface — the Findings1. let _t = wacore::telemetry::timer(wacore::telemetry::APPSTATE_SYNC_DURATION);
// ...
Ok(()) => { wacore::telemetry::appstate_sync("ok"); return Ok(()); }
Err(e) => { /* ... */ return Err(e); } // ← no appstate_sync("fail")The duration histogram captures both branches but only the 2. let _t = wacore::telemetry::timer(wacore::telemetry::IQ_DURATION); // L211
if !self.is_running.load(...) { return Err(IqError::NotConnected); } // L212 — bypasses counter
// ...
if let Err(e) = send_fn.await { /* Socket / EncryptSend errors */ return ... } // L231 — bypasses counter
// ...
wacore::telemetry::iq(match &result { Ok(_) => "ok", Err(IqError::Timeout) => "timeout", Err(_) => "error" }); // L263The histogram ticks on every path (good), but the counter only ticks after the 3. The docstring on async fn run_retry_receipt(...) -> bool {
wacore::telemetry::retry_receipt(reason.as_str()); // ← unconditional
let Some(retry_count) = self.increment_retry_count(...).await else {
// max retries hit → PDO instead, no receipt sent
...
};
let retry_sent = match self.send_retry_receipt(...).await { Ok(()) => true, Err(_) => false };This increments for capped-out attempts (which send a PDO, not a receipt) and for failed sends. Either move the emit past the cap check + send success, or rename the metric and docstring to "retry-receipt attempts". 4. wacore::telemetry::send(match to.server {
wacore_binary::Server::Group => "group",
wacore_binary::Server::Broadcast => "status",
_ => "dm", // ← Newsletter / Hosted / Bot / Messenger / Interop all bucketed as "dm"
});
5.
6.
7. Documented Docstring lists 8. The counter ticks at the top of Smaller things
VerdictThe shape (feature-gated Happy to apply any subset on this branch if you confirm. Apply findings 1-4 →
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd29059c9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )] | ||
| pub async fn disconnect(self: &Arc<Self>) { | ||
| info!("Disconnecting client intentionally."); | ||
| wacore::telemetry::set_connected(false); |
There was a problem hiding this comment.
When the socket drops through the normal run-loop cleanup path (for example a read loop error, stream error, or auto-reconnect) run() calls cleanup_connection_state() without going through disconnect(), so this new wa_connected gauge remains at 1 for the entire offline/backoff window and can stay stale after shutdown with auto-reconnect disabled. Since cleanup_connection_state() is the authoritative place that clears is_connected, the gauge needs to be cleared there rather than only in the explicit disconnect API.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| }; | ||
| wacore::telemetry::iq(match &result { |
There was a problem hiding this comment.
With the counter only emitted after the select!, any IQ whose send_fn fails (socket error, encrypt/send error, or not connected after the waiter was registered) returns above this point and never increments wa_iq_total{result="error"}. Those are completed IQ attempts at the same chokepoint, so dashboards will undercount IQ errors exactly during connection/send failures.
Useful? React with 👍 / 👎.
| match res { | ||
| Ok(()) => return Ok(()), | ||
| Ok(()) => { | ||
| wacore::telemetry::appstate_sync("ok"); |
There was a problem hiding this comment.
This records only successful app-state syncs; when process_app_state_sync_task ultimately returns a non-retryable error (or a DB-lock error after the retry limit), the function returns Err(e) later without ever calling appstate_sync("fail"). The advertised wa_appstate_sync_total{outcome} metric therefore has no failure samples, hiding failed syncs from metrics users.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/lifecycle.rs (1)
575-680:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: The
wa_connectedgauge isn't being reset when the connection drops unexpectedly.Look, when
cleanup_connection_state()clearsis_connectedat line 602, it needs to also callwacore::telemetry::set_connected(false). Right now if the read loop crashes or the transport dies, we clean up the atomic state but leave the Prometheus gauge sitting at 1.0. That means your monitoring dashboards will show the client connected when it's actually offline.The
disconnect()path works fine because you callset_connected(false)at line 482 before cleanup. But whenrun()calls cleanup directly at line 314 after an unexpected disconnect, the gauge never gets cleared.This is a data integrity issue—the telemetry layer and the connection state must stay synchronized. Production monitoring depends on these metrics being accurate.
🔧 Fix: Add telemetry marker to cleanup
pub(crate) async fn cleanup_connection_state(&self) { + // Clear the connected gauge to match is_connected atomic state. + // Ensures wa_connected stays synchronized on both expected and unexpected disconnects. + wacore::telemetry::set_connected(false); + // Note: node_waiters are intentionally NOT cleared here — they are🤖 Prompt for 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. In `@src/client/lifecycle.rs` around lines 575 - 680, The cleanup_connection_state() path clears the atomic is_connected flag but does not update the Prometheus gauge; add a call to wacore::telemetry::set_connected(false) immediately after the line that sets self.is_connected.store(false, Ordering::Release) so telemetry stays in sync on unexpected disconnects (ensure the wacore::telemetry symbol is in scope or fully-qualified if necessary).
🤖 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 `@src/client/app_state.rs`:
- Around line 74-77: The code only emits a success metric via
wacore::telemetry::appstate_sync("ok") on the OK path; update the
error/early-return paths to emit a failure metric as well (e.g., call
wacore::telemetry::appstate_sync("error") immediately before every Err(e) return
in the same function), specifically add the call just before the terminal return
that currently returns Err(e) (and any other early Err returns) so failures are
counted alongside successes.
In `@src/handlers/notification.rs`:
- Line 312: The telemetry call wacore::telemetry::identity_change() is invoked
too early and currently counts every identity-change push received rather than
only those that pass validation and trigger a session reset; move the telemetry
invocation inside the branch gated by had_prior_identity (the logic that
actually performs the session reset) after the validation checks so it only
increments for processed identity changes, or alternatively rename the metric to
reflect it tracks received notifications (e.g., identity_change_received) if you
intend to keep the current placement.
In `@src/message/receive.rs`:
- Line 512: The DECRYPT_DURATION timer in process_session_enc_batch is started
before awaiting per-sender locks (session_lock_for(...).await /
lock_arc().await), so wa_decrypt_duration_seconds currently includes lock wait
time; either move the let _t =
wacore::telemetry::timer(wacore::telemetry::DECRYPT_DURATION) line to after the
session_guard is acquired (i.e., after session_lock_for and lock_arc complete)
to measure crypto-only time, or rename/update the metric/constant
(DECRYPT_DURATION / wa_decrypt_duration_seconds) and its documentation to
explicitly state it includes lock/queue wait time so the metric semantics are
correct.
In `@src/message/retry.rs`:
- Line 218: The telemetry call wacore::telemetry::retry_receipt is being emitted
too early; change the flow so you only increment the retry_receipt metric after
a retry receipt is actually sent and succeeds: call
wacore::telemetry::retry_receipt inside the branch where
increment_retry_count(Some/Ok) returns a counter and where the subsequent send
of the retry receipt (the code path that currently follows that check, not the
PDO fallback) succeeds; leave the PDO fallback branch unchanged (or
alternatively rename the metric to wa_retry_attempt_total if you intend to count
attempts rather than sent receipts). Ensure you update the location around
increment_retry_count and the send/retry-success branch so the metric is not
incremented before the retry-cap check or before the send completes.
In `@src/prekeys.rs`:
- Around line 162-164: The login path calls upload_pre_keys_inner() directly
from upload_pre_keys_at_login(), bypassing the telemetry emitted by
upload_pre_keys(); modify upload_pre_keys_at_login() to either (A) wrap the
inner call with the same telemetry call (use wacore::telemetry::prekey_upload
with "ok"/"fail" based on the Result returned by upload_pre_keys_inner()), or
(B, preferred) call the instrumented wrapper upload_pre_keys(true).await instead
of upload_pre_keys_inner().await so the prekey_upload metric is always emitted
for login flows—if you choose option B, confirm that passing force=true
preserves the intended login semantics (i.e., bypasses the count-guard) before
changing the call.
- Around line 162-164: The telemetry is emitted per attempt in upload_pre_keys
(the block calling upload_pre_keys_inner and wacore::telemetry::prekey_upload),
causing multiple metrics for retries; move the wacore::telemetry::prekey_upload
call out of upload_pre_keys_inner/upload_pre_keys and into
upload_pre_keys_with_retry so only the final operation outcome is recorded:
remove the per-attempt prekey_upload emission from upload_pre_keys/_inner and
have upload_pre_keys_with_retry call wacore::telemetry::prekey_upload(if
final_result.is_ok() { "ok" } else { "fail" }) after retries complete.
In `@wacore/src/protocol/retry.rs`:
- Around line 64-84: Add an exhaustive unit test for RetryReason::as_str() that
asserts each enum variant (UnknownError, NoSession, InvalidKey, InvalidKeyId,
InvalidMessage, InvalidSignature, FutureMessage, BadMac, InvalidSession,
InvalidMsgKey, BadBroadcastEphemeralSetting, UnknownCompanionNoPrekey,
AdvFailure, StatusRevokeDelay) returns the exact expected static string;
implement the test by explicitly listing every variant and comparing
variant.as_str() to its canonical label (e.g.,
RetryReason::UnknownError.as_str() == "unknown"), and mark the test as failing
if any mapping changes or if new variants are added so maintainers must update
the expected labels accordingly.
---
Outside diff comments:
In `@src/client/lifecycle.rs`:
- Around line 575-680: The cleanup_connection_state() path clears the atomic
is_connected flag but does not update the Prometheus gauge; add a call to
wacore::telemetry::set_connected(false) immediately after the line that sets
self.is_connected.store(false, Ordering::Release) so telemetry stays in sync on
unexpected disconnects (ensure the wacore::telemetry symbol is in scope or
fully-qualified if necessary).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 619373a8-2520-4bce-a3c2-7172926d7f8a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlexamples/metrics.rssrc/client/app_state.rssrc/client/lifecycle.rssrc/client/node_io.rssrc/handlers/notification.rssrc/lib.rssrc/message/dispatch.rssrc/message/receive.rssrc/message/retry.rssrc/prekeys.rssrc/request.rssrc/send.rswacore/Cargo.tomlwacore/src/lib.rswacore/src/protocol/retry.rswacore/src/telemetry.rs
Benchmark Results67 unchanged benchmark(s)
|
Addresses the Codex/CodeRabbit/Claude review: several counters recorded a duration but no outcome, or counted attempts instead of completed operations. - wa_connected: cleared in cleanup_connection_state() so it drops on every disconnect (run-loop drop / reconnect), not only the explicit disconnect() API. - wa_iq_total: emit "error" on the three early-return paths (NotConnected, send failure) so the counter matches the duration histogram on every exit. - wa_appstate_sync_total: emit "fail" on the terminal Err so the success rate isn't pinned at 100%; also wire wa_appstate_mutations_total at the two mutation-dispatch sites (it was declared but never emitted). - wa_retry_receipt_total: move the emit into the send-success branch so it counts receipts actually sent, not capped-out attempts (which send a PDO) or failed sends. Matches the "Retry receipt sent" docstring. - wa_send_total: add a "newsletter" arm (was bucketed as "dm") and count status posts in send_status_message. Deliberately not instrumenting send_message_impl: it is shared with internal protocol traffic (PDO, app-state key requests) that must not inflate send counts. - wa_prekey_upload_total: emit once per logical operation (the retry wrapper's final outcome + the login path) instead of once per retry attempt. - wa_identity_change_total: count past the companion/self/no-prior-identity gates so it reflects actual session resets, not every push received; describe updated. - wa_decrypt_duration_seconds: start the timer after the per-sender session lock so it measures crypto time, not lock/queue contention. - Drop the unused wa_pending_retries gauge (high-churn transient state, no call site). - Add an exhaustive RetryReason::as_str() stability test (label drift guard). Verified: clippy --all-targets -- -D warnings clean with and without --features metrics; fmt clean; cargo test --workspace --exclude e2e-tests green.
|
Docs PR opened: oxidezap/whatsapp-rust-docs#270 Added an Advanced page documenting the new opt-in metrics feature, its wa_* metric catalogue, recorder wiring, and overhead. |
1 participant
What
Adds an opt-in
metricsfeature that emitswa_*metrics through themetricsfacade for rates and latency percentiles, complementing the tracing spans from #733. Together: spans give the trace of one case, metrics give the rates/percentiles for dashboards.Off by default: with the feature disabled there is no
metricsdependency and zero overhead - every emit is an inlined no-op and the durationTimeris a zero-sized type that reads no clock. The library only emits; the application installs a recorder (Prometheus, OTLP, ...). Seeexamples/metrics.rs.Design
Same philosophy as the tracing work: the library emits through a facade, the app owns the recorder; no Prometheus/OTLP dependency in the library.
metrics = { optional = true }inwacore; featuremetrics = ["wacore/metrics"]on the main crate, not indefault.wacore::telemetry(re-exported aswhatsapp_rust::telemetry) provides typed helpers (counters/gauges + a record-on-dropTimer), so call sites stay clean. Durations use the pluggablewacore::time::Instantso WASM/deterministic builds are unaffected.PII / cardinality
Labels are strictly low-cardinality categorical values (
outcome/kind/result/reason). A JID, phone number or message id is never used as a label - it would explode the metrics backend and leak PII. The duration histograms are unlabeled (the matching_totalcounter carries the categorical breakdown).Metrics
Counters:
wa_recv_total{outcome},wa_send_total{kind},wa_retry_receipt_total{reason},wa_iq_total{result},wa_reconnect_total,wa_stream_error_total,wa_connect_total{outcome},wa_appstate_sync_total{outcome},wa_identity_change_total,wa_prekey_upload_total{outcome}.Histograms:
wa_iq_duration_seconds,wa_connect_duration_seconds,wa_decrypt_duration_seconds,wa_send_duration_seconds,wa_appstate_sync_duration_seconds.Gauges:
wa_connected(plus awa_pending_retrieshelper).Emitted at the same boundaries as the
wa.*spans.RetryReasongained a stableas_str()for the reason label.Verification
cargo clippy --all-targets -- -D warningsclean both with and without--features metrics.cargo fmt --all -- --checkclean.cargo test --workspace --exclude e2e-tests: 1992 passed, 0 failed. The instrumentation is additive and cfg-gated, so no behavior change.Usage