refactor: Export the monotonic consumed cycles total as the existing gauge - #11416
refactor: Export the monotonic consumed cycles total as the existing gauge#11416mraszyk wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Online subnet splits can decrease the exported counter and invalidate the prepayment invariant.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a persistent monotonic consumed-cycles counter, migration logic, Prometheus export, and coverage across state loading and scheduling.
Changes:
- Tracks and serializes
consumed_cycles_as_counter. - Backfills historical values from gauges and outstanding prepayments.
- Exports and tests the aggregate counter.
File summaries
| File | Description |
|---|---|
rs/state_manager/src/tip.rs |
Serializes the counter. |
rs/state_manager/src/checkpoint.rs |
Restores the counter. |
rs/state_layout/src/state_layout/tests.rs |
Tests protobuf compatibility. |
rs/state_layout/src/state_layout/proto.rs |
Converts the protobuf field. |
rs/state_layout/src/state_layout.rs |
Adds persisted state. |
rs/replicated_state/src/metrics.rs |
Exports the aggregate counter. |
rs/replicated_state/src/canister_state/tests.rs |
Tests accounting and migration. |
rs/replicated_state/src/canister_state/system_state.rs |
Implements counter accounting and derivation. |
rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs |
Updates generated protobuf types. |
rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto |
Defines the protobuf field. |
rs/execution_environment/src/scheduler/tests/metrics.rs |
Tests scheduler migration and export. |
rs/execution_environment/src/scheduler.rs |
Runs checkpoint backfills. |
rs/execution_environment/src/canister_manager.rs |
Documents deletion accounting. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let consumed_cycles_as_counter = state.metadata.subnet_metrics.consumed_cycles_total() | ||
| + consumed_cycles_by_canisters_as_counter; | ||
| self.consumed_cycles_as_counter.reset(); | ||
| self.consumed_cycles_as_counter | ||
| .inc_by(consumed_cycles_as_counter.get() as f64); |
There was a problem hiding this comment.
The premise is right -- an online split does lower this total -- but neither suggested remedy is the one we want, so this stays as is, now with a comment explaining why.
Transferring the migrated canisters' consumption into a monotonic subnet accumulator would count it twice: subnet B keeps those canisters and their counters, so subnet A' would report consumption that is still being reported next door. It would also change SubnetMetrics::consumed_cycles_total(), which feeds the certified /subnet/<subnet_id>/metrics from CertificationVersion::V29 -- not something to change as a side effect of adding a metric.
A process-local baseline would keep the exported number monotone at the cost of permanently diverging from the state it reports, and would silently mask any genuine regression in the underlying value. The debug_assert in migrate_consumed_cycles_to_counters exists precisely to catch those.
What is left is a counter reset at an online split, which Prometheus handles: rate()/increase() recognise the reset and lose one interval's increment. Worth accepting, because the drop is not an artefact -- the certified consumed_cycles_total_including_canisters that this metric mirrors drops for exactly the same reason, as does the existing replicated_state_consumed_cycles_from_replica_start_as_counters, which folds the same per-canister counters into a CounterVec. Making this one metric split-proof in isolation would leave it inconsistent with both.
There was a problem hiding this comment.
🟡 Changes recommended
The migration invariant documentation contains two materially inaccurate descriptions of paused and split execution handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The consensus-critical cycle accounting and checkpoint migration require final human validation despite comprehensive tests.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
alin-at-dfinity
left a comment
alin-at-dfinity
left a comment
There was a problem hiding this comment.
Logic and tests look sane.
But man, is this verbose! I'm fine with leaving any temporary migration, assert and test code (and their comments) as is, but I would try to cut down on noise for everything that's here to stay.
| /// The value that [`CanisterMetrics::consumed_cycles_as_counter`] must have, | ||
| /// derived from the [`CanisterMetrics::consumed_cycles`] gauge, which predates | ||
| /// it and thus holds the full history. | ||
| /// | ||
| /// This derivation is exact, thanks to the invariant documented on | ||
| /// [`Self::outstanding_prepayments`]: the gauge differs from the counter by | ||
| /// exactly the prepayments whose refund is still outstanding, all of which are | ||
| /// recorded in the replicated state. And because the invariant holds at all | ||
| /// times (not just before the first observation of a canister), deriving the | ||
| /// counter this way is idempotent, so it is safe to redo it in every round and | ||
| /// after a downgrade has dropped the counter. | ||
| /// | ||
| /// Returns `None` if the canister has a paused execution whose prepayment is not | ||
| /// part of the replicated state (see [`Self::outstanding_prepayments`]). | ||
| pub fn migrated_consumed_cycles_as_counter(&self) -> Option<NominalCycles> { | ||
| let outstanding = self.outstanding_prepayments()?; | ||
| // `max` rather than a plain assignment: the counter must never go down, not | ||
| // even if a saturating subtraction somewhere made the gauge lag behind it. | ||
| Some( | ||
| self.canister_metrics | ||
| .consumed_cycles_as_counter | ||
| .max(self.canister_metrics.consumed_cycles - outstanding), | ||
| ) | ||
| } | ||
|
|
||
| /// Backfills [`CanisterMetrics::consumed_cycles_as_counter`] with | ||
| /// [`Self::migrated_consumed_cycles_as_counter`]. Returns `false` (leaving the | ||
| /// counter untouched) if the latter cannot be derived; the caller is expected to | ||
| /// retry once paused executions have been aborted. | ||
| pub fn migrate_consumed_cycles_to_counter(&mut self) -> bool { | ||
| match self.migrated_consumed_cycles_as_counter() { | ||
| Some(counter) => { | ||
| self.canister_metrics.consumed_cycles_as_counter = counter; | ||
| true | ||
| } | ||
| None => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry Claude, this is stupidly verbose. migrated_consumed_cycles_as_counter can be trivially inlined; and it's not used anywhere else.
There was a problem hiding this comment.
Done, inlined in 2339379. migrate_consumed_cycles_to_counter now reads:
pub fn migrate_consumed_cycles_to_counter(&mut self) -> bool {
let Some(outstanding) = self.outstanding_prepayments() else {
return false;
};
// `max` rather than a plain assignment: the counter must never go down, not
// even if a saturating subtraction somewhere made the gauge lag behind it.
self.canister_metrics.consumed_cycles_as_counter = self
.canister_metrics
.consumed_cycles_as_counter
.max(self.canister_metrics.consumed_cycles - outstanding);
true
}The doc comment of the removed method moved onto it, with the Returns None paragraph turned into the Returns false one.
| // Refund the execution cycles prepaid for the dropped `install_code` in full. | ||
| // The canister has nothing to show for them: an aborted execution discards the | ||
| // slices it had already run and starts over when retried, and this one is | ||
| // never retried -- subnet A' rejects the corresponding call, unwinding the | ||
| // operation there as well. | ||
| // | ||
| // This also keeps the consumed cycles metrics consistent. Dropping the task | ||
| // without refunding would leave the prepayment in the gauges with nothing left | ||
| // in the state to account for it, breaking the invariant on | ||
| // `Self::outstanding_prepayments`; and the monotonic counters, which only | ||
| // account for a prepayment once its refund is known, would never see it at | ||
| // all. A full refund lowers the gauges by the prepayment and adds nothing to | ||
| // the counters, which is exactly right: nothing was consumed. | ||
| if let Some(prepaid_execution_cycles) = self.task_queue.remove_aborted_install_code_task() { | ||
| self.refund_cycles(prepaid_execution_cycles, prepaid_execution_cycles); | ||
| } |
There was a problem hiding this comment.
I believe that everything after the first comment line can be safely dropped.
| // Refund the execution cycles prepaid for the dropped `install_code` in full. | |
| // The canister has nothing to show for them: an aborted execution discards the | |
| // slices it had already run and starts over when retried, and this one is | |
| // never retried -- subnet A' rejects the corresponding call, unwinding the | |
| // operation there as well. | |
| // | |
| // This also keeps the consumed cycles metrics consistent. Dropping the task | |
| // without refunding would leave the prepayment in the gauges with nothing left | |
| // in the state to account for it, breaking the invariant on | |
| // `Self::outstanding_prepayments`; and the monotonic counters, which only | |
| // account for a prepayment once its refund is known, would never see it at | |
| // all. A full refund lowers the gauges by the prepayment and adds nothing to | |
| // the counters, which is exactly right: nothing was consumed. | |
| if let Some(prepaid_execution_cycles) = self.task_queue.remove_aborted_install_code_task() { | |
| self.refund_cycles(prepaid_execution_cycles, prepaid_execution_cycles); | |
| } | |
| // Refund the execution cycles prepaid for the dropped `install_code` in full. | |
| if let Some(prepaid_execution_cycles) = self.task_queue.remove_aborted_install_code_task() { | |
| self.refund_cycles(prepaid_execution_cycles, prepaid_execution_cycles); | |
| } |
Better yet, the one sentence can be added to the first paragraph of the existing comment, something like "Remove aborted install code task and fully refund the [prepaid] cycles."
There was a problem hiding this comment.
Done in 2339379, taking the "better yet" variant -- the sentence is now part of the existing first paragraph:
// Remove aborted install code task and fully refund the prepaid execution
// cycles.
//
// Note that this cannot be a paused install code task, because we abort all
// paused tasks before triggering the split.
if let Some(prepaid_execution_cycles) = self.task_queue.remove_aborted_install_code_task() {
self.refund_cycles(prepaid_execution_cycles, prepaid_execution_cycles);
}| consumed_cycles_as_counter: metrics_registry.register( | ||
| Counter::new( | ||
| "replicated_state_consumed_cycles_since_replica_started_as_counter", |
There was a problem hiding this comment.
"As counter" is not a great name. It does mirror "by use cases as counters", but while the CanisterMetrics field does kind of act like a Prometheus counter, it is it is just a field in a struct (nothing to do with Prometheus counters).
And the actual Prometheus metric should emphatically not be a counter (as hinted to by the fact that we reset it to zero and then increment it every time): if the replica process restarts half-way through a checkpoint interval and then resumes from the checkpoint, the scraped time series will increase past the CanisterMetrics "counter" at the checkpoint height (say N), then reset back to N and continue from there. From the POV of Prometheus, the underlying value just jumped from N+k to 0 and then back to N, an increase of N cycles.
I.e. definitely make this a gauge. And ideally drop the _as_counter from everywhere. Just name it _monotonic.
Even better, there is already code that aliases the replicated_state_consumed_cycles_since_replica_started / replicated_state_consumed_cycles_from_replica_start to names that drop their misleading suffixes. Those alias names are not used anywhere (dashboards, alerts or other rules). So we could grab the chance to drop the unused aliases and export the monotonic counters cleanly as replicated_state_consumed_cycles[_by_use_case].
There was a problem hiding this comment.
And actually, since the new metric must be a gauge, we may as well avoid adding a new metric and instead simply export the new monotonic value under the old name(s).
That way we save significant effort on rules and dashboards (including the public dashboard): as said, the value is potentially not actually monotonic across replica restarts, so computing rates of increase and whatnot still requires the same high water mark approach we have for the current gauge. It's merely a much better behaved (in the average case) gauge than the existing one.
There was a problem hiding this comment.
Both points taken, and the second one supersedes the first, so 2339379 does exactly that: the new Prometheus metric is gone, and the monotonic total is now exported under the existing replicated_state_consumed_cycles_since_replica_started gauge (and, via the existing register_alias, replicated_state_consumed_cycles), in place of the certified consumed_cycles_total_including_canisters it used to carry.
// The monotonic counterpart of the certified
// `SubnetMetrics::consumed_cycles_total_including_canisters()`, from which it
// differs by exactly the prepayments whose refund is not known yet: the
// subnet-level aggregate (monotonic already, ...) plus the canisters' monotonic
// counters. Exported in place of the certified total, which it tracks closely,
// so that existing rules and dashboards need no change.
//
// Still a gauge, not a Prometheus `Counter`: the value is recomputed from the
// state, so a replica resuming from a checkpoint re-exports a lower value than
// it had reached before; and an online subnet split hands the migrating
// canisters' consumption over to the other subnet (retaining it here would
// count it on both). Both need the same high water mark treatment as the
// non-monotonic value did.
self.consumed_cycles.set(
(state.metadata.subnet_metrics.consumed_cycles_total()
+ consumed_cycles_by_canisters_as_counter)
.get() as f64,
);Your N+k → 0 → N argument is the decisive one and I had missed it: a Counter whose value is recomputed from the state on every observation is not a counter at all. The reset() + inc_by() pattern only works for a value the process itself accumulates, and this one is derived from a checkpoint the process may be resumed from. So no rules or dashboards to write, and no new metric to roll out -- just a strictly better behaved gauge under the name everything already scrapes. The help string now says so ("Monotonic, except across replica restarts and subnet splits: a prepayment is only accounted for once the refund it produces is known.").
The aliases stay as they are: since we are reusing the old names rather than introducing new ones, dropping them is unrelated to this PR (and they are still the in-flight half of the earlier rename).
What I have not done here is the _as_counter → _monotonic rename, in CanisterMetrics, the protobuf field and everywhere else. It touches the persisted field name and a good deal of surrounding code, so I would rather do it as its own commit/PR than bury it in this one -- happy to do it right away if you would prefer it in here.
Test-wise: consumed_cycles_total_as_counter_is_exported became consumed_cycles_total_is_exported_net_of_outstanding_prepayments, asserting exported == certified_total - outstanding; and consumed_cycles_gauge_accounts_for_all_subnet_level_use_cases no longer stages a canisters' part via refresh_consumed_cycles (the gauge folds the canisters directly now), with the canisters' part covered by the scheduler test above.
There was a problem hiding this comment.
🔵 Needs a closer look
The consensus-state migration and canister-balance change require human review, and metric contract documentation remains inconsistent.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Saturating subtraction can conceal an invalid outstanding-prepayment state from the new invariant metric.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
| let derived = canister_metrics.consumed_cycles() - outstanding; | ||
| match derived.cmp(&canister_metrics.consumed_cycles_as_counter()) { |
…gauge `replicated_state_consumed_cycles_since_replica_started` (and its `replicated_state_consumed_cycles` alias) exported `SubnetMetrics::consumed_cycles_total_including_canisters()`, i.e. the certified total, which behaves like a gauge: a prepayment raises it and the matching refund lowers it again. That is awkward to build monitoring on. Export the monotonic counterpart instead: the subnet-level aggregate plus the canisters' `consumed_cycles_monotonic`. It differs from the certified total by exactly the prepayments whose refund is not known yet, so it tracks it closely -- which is why it goes out under the existing name rather than a new one, and existing rules and dashboards keep working. The certified total itself is unchanged; only what the gauge reads changes. The subnet-level aggregate needs no monotonic twin of its own: subnet-level use cases are only ever charged, never refunded, and a deleted canister's consumption is moved into `consumed_cycles_by_deleted_canisters`. Deliberately still a Prometheus `Gauge` rather than a `Counter`. The value is recomputed from the replicated state on every observation, so a replica resuming from a checkpoint re-exports a lower value than it had reached before, and an online subnet split hands the migrating canisters' consumption over to the other subnet. Both need the same high water mark treatment on the consumer side that the non-monotonic value already needed; a `Counter` would instead report the whole checkpoint value as a jump on every restart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f9e2643 to
b56f1c7
Compare
mraszyk
changed the title
consumed_cycles_as_counter to CanisterMetrics
What this changes
replicated_state_consumed_cycles_since_replica_started(and itsreplicated_state_consumed_cyclesalias) exportedSubnetMetrics::consumed_cycles_total_including_canisters(), i.e. the certifiedtotal, which behaves like a gauge: a prepayment raises it and the matching refund
lowers it again. That makes it awkward to build monitoring on top of.
It now exports the monotonic counterpart instead: the subnet-level aggregate plus
the canisters'
consumed_cycles_monotonic. The subnet-level aggregate needs nomonotonic twin of its own -- subnet-level use cases are only ever charged, never
refunded, and a deleted canister's consumption moves into
consumed_cycles_by_deleted_canisters.No new metric is added, and the certified total is untouched:
CanisterMetrics::consumed_cyclesandSubnetMetrics::consumed_cycles_total_including_canistersare unchanged, so/subnet/<subnet_id>/metricsis unaffected. The two differ by exactly theprepayments whose refund is not known yet, i.e. the gauge tracks the certified
total closely and is simply better behaved.
Why reuse the name, and why it stays a gauge
It stays a gauge, not a Prometheus
Counter. The value is recomputed from thestate on every observation, so a replica that restarts mid-interval and resumes
from the last checkpoint re-exports a lower value than it had reached before; a
Counter'sreset()+inc_by()would report that as an increase by the wholecheckpoint value. An online subnet split lowers it for the same reason -- the
migrating canisters' consumption goes to the other subnet, and retaining it here
would count it on both.
So it still needs the same high water mark treatment on the consumer side that the
non-monotonic value already needed; it is just a much better behaved gauge in the
average case. And since it must be a gauge anyway, exporting the monotonic value
under the old name saves rewriting every rule and dashboard (including the public
dashboard) that already scrapes it.
Tests
consumed_cycles_total_is_exported_net_of_outstanding_prepayments: the exportedgauge equals the certified total net of the outstanding prepayments.
consumed_cycles_gauge_accounts_for_all_subnet_level_use_casesupdated: it nowcovers the subnet-level contributions only, since the state it builds holds no
canisters.
Notes for the reviewer
would work for
consumed_cycles_by_use_cases_monotonic-- splitoutstandinginto its
InstructionsandRequestAndResponseTransmissionparts and it fallsout. Left for later.
replicated_state,state_layout,state_manager,execution_environment,canonical_state,cycles_account_manager,messaging,replay,state_machine_tests,protobuf,types) plusrust-lint.sh. The NNS/SNS/registry integration tests,which reach this change only through the regenerated protobuf bindings, exhaust
the disk on my machine, and the two golden-state tests need internal SSH access;
those are left to CI.
🤖 Generated with Claude Code