feat(ledger): DEFI-1565: Expose archive and dedup settings as metrics - #11418
Open
mbjorkqvist wants to merge 8 commits into
Open
feat(ledger): DEFI-1565: Expose archive and dedup settings as metrics#11418mbjorkqvist wants to merge 8 commits into
mbjorkqvist wants to merge 8 commits into
Conversation
The archiving configuration was not observable from outside either ledger: `archives()` and `ledger_num_archives` show which archives exist, but nothing showed the settings that govern when archiving happens. An alert that needs to compare against one of them has to hard-code it, and then drifts whenever `ChangeArchiveOptions` is used. Exposes the six numerical archive settings, plus the transaction deduplication settings, which were equally invisible. All are the effective values in force: `Archive::new` already resolves the optional `ArchiveOptions` fields, and `max_transactions_per_response` now resolves through a default shared with the archive canister so the value the ledger reports cannot drift from the one the archive enforces. Principals are deliberately left out; `controller_id` and `more_controller_ids` would need label-valued series and are out of scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several metrics report values that differ from the limits actually enforced by the ledger or archive canister.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds archive and transaction-deduplication configuration metrics to ICP and ICRC ledgers.
Changes:
- Exposes archive and deduplication settings as gauges.
- Shares the archive transaction-response default.
- Adds initialization and upgrade metric tests.
File summaries
| File | Description |
|---|---|
rs/ledger_suite/tests/sm-tests/src/lib.rs |
Adds shared metric tests. |
rs/ledger_suite/icrc1/ledger/tests/tests.rs |
Runs shared ICRC tests. |
rs/ledger_suite/icrc1/ledger/src/main.rs |
Exports ICRC configuration metrics. |
rs/ledger_suite/icrc1/archive/src/main.rs |
Uses the shared response default. |
rs/ledger_suite/icp/ledger/tests/tests.rs |
Tests ICP configuration metrics. |
rs/ledger_suite/icp/ledger/src/main.rs |
Exports ICP configuration metrics. |
rs/ledger_suite/common/ledger_canister_core/src/archive.rs |
Defines shared default and effective-value accessor. |
Review details
Suppressed comments (3)
rs/ledger_suite/icp/ledger/src/main.rs:1185
Duration::as_secs()truncates the configurable ICP transaction window to whole seconds. The init API accepts and stores an arbitraryDuration, so a 1.5-second window is reported as1even though the metric format supports fractional seconds. Preserve the configured value withas_secs_f64().
ledger.transaction_window().as_secs() as f64,
rs/ledger_suite/icrc1/ledger/src/main.rs:343
- The default archive option is 2 MiB, but this ledger always passes
MAX_MESSAGE_SIZE(1 MiB) toarchive_blocks, andsend_blocks_to_archivetakes the minimum. Consequently the new default test expects 2 MiB while the effective maximum described here is 1 MiB. Exportarchive.max_message_size_bytes.min(MAX_MESSAGE_SIZE)and update the test expectation accordingly.
archive.max_message_size_bytes as f64,
rs/ledger_suite/icp/ledger/src/main.rs:1172
- This metric does not describe the ICP archive's behavior. The ICP archive init decodes only
(CanisterId, u64, Option<u64>), and its block-response limits are the fixedicp_ledger::max_blocks_per_requestvalues (icp/archive/src/main.rs:272-279, 304-335);max_transactions_per_responseis never passed to or enforced by it. Thus the test's configured value99is exported even though the archive still returns up to 2000 (or 50 for replicated ingress). Please omit this metric for ICP, or add end-to-end support for the setting in the ICP archive before exposing it.
"ledger_archive_max_transactions_per_response",
archive.effective_max_transactions_per_response() as f64,
"Maximum number of transactions an archive returns per response.",
- Files reviewed: 7/7 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.
Three of the exposed settings only bear on archives the ledger spawns from now on: an archive that already exists was installed with whatever was configured at the time and keeps using it. The metric help text now says so and points at the archive's own metric for the value actually being enforced. Drops `ledger_archive_max_transactions_per_response` from the ICP ledger. The ICP archive's `init` takes only the ledger id, the block height offset and the memory cap, so the fourth argument the shared spawn code encodes is silently ignored and the setting has no effect there. The ICP archive already reported its own cap; the ICRC archive reported neither of its settings, so it now exposes both. That closes the gap: the ledger says what a new archive will get, each archive says what it enforces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two metrics claimed limits neither canister applies, both found in review. The message size limit is the archive option, but the size used when archiving is the smaller of it and the ledger's own limit. On an ICRC ledger that is the default case, not an edge case: the option defaults to 2 MiB while the ledger passes 1 MiB, so the metric was twice the truth everywhere. The help text now says which of the two it is, and the ICRC ledger exposes its own limit as well, so the smaller of the two can be seen. The ICP ledger already exposed its own. An ICRC archive both defaults to and clamps its memory cap at 3 GiB, so a larger value configured on the ledger has no effect. The ledger now reports the clamped value, through a limit shared with the archive so the two cannot disagree. The ICP archive has no such bound and is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`with_ledger` already hands out a reference, so `&*ledger` there is a reborrow rather than a deref. Caught by `clippy::borrow_deref_ref` in the Cargo Lint job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither limit applies to the ICP ledger, so `ledger_canister_core` was the wrong home for them: the ICP archive has no `max_transactions_per_response` setting at all, and does not bound the memory cap it is given. Putting them there also left an ICRC-only accessor hanging off the shared `Archive` type. Moves both to `ic_icrc1::archive_limits`, which the ICRC ledger and the ICRC archive already depend on, and resolves the default and the clamp at the ledger's metric site instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ledgers had their own copy of the archive and deduplication metric encoders. `ic-metrics-encoder` is already a workspace dependency and already in the Bazel crate index, so `ledger_canister_core` can take it without repinning and hold the shared versions. The deduplication encoder was identical and moved as it was. The archive encoder was not: an ICRC archive clamps the memory cap it is given and has a response limit that the ICP archive does not, so the effective cap is a parameter and the ICRC-only gauge stays at its call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The metrics reflect effective settings, preserve ledger-specific behavior, and are covered by focused integration tests.
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…xpose-archive-settings-as-metrics # Conflicts: # rs/ledger_suite/icrc1/ledger/tests/tests.rs # rs/ledger_suite/tests/sm-tests/src/lib.rs
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.
The archiving configuration of both ledgers is not observable from outside the canister.
archives()andledger_num_archivesshow which archive canisters exist, but nothing shows the settings that decide when archiving happens. Anything that needs to compare against one of them — an alert, a dashboard, a review — has to hard-code the value, and then silently drifts the first timeChangeArchiveOptionsis used.This PR proposes to expose those settings as metrics: six on the ICRC ledger, and five on the ICP ledger, whose archive has no maximum transactions per response — the shared spawn code encodes that argument but the ICP archive ignores it. The optional ones are reported with their defaults already applied, so a consumer never has to know what the default is or whether it was set.
The memory cap, the creation cycles and the response limit bear only on archives the ledger spawns from now on; an archive that already exists keeps what it was installed with. The metric help text says so, and points at the archive's own metric for the value being enforced. The ICP archive already reported its memory cap; the ICRC archive reported neither of its settings and now reports both. Being able to tell what a future archive will get is the reason the ledger reports these at all: a ledger with no archives yet has nothing else to ask.
Two of them needed care, because the archive option is not the limit that ends up applying. An ICRC archive clamps its memory cap at 3 GiB, so a larger configured value has no effect and the metric reports it clamped, through a limit shared with the archive so the two cannot disagree. The size of a message sent to an archive is the smaller of the archive option and the ledger's own limit — on an ICRC ledger with default settings that is the ledger's, not the archive's — so rather than report a minimum, that metric is labelled as the option it is, and the ICRC ledger now reports its own limit alongside it, which the ICP ledger already did.
The transaction deduplication settings had the same visibility problem and are included: the window length, the number of transactions retained in it, and the number purged per operation. The window is a constant on the ICRC ledger and configurable on the ICP ledger; both report the value actually in force. The controller principals are deliberately left out.
Tests cover both ledgers and the archive side, asserting values rather than presence: that unset options resolve to their defaults, that changing options is reflected, that options not named in a change are left alone, that a memory cap above what an archive will accept is reported clamped, and that changing a ledger setting does not change what an existing archive enforces.