feat: add chainlink unique pubkey cardinality metric#1383
Conversation
* master: feat: add pending fetch metrics (#1377)
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a HyperLogLog-based Sequence Diagram(s)See the diagrams embedded in the hidden review stack artifact above. Suggested reviewers: ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
1959-1995: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEstimator observation runs while the broad
subscription_ownershiplock is still held.In the
SubscriptionAlreadyPresentbranch,self.unique_pubkey_estimator.observe(...)(Lines 1981-1985) executes beforeownership(thesubscription_ownershipasyncMutexGuard, which serializes ALL pubkeys' ownership state) goes out of scope.observe()internally takes a separatestd::sync::Mutexand, whenshould_export()fires, performs the periodic multi-series export/Prometheus-set work — all whilesubscription_ownershipremains locked, blocking every other concurrentacquire_subscription/release_subscription/register_subscriptioncall across the provider.This is inconsistent with the three sibling call sites in
register_subscription(Lines 1727-1743, 1797-1806), which all observe only after theadd_outcomeblock has already dropped itsownershipguard. Move the observe call afterdrop(ownership)here too.🔧 Proposed fix
let mut ownership = self.subscription_ownership.lock().await; if let Some(existing) = ownership.get_mut(pubkey) { if !skip_existing_reason || !existing.contains(reason) { existing.acquire(reason); } self.lrucache_subscribed_accounts.add(*pubkey); inc_chainlink_subscription_registration_accounts( origin, reason.into(), SubscriptionRegistrationOutcome::AlreadyPresent, ); - self.unique_pubkey_estimator.observe( - origin, - UniquePubkeyStage::SubscriptionAlreadyPresent, - pubkey, - ); + drop(ownership); + self.unique_pubkey_estimator.observe( + origin, + UniquePubkeyStage::SubscriptionAlreadyPresent, + pubkey, + ); return Ok(()); } drop(ownership);🤖 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 `@magicblock-chainlink/src/remote_account_provider/mod.rs` around lines 1959 - 1995, The SubscriptionAlreadyPresent path in acquire_subscription_with_mode holds the broad subscription_ownership MutexGuard while calling unique_pubkey_estimator.observe, which can block other subscription operations during export work. Move the observe call so it runs only after the ownership guard is dropped, matching the register_subscription call sites and keeping the lock scope minimal.
🤖 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 `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 820-824: The CloneFailed unique-pubkey metric is only recorded in
clone_account_with_ownership, but the equivalent failure branches in
clone_program_with_ownership and
clone_account_and_schedule_undelegation_with_ownership are missing the same
observation. Update both sibling clone-failure paths to call
unique_pubkey_estimator().observe(fetch_origin,
ChainlinkCloneOutcome::CloneFailed, &pubkey) alongside their existing failure
handling so all live clone failure paths report consistently.
In `@magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs`:
- Around line 71-82: The `Default` implementation for `UniquePubkeyEstimator` is
calling `force_export_for_tests(0)` even though the newly constructed `series`
is empty, so the `dev-context` path has no effect. Remove this no-op from
`Default`, or if the goal is test isolation, make the reset explicit in
`force_export_for_tests`/`export_series` by clearing or zeroing the known
`UNIQUE_PUBKEY_STAGES` and origin combinations against the registry.
- Around line 106-141: The hot-path export in observe_at is holding the shared
self.series mutex while calling export_series, which can block all
observe/observe_many callers across origin/stage pairs. Refactor
UniquePubkeyEstimator::observe_at to snapshot the needed series data while the
lock is held, then drop the guard before performing Prometheus writes via
export_series/set_chainlink_unique_pubkeys_estimate. Keep the lock scope limited
to updating series, and use the existing observe_at/export_series flow to locate
the change.
- Around line 119-136: Replace the check-then-insert-then-get flow in
unique_pubkey_estimator::observe with entry() so the series and stage_series
maps are initialized and retrieved without `.expect()` panics. Use the existing
`series` lock and the `origin_label`/`stage_label` keys to perform in-place
insertion via `HashMap::entry`, then call `UniquePubkeySeries::new` and
`observe_hash` on the returned mutable reference. Keep the behavior the same,
but remove the `.expect()` lookups in the `series` and `stage_series` access
path.
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 210-219: The Chainlink metric definition in
CHAINLINK_UNIQUE_PUBKEYS_ESTIMATE uses IntGaugeVec::new(...).unwrap(), which can
panic during startup if initialization fails. Keep the same pattern as the
surrounding metric definitions in mod.rs, but add an explicit justification in
the code or surrounding comment that this invariant is guaranteed; if you prefer
to harden it, route the initialization through the existing registration/init
path used for metrics so the failure is handled instead of unwrapping.
---
Outside diff comments:
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 1959-1995: The SubscriptionAlreadyPresent path in
acquire_subscription_with_mode holds the broad subscription_ownership MutexGuard
while calling unique_pubkey_estimator.observe, which can block other
subscription operations during export work. Move the observe call so it runs
only after the ownership guard is dropped, matching the register_subscription
call sites and keeping the lock scope minimal.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d1b3b1-b12e-4883-9bbb-51db5a093ba4
📒 Files selected for processing (10)
.agents/context/crates/magicblock-chainlink.md.agents/context/crates/magicblock-metrics.mdmagicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/fetch_cloner/tests.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-chainlink/src/chainlink/unique_pubkey_estimator.rsmagicblock-chainlink/src/remote_account_provider/mod.rsmagicblock-chainlink/src/remote_account_provider/tests.rsmagicblock-metrics/src/metrics/mod.rsmagicblock-metrics/src/metrics/types.rs
Amp-Thread-ID: https://ampcode.com/threads/T-019f225e-8224-765c-ba4e-6985c3edd79f Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019f225e-8224-765c-ba4e-6985c3edd79f Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019f2266-6bc4-729a-9f45-96e55b52e7aa Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019f2266-6bc4-729a-9f45-96e55b52e7aa Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs (2)
2509-2515: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
clone_as_emptyinRemoteNotFound.Line 2509 observes only the post-partition
not_foundset.clone_as_emptyentries were also remoteNotFoundresults, so they disappear from theRemoteNotFoundunique-pubkey estimate.Proposed fix
self.remote_account_provider .unique_pubkey_estimator() .observe_many( fetch_origin, UniquePubkeyStage::RemoteNotFound, - not_found.iter().map(|(pubkey, _)| pubkey), + not_found + .iter() + .chain(clone_as_empty.iter()) + .map(|(pubkey, _)| pubkey), );🤖 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 `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 2509 - 2515, The RemoteNotFound unique-pubkey observation in the fetch cloner only uses the post-partition not_found set, so clone_as_empty results are omitted. Update the observation around self.remote_account_provider.unique_pubkey_estimator().observe_many in fetch_cloner/mod.rs to include pubkeys from clone_as_empty as well, so both not_found and clone_as_empty are counted under UniquePubkeyStage::RemoteNotFound.
2428-2434: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror remote-fetch observations for action dependency fetches.
Line 2428 instruments
fetch_accounts, but Line 2662 performs another batchedtry_get_multifor action dependencies and Line 2679 classifies its results withoutRemoteFetch/RemoteNotFoundobservations. Those dependency pubkeys will be missing from the new stage metric.Proposed addition for the direct dependency fetch path
self.fetch_count.fetch_add( action_dependencies_to_fetch.len() as u64, Ordering::Relaxed, ); + self.remote_account_provider + .unique_pubkey_estimator() + .observe_many( + fetch_origin, + UniquePubkeyStage::RemoteFetch, + action_dependencies_to_fetch.iter(), + ); let action_dep_accs = self .remote_account_provider .try_get_multi( &action_dependencies_to_fetch, None, @@ let ClassifiedAccounts { not_found, plain, owned_by_deleg, programs, atas, } = pipeline::classify_remote_accounts( action_dep_accs, &action_dependencies_to_fetch, ); + self.remote_account_provider + .unique_pubkey_estimator() + .observe_many( + fetch_origin, + UniquePubkeyStage::RemoteNotFound, + not_found.iter().map(|(pubkey, _)| pubkey), + );🤖 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 `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs` around lines 2428 - 2434, Mirror the remote-fetch metric observations for the action-dependency fetch path: the `fetch_accounts` flow already calls `unique_pubkey_estimator().observe_many(...)` with `UniquePubkeyStage::RemoteFetch`, but the `try_get_multi` logic used for action dependencies does not record the same `RemoteFetch`/`RemoteNotFound` outcomes. Update the action-dependency handling in `fetch_cloner::mod.rs` so that the results classified after the dependency `try_get_multi` also feed `observe_many` (or equivalent per-result observation) using the same `remote_account_provider` and `UniquePubkeyStage` values, keeping the dependency pubkeys visible in the stage metric.magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs (1)
97-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExport after the whole
observe_manybatch.Line 129 snapshots immediately after the current
observe_atcall. Whenobserve_manycrosses the export interval, the first pubkey in the batch can export before the remaining pubkeys are inserted, under-reporting the gauge until the next export.Proposed direction
pub(crate) fn observe_many<'a>( &self, origin: impl LabelValue + Copy, stage: impl LabelValue + Copy, pubkeys: impl IntoIterator<Item = &'a Pubkey>, ) { let now_epoch_seconds = now_epoch_seconds(); - for pubkey in pubkeys { - self.observe_at(origin, stage, pubkey, now_epoch_seconds); + let pubkey_hashes = pubkeys.into_iter().map(hash_pubkey).collect::<Vec<_>>(); + if pubkey_hashes.is_empty() { + return; } + self.observe_hashes_at(origin, stage, pubkey_hashes, now_epoch_seconds); } + + fn observe_hashes_at( + &self, + origin: impl LabelValue, + stage: impl LabelValue, + pubkey_hashes: impl IntoIterator<Item = u64>, + now_epoch_seconds: u64, + ) { + let epoch_minute = now_epoch_seconds / BUCKET_SECONDS; + let origin_label = origin.value(); + let stage_label = stage.value(); + let should_export = self.should_export(now_epoch_seconds); + + let mut series = + self.series.lock().unwrap_or_else(|err| err.into_inner()); + let stage_series = series.entry(origin_label.to_string()).or_default(); + let pubkey_series = stage_series + .entry(stage_label.to_string()) + .or_insert_with(|| UniquePubkeySeries::new(origin_label, stage_label)); + + for pubkey_hash in pubkey_hashes { + pubkey_series.observe_hash(epoch_minute, pubkey_hash); + } + + let export_estimates = + should_export.then(|| collect_series_estimates(&series, epoch_minute)); + drop(series); + + if let Some(estimates) = export_estimates { + write_series_estimates(estimates); + } + }Also applies to: 126-130
🤖 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 `@magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs` around lines 97 - 100, The batch update in observe_many currently lets observe_at trigger export per pubkey, which can snapshot before the full batch is inserted. Change observe_many so it records all pubkeys first and only performs the export once after the loop, using the same timestamp; keep the export logic in observe_at gated so it does not run during the per-item inserts. Use observe_many and observe_at in unique_pubkey_estimator.rs as the main symbols to update.
🤖 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.
Outside diff comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs`:
- Around line 2509-2515: The RemoteNotFound unique-pubkey observation in the
fetch cloner only uses the post-partition not_found set, so clone_as_empty
results are omitted. Update the observation around
self.remote_account_provider.unique_pubkey_estimator().observe_many in
fetch_cloner/mod.rs to include pubkeys from clone_as_empty as well, so both
not_found and clone_as_empty are counted under
UniquePubkeyStage::RemoteNotFound.
- Around line 2428-2434: Mirror the remote-fetch metric observations for the
action-dependency fetch path: the `fetch_accounts` flow already calls
`unique_pubkey_estimator().observe_many(...)` with
`UniquePubkeyStage::RemoteFetch`, but the `try_get_multi` logic used for action
dependencies does not record the same `RemoteFetch`/`RemoteNotFound` outcomes.
Update the action-dependency handling in `fetch_cloner::mod.rs` so that the
results classified after the dependency `try_get_multi` also feed `observe_many`
(or equivalent per-result observation) using the same `remote_account_provider`
and `UniquePubkeyStage` values, keeping the dependency pubkeys visible in the
stage metric.
In `@magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs`:
- Around line 97-100: The batch update in observe_many currently lets observe_at
trigger export per pubkey, which can snapshot before the full batch is inserted.
Change observe_many so it records all pubkeys first and only performs the export
once after the loop, using the same timestamp; keep the export logic in
observe_at gated so it does not run during the per-item inserts. Use
observe_many and observe_at in unique_pubkey_estimator.rs as the main symbols to
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f19d6e1-a6ac-4944-9b07-bf067d57a4b7
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs
Amp-Thread-ID: https://ampcode.com/threads/T-019f2272-19c8-7408-84e8-451c3c36ec8a Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs`:
- Around line 391-405: The `unique_pubkey_estimator` test currently lets
`ChainlinkUniquePubkeyWindow::OneMinute` pass with a loose lower bound, so it
won’t catch stale pubkey leakage. Update the assertions in the test around
`chainlink_unique_pubkeys_estimate_value` to make the `OneMinute` check stricter
by comparing it against the `FiveMinutes` estimate or asserting it is exactly 1,
using the existing `origin`, `stage`, and `ChainlinkUniquePubkeyWindow` symbols
to keep the rotation regression test meaningful.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c67f090-8088-4547-8ea9-8686ee82ac06
📒 Files selected for processing (1)
magicblock-chainlink/src/chainlink/unique_pubkey_estimator.rs
1 participant
Summary
Adds a bounded, low-cardinality estimate of unique Chainlink pubkeys flowing
through each account-sync stage, exported as the Prometheus gauge
mbv_chainlink_unique_pubkeys_estimate{origin,stage,window}. This lets usdistinguish "many requests for a few hot accounts" from "many distinct
accounts" when reading the remote-fetch / eviction graphs, without ever putting
pubkeys or signatures into label values.
This PR addresses issue #1366
IMPORTANT NOTE
I'm not sure about this one (which is why I implemented it last).
It is only really useful if we have to understand how many times we repeatedly access
accounts with the same pubkeys vs. unique ones.
If that detail is not important for the issue we are trying to diagnose then I'd say the
complexity + amount of changes are a bit over the top.
We should carefully decide if we want to merge it.
Details
The new metric estimates unique pubkey cardinality per
(origin, stage)overrolling 1m / 5m / 1h windows. The estimator is a fixed-memory
HyperLogLog-style sketch with rotating minute buckets, so memory is bounded by
the number of active
(origin, stage)series and a fixed 60 sketches perseries — never by the number of distinct pubkeys observed. Pubkeys are hashed
only inside the estimator and are never exported as labels.
Stage labels reuse the existing current-master Chainlink label enums wherever
an exact stage already exists (
ChainlinkCloneOutcome::CloneFailed,ChainlinkCloneMaterializationOutcome::StillMissingAfterEnsure); only stagesthat had no existing label live in the new Chainlink-local
UniquePubkeyStageenum. The estimator, metric setter, and dev getter all accept generic
LabelValueorigin/stage arguments so no stage enums were duplicated inmagicblock-metrics.magicblock-metrics
ChainlinkUniquePubkeyWindowwindow label type (1m/5m/1h)with
LabelValue/Display.chainlink_unique_pubkeys_estimate{origin,stage,window}gauge(scraped as
mbv_chainlink_unique_pubkeys_estimate), itsregister()entry,the generic
set_chainlink_unique_pubkeys_estimate(...)setter, and adev/test-only getter — all reusing existing bounded label enums for
originand
stage.magicblock-chainlink
unique_pubkey_estimatorwith the bounded HLL estimator, rotatingminute buckets, throttled export (
EXPORT_INTERVAL_SECONDS), and theChainlink-local
UniquePubkeyStageenum for stages without an existing label.RemoteAccountProvider(viaArc) and is reachablethrough a
FetchCloneraccessor.populated at least once:
requested_by_rpc,bank_miss,remote_fetch,remote_not_found,companion_fetchin the fetch/clone path.subscription_already_present,subscription_added,subscription_evictedon terminal subscription outcomes.clone_failed(reusingChainlinkCloneOutcome::CloneFailed) on ownerclone failure.
still_missing_after_ensure(reusingChainlinkCloneMaterializationOutcome::StillMissingAfterEnsure) afterensure completes.
send_transactionorigin) plus metric-observation tests at the fetch/clone,subscription, and post-ensure call sites.
Example PromQL
Unique accounts hitting the base-chain remote fetch over the last 5 minutes,
broken down by origin:
Compare unique remote-fetch accounts against total remote fetches to spot
hot-account amplification (few unique accounts, many fetches):
Summary by CodeRabbit