feat: more metrics by kpop-dfinity · Pull Request #9906 · dfinity/ic

kpop-dfinity · 2026-04-16T14:24:16Z

No description provided.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

Copilot

Pull request overview

This PR extends canister metrics to include canister-to-canister connectivity (message counts by sender), persists those metrics in checkpoints, and updates state-tool / subnet-splitting tooling to export and consume the new connectivity CSV.

Changes:

Add per-canister connection metrics (LRU-capped map keyed by sender canister) and increment them during local-subnet request execution.
Persist connection metrics in checkpoint protobufs and restore them on checkpoint load.
Update state-tool canister_metrics to emit two CSVs (load metrics + connectivity metrics) and adjust subnet-splitting integration + data loading.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
rs/state_tool/src/main.rs	Extends CLI to accept two output paths for canister metrics export.
rs/state_tool/src/commands/canister_metrics.rs	Writes separate CSVs for load metrics and connectivity metrics.
rs/state_manager/src/tip.rs	Serializes connection metrics into `CanisterStateBits` when checkpointing.
rs/state_manager/src/checkpoint.rs	Restores connection metrics when loading canister state from checkpoints.
rs/state_layout/src/state_layout.rs	Adds `connection_metrics` field to `CanisterStateBits`.
rs/state_layout/src/state_layout/proto.rs	Adds protobuf (de)serialization for `connection_metrics`.
rs/state_layout/src/state_layout/tests.rs	Updates test fixture to include the new `connection_metrics` field.
rs/replicated_state/src/canister_state/system_state.rs	Introduces `ConnectionMetrics` + `LRUConnectionMetrics` and plumbs into `CanisterMetrics`.
rs/execution_environment/src/execution_environment.rs	Increments connectivity metrics when executing local-subnet requests.
rs/recovery/subnet_splitting/tests/integration_tests.rs	Updates e2e test to use real connectivity metrics output instead of fake CSV.
rs/recovery/subnet_splitting/split_finder/data_io.py	Adjusts communication/load data ingestion (currently with commented-out filtering).
rs/protobuf/def/state/canister_state_bits/v1/canister_state_bits.proto	Adds `connection_metrics` field and `CanisterToCanisterMetrics` message.
rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs	Regenerated protobuf bindings for the new message/field.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T14:32:15Z

            instructions_executed,
            load_metrics,
+            connection_metrics: LRUConnectionMetrics {
+                metrics_per_canister: connection_metrics,
+            },


MAX_CAPACITY is only enforced on increment(). When metrics are constructed from checkpoint data (CanisterMetrics::new takes a full BTreeMap), a malformed/large checkpoint could load an arbitrarily large map and bypass the intended cap. Consider trimming/evicting down to MAX_CAPACITY during construction (and/or in deserialization) to make the bound invariant hold.

Copilot · 2026-04-16T14:32:16Z

        local_subnet_messages_executed: 0,
        http_outcalls_executed: 0,
        heartbeats_and_global_timers_executed: 0,
+        connection_metrics: BTreeMap::new(),
    }


A new connection_metrics field was added to CanisterStateBits, but the encode/decode tests in this module don’t cover non-empty connection_metrics. Adding a roundtrip test with at least one entry would help prevent regressions in the new protobuf mapping.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T12:01:55Z

+        /// Canister connections output path.
+        #[clap(long)]
+        canister_connections_output: PathBuf,


The CLI flag/field name uses "connections" (canister_connections_output) while the implementation and test artifacts use "connectivity". Consider standardizing on one term across the CLI, variable names, and canister_metrics::get parameter naming to reduce user confusion and make it easier to discover the right output file.

Suggested change

/// Canister connections output path.

#[clap(long)]

canister_connections_output: PathBuf,

/// Canister connectivity output path.

#[clap(long = "canister-connectivity-output", alias = "canister-connections-output")]

canister_connectivity_output: PathBuf,

Copilot · 2026-04-17T12:01:56Z

        local_subnet_messages_executed: 0,
        http_outcalls_executed: 0,
        heartbeats_and_global_timers_executed: 0,
+        connection_metrics: BTreeMap::new(),
    }


A new connection_metrics field was added to CanisterStateBits and its proto (de)serialization, but there is no encode/decode test coverage for it in this test module (only the default is updated). Please add a round-trip test that populates connection_metrics with at least one entry and asserts it survives pb encode/decode, to guard against future proto/schema regressions.

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T12:16:05Z

+        /// Canister load output path.
        #[clap(long)]
-        output: PathBuf,
+        canister_load_output: PathBuf,
+
+        /// Canister connections output path.
+        #[clap(long)]
+        canister_connections_output: PathBuf,


The canister_metrics subcommand description still says metrics are printed “to the specified file”, but the command now requires two output paths (load + connectivity). Please update the subcommand-level doc/help text to clearly describe both outputs and their corresponding flags so users don’t assume a single CSV is produced.

Copilot · 2026-04-17T12:16:06Z

