feat: Add a monotonic consumed_cycles total to CanisterMetrics - #11465
Merged
Conversation
`CanisterMetrics::consumed_cycles` 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, which is why `consumed_cycles_by_use_cases` already has a monotonic `consumed_cycles_by_use_cases_as_counters` twin, only bumped once the refund of a prepayment is known. This adds the same twin for the scalar total, maintained by `observe_consumed_cycles_with_use_case` alongside the by-use-case counters and persisted in `CanisterStateBits`. It is exactly the scalar equivalent of `consumed_cycles_by_use_cases_as_counters` summed over the use cases that `consumed_cycles` covers, i.e. everything except HTTPS outcalls, which are only tracked at the subnet level. The field is absent in checkpoints written before it and decodes as zero, the same starting point the by-use-case counters had. Nothing reads the new counter in production yet; backfilling it from the gauge, so that it starts out with the full history, and exporting it are left to follow-ups. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The counter’s accounting, persistence, backward compatibility, and key refund behaviors are implemented consistently and adequately tested.
Pull request overview
Adds a persistent monotonic total for canister-consumed cycles, complementing the existing gauge and per-use-case counters.
Changes:
- Tracks finalized cycle consumption without decrementing refunds.
- Persists the counter through checkpoints using protobuf field 71.
- Tests accounting, serialization, and backward-compatible decoding.
File summaries
| File | Description |
|---|---|
rs/state_manager/src/tip.rs |
Serializes the counter into checkpoints. |
rs/state_manager/src/checkpoint.rs |
Restores the counter from checkpoints. |
rs/state_layout/src/state_layout/tests.rs |
Tests round-tripping and missing-field defaults. |
rs/state_layout/src/state_layout/proto.rs |
Converts the counter to and from protobuf. |
rs/state_layout/src/state_layout.rs |
Adds the persisted state field. |
rs/replicated_state/src/canister_state/tests.rs |
Tests refundable and final cycle accounting. |
rs/replicated_state/src/canister_state/system_state.rs |
Implements and exposes monotonic tracking. |
rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs |
Adds generated protobuf bindings. |
rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto |
Defines protobuf field 71. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅ No security or compliance issues detected. Reviewed everything up to 72e78ca. Security Overview
Detected Code Changes
|
alin-at-dfinity
approved these changes
Sep 7, 2026
alin-at-dfinity
left a comment
alin-at-dfinity
left a comment
Contributor
There was a problem hiding this comment.
Only a couple of nitpicks, the only meaningful one being an objection to the "as counter" qualifier.
…cles-as-counter-field # Conflicts: # rs/replicated_state/src/canister_state/tests.rs
Addresses review feedback: neither the `CanisterMetrics` / `CanisterStateBits` fields nor the Prometheus metrics they feed are actual counters, so `monotonic` is the more accurate qualifier. Renames the new `consumed_cycles_as_counter` along with the pre-existing canister- and subnet-level `consumed_cycles_by_use_case(s)_as_counters`. The exported Prometheus metric names are deliberately left untouched, so that existing dashboards and alerts keep working. Also, as requested: * trims the `consumed_cycles_monotonic` proto comment down to what is specific to the field; * places it next to `consumed_cycles` everywhere; and * calls a charge made without a prepayment a "direct charge", the term already used by `consume_cycles`, rather than a "final prepayment". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mraszyk
changed the title
consumed_cycles_as_counter to CanisterMetricsconsumed_cycles total to CanisterMetrics
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The Prometheus monotonic aggregate incorrectly incorporates the subnet gauge map, allowing migration backfills to create spurious counter jumps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced
`refunds_prepayment_of_aborted_canister_install_dropped_after_split` only showed that the monotonic total stays at zero across a full refund, which is strictly weaker than `full_refund_does_not_lower_consumed_cycles_monotonic`: the latter seeds a nonzero total first, so it actually pins down that the refund does not decrement it. Leaves the test to its own subject, the drop path. `test_decode_missing_consumed_cycles_monotonic` was a near-duplicate of `test_encode_decode_consumed_cycles_monotonic`, down to the fixture; folded the missing-field case into the latter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The monotonic Prometheus aggregate incorrectly reads the subnet gauge map instead of the monotonic map.
Review details
Suppressed comments (1)
rs/replicated_state/src/metrics.rs:566
- This aggregate is named and exported as monotonic, but its subnet contribution still comes from
get_consumed_cycles_by_use_case(), the gauge map. In particular,migrate_outcalls_cycles_to_use_cases()backfills only that gauge map, so a migration can create the exact spurious jump that the separate monotonic map is intended to avoid; the persisted subnet monotonic values are otherwise never used by this production exporter. Readget_consumed_cycles_by_use_case_monotonic()here instead.
&mut consumed_cycles_total_by_use_case_monotonic,
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
mraszyk
added a commit
that referenced
this pull request
Sep 8, 2026
…cles-as-counter The base of this branch landed upstream as #11465 with `monotonic` naming in place of `as_counter`, so the conflicts are almost entirely that rename plus the review polish it picked up on the way in. Resolved in favour of master throughout, and renamed the identifiers this branch adds on top to match: * `CanisterMetrics::consumed_cycles_as_counter` and the `CanisterStateBits` / proto field 71 behind it: dropped this branch's copies, which master now carries as `consumed_cycles_monotonic`. * `SystemState::migrate_consumed_cycles_to_counter` and the scheduler's `migrate_consumed_cycles_to_counters` -> `..._to_monotonic`; the `reset_consumed_cycles_as_counter` test helper likewise. * `ReplicatedStateMetrics::observe`: this branch's `consumed_cycles_by_canisters_as_counter` accumulator -> `consumed_cycles_by_canisters_monotonic`, alongside master's `consumed_cycles_total_by_use_case_monotonic`. * The prose that named the field "the counter" now says "monotonic", and the three `canister_state/tests.rs` unit tests this branch and master both added are kept once, in master's form. The exported Prometheus metric names keep their `as_counters` spelling, as master deliberately left them untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 8, 2026
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.
CanisterMetrics::consumed_cyclesbehaves like a gauge: a prepayment raises it and the matching refund lowers it again. That makes it awkward to build monitoring on top of, which is whyconsumed_cycles_by_use_casesalready has a monotonic twin, only bumped once the refund of a prepayment is known.This adds the same twin for the scalar total:
CanisterMetrics::consumed_cycles_monotonic, maintained byobserve_consumed_cycles_with_use_casealongside the by-use-case amounts, and persisted inCanisterStateBits(proto field 71).It is exactly the scalar equivalent of
consumed_cycles_by_use_cases_monotonicsummed over the use cases thatconsumed_cyclescovers, i.e. everything except HTTPS outcalls, which are only tracked at the subnet level.The field is absent in checkpoints written before it, and decodes as zero — the same starting point the by-use-case map had when it was introduced. Nothing consumes the new field yet: outside of tests, the only reader is
tip.rs, which persists it. No metric or behaviour changes.A follow-up commit renames the pre-existing
as_counter(s)fields tomonotonic, at both the canister and the subnet level: neither theCanisterMetrics/CanisterStateBitsfields nor the Prometheus metrics they feed are actual counters. It is a pure rename of Rust and protobuf identifiers; the exported Prometheus metric names are deliberately left untouched, so existing dashboards and alerts keep working.Note on a pre-existing inconsistency that the rename makes visible, and that Copilot flags: in
metrics.rs, the subnet-level contribution to the monotonic Prometheus aggregate is joined fromget_consumed_cycles_by_use_case(), i.e. the gauge map, notget_consumed_cycles_by_use_case_monotonic(). Sincemigrate_outcalls_cycles_to_use_cases()deliberately backfills only the gauge map, that join can export the very jump the separate monotonic map exists to avoid. This is unchanged frommaster(only the accumulator's name differs here), and correcting it would lower the exported HTTP/ECDSA outcall values by the backfilled amount, which reads as a counter reset — so it is left out of this PR.Follow-ups, kept separate:
consumed_cycles_monotonicfrom theconsumed_cyclesgauge, so it starts out with the full history rather than from zero;replicated_state_consumed_cycles_since_replica_startedgauge.🤖 Generated with Claude Code