+def load_subnet_data(
+    load_path: Path,
+    load_baseline_path: Path,
+    load_type: str,
+    communication_data_path: Path,
+    communication_baseline_data_path: Path,
+) -> Dict:


load_subnet_data gained a new communication_baseline_data_path parameter, but the function docstring still only documents communication_data_path. Please update the docstring to describe the baseline communication CSV and how it’s used (e.g., subtraction to compute deltas).

Copilot · 2026-04-17T12:16:07Z

+    parser.add_argument(
+        "--communication-baseline-data-path",
+        type=Path,
+        help="Path to canister-to-canister communication data",


The help text for --communication-baseline-data-path is identical to the non-baseline flag (“Path to canister-to-canister communication data”), which makes it hard to understand what the baseline file represents. Please update the help string to explicitly say it’s the baseline/earlier snapshot used to compute relative (delta) communication counts.

Suggested change

help="Path to canister-to-canister communication data",

help="Path to baseline canister-to-canister communication data. "

"It should represent the earlier snapshot taken before the data under "

"`communication-data-path` so that relative (delta) communication counts can be computed",

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T12:33:31Z

+            connection_metrics: item
+                .connection_metrics
+                .into_iter()
+                .map(
+                    |(


New connection_metrics encoding/decoding logic is added here, but there’s no test exercising a non-empty map roundtrip (including timestamp + count). Since this module already has encode/decode tests, please add a small test case that serializes a CanisterStateBits with at least one connection_metrics entry and verifies it decodes back correctly.

Copilot · 2026-04-17T12:33:32Z

        #[clap(long = "checkpoint")]
        path: PathBuf,

-        /// Output path.
+        /// Canister load output path.
        #[clap(long)]


The canister_metrics subcommand’s top-level doc comment still says it prints metrics to a single “specified file”, but this variant now requires two output paths (load + connections). Please update that description so --help accurately reflects that two CSV files are produced.

kpop-dfinity and others added 30 commits April 10, 2026 08:34

feat: add split finder script

cf1605d

linter

74ca2b1

Automatically fixing code for linting and formatting issues

a96ff53

lint

8be9d1b

fix

ab5728c

more validations

eedc856

Update rs/recovery/subnet_splitting/split_finder/split_finder.py

91a116c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

rm deps

b39bc1b

Run bazel run //:python-requirements.update

436c9e6

fix deps

bc9dc59

Pierugo's comments

fc38c29

more comments

9bac75e

rm

8570c39

oups

7f1c4a8

AI comments

1d1d469

Update rs/recovery/subnet_splitting/tests/integration_tests.rs

c37887e

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

rm

1fac25d

unit test

1241dfd

rename

0a49928

Automatically fixing code for linting and formatting issues

89600de

.

34c1c30

ass ert

a138d0c

comment

4b15d7c

Update rs/recovery/subnet_splitting/split_finder/split_finder.py

fb56776

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rs/recovery/subnet_splitting/split_finder/split_finder.py

fafcebb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rs/recovery/subnet_splitting/split_finder/split_finder.py

87ee8b6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Automatically fixing code for linting and formatting issues

768d1d4

prog

b7ea74a

generalize

fcb71b0

clippy

f262f30

kpop-dfinity and others added 11 commits April 15, 2026 12:44

.

cc9af82

unused

e9f4b90

Update rs/recovery/subnet_splitting/split_finder/solver_core.py

5330b79

Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>

fix PIERUGO's typo

e7e9c67

.

82897c2

baseline

d9ad55b

Pierugo's comments

9a1cd2b

.

b45adcb

.

8266439

.

c025c1e

Merge branch 'master' into kpop/ss_tool_follow_up

640628c

github-actions Bot added the feat label Apr 16, 2026

kpop-dfinity requested a review from Copilot April 16, 2026 14:24

Copilot started reviewing on behalf of kpop-dfinity April 16, 2026 14:25 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

fixes

2d60ce1

kpop-dfinity requested a review from Copilot April 17, 2026 11:56

Copilot started reviewing on behalf of kpop-dfinity April 17, 2026 11:56 View session

Automatically fixing code for linting and formatting issues

710cc5d

Copilot AI reviewed Apr 17, 2026

View reviewed changes

.

99cf0db

kpop-dfinity requested a review from Copilot April 17, 2026 12:10

Copilot started reviewing on behalf of kpop-dfinity April 17, 2026 12:11 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

kpop-dfinity added 2 commits April 17, 2026 12:25

.

e8f8dfb

.

69ee0a1

kpop-dfinity requested a review from Copilot April 17, 2026 12:27

Copilot started reviewing on behalf of kpop-dfinity April 17, 2026 12:27 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

-        help="Path to canister-to-canister communication data",
+        help="Path to baseline canister-to-canister communication data. "
+        "It should represent the earlier snapshot taken before the data under "
+        "`communication-data-path` so that relative (delta) communication counts can be computed",

Conversation

kpop-dfinity commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants