Skip to content

Commit fc79106

Browse files
mraszykclaude
andauthored
feat(migration-canister): export a metric for the oldest request in flight and add endpoint for listing requests (#10978)
Adds the gauge `migration_canister_oldest_request_in_flight_age_seconds` to the metrics served via `http_request`, reporting the age of the migration request that has been in flight the longest. The gauge is not set if there is no request in flight. The age is derived from a new stable map `ACCEPTED_TIME` recording, for every request in `REQUESTS`, the time at which it was accepted. The timestamp is retained across state transitions (which remove and re-insert the request) and dropped once the request leaves `REQUESTS` for good, i.e., when the corresponding event is recorded in `HISTORY` (both on success and on failure). Requests that were already in flight before this bookkeeping was introduced are backfilled upon their next state transition. The motivation for adding the gauge is ability to introduce alerts for request that are stuck for a long time. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0bbf497 commit fc79106

3 files changed

Lines changed: 207 additions & 4 deletions

File tree

rs/migration_canister/src/canister_state.rs

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ thread_local! {
3030
static REQUESTS: RefCell<BTreeMap<RequestState, (), Memory>> =
3131
RefCell::new(BTreeMap::init(MEMORY_MANAGER.with(|m| m.borrow().get(MemoryId::new(1)))));
3232

33+
/// Records for every request in `REQUESTS` the time (IC time in nanos since epoch)
34+
/// at which it entered `REQUESTS`, i.e., when it was accepted.
35+
/// An entry is added when a request is first inserted into `REQUESTS` and is
36+
/// retained across state transitions (which remove and re-insert the request).
37+
/// It is removed once the request leaves `REQUESTS` for good, i.e., when the
38+
/// corresponding event is recorded in `HISTORY`.
39+
static ACCEPTED_TIME: RefCell<BTreeMap<CanisterMigrationArgs, u64, Memory>> =
40+
RefCell::new(BTreeMap::init(MEMORY_MANAGER.with(|m| m.borrow().get(MemoryId::new(5)))));
41+
3342
/// Stores timestamps of all successful events in `HISTORY`
3443
/// that are within the last 24 hours.
3544
/// It can also store timestamps beyond the last 24 hours
@@ -53,6 +62,20 @@ thread_local! {
5362
// BTreeMap<(Request, Reqstate: String), (Counter: u64, FirstTs: Time, LastTs: Time)>
5463
}
5564

65+
/// Returns the current IC time in nanos since epoch.
66+
///
67+
/// Unit tests do not run inside a canister, where `ic_cdk::api::time` traps,
68+
/// so we fall back to zero there.
69+
#[cfg(not(test))]
70+
fn now() -> u64 {
71+
ic_cdk::api::time()
72+
}
73+
74+
#[cfg(test)]
75+
fn now() -> u64 {
76+
0
77+
}
78+
5679
pub fn migrations_disabled() -> bool {
5780
DISABLED.with_borrow(|x| *x.get())
5881
}
@@ -86,16 +109,36 @@ pub mod privileged {
86109
pub mod requests {
87110
use candid::Principal;
88111

89-
use crate::{RequestState, canister_state::REQUESTS};
112+
use crate::{
113+
CanisterMigrationArgs, RequestState,
114+
canister_state::{ACCEPTED_TIME, REQUESTS, now},
115+
};
90116

91117
pub fn num_requests() -> u64 {
92118
REQUESTS.with_borrow(|req| req.len())
93119
}
94120

95121
pub fn insert_request(request: RequestState) {
122+
let args = CanisterMigrationArgs::from(request.request());
123+
// State transitions remove and re-insert a request, so we must only
124+
// record the time at which the request was accepted, i.e., first inserted.
125+
// Requests that were already in flight before this bookkeeping was
126+
// introduced are backfilled upon their next state transition.
127+
ACCEPTED_TIME.with_borrow_mut(|a| {
128+
if !a.contains_key(&args) {
129+
a.insert(args, now());
130+
}
131+
});
96132
REQUESTS.with_borrow_mut(|r| r.insert(request, ()));
97133
}
98134

135+
/// Returns the age in nanos of the request that has been in flight the longest,
136+
/// or `None` if there is no request in flight.
137+
pub fn oldest_request_age_nanos() -> Option<u64> {
138+
let now = now();
139+
ACCEPTED_TIME.with_borrow(|a| a.values().map(|time| now.saturating_sub(time)).max())
140+
}
141+
99142
pub fn remove_request(request: &RequestState) {
100143
let _ = REQUESTS.with_borrow_mut(|r| r.remove(request));
101144
}
@@ -134,12 +177,15 @@ pub mod requests {
134177
pub mod events {
135178
use crate::{
136179
CanisterMigrationArgs, Event, EventType,
137-
canister_state::{HISTORY, LAST_EVENT, LIMITER},
180+
canister_state::{ACCEPTED_TIME, HISTORY, LAST_EVENT, LIMITER},
138181
};
139182
use candid::Principal;
140183
use ic_cdk::api::time;
141184

142185
pub fn insert_event(event: EventType) {
186+
// Recording an event is the terminal step of a request, i.e., the request
187+
// has just left `REQUESTS` and thus is not in flight anymore.
188+
ACCEPTED_TIME.with_borrow_mut(|a| a.remove(&CanisterMigrationArgs::from(event.request())));
143189
let time = time();
144190
if let EventType::Succeeded { .. } = event {
145191
LIMITER.with_borrow_mut(|l| {

rs/migration_canister/src/migration_canister.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use crate::{
1616
events::{find_last_event, history_len},
1717
limiter::num_successes_in_past_24_h,
1818
migrations_disabled, num_validations,
19-
requests::{find_request, insert_request, num_requests},
19+
requests::{find_request, insert_request, num_requests, oldest_request_age_nanos},
2020
set_allowlist,
2121
},
2222
rate_limited, start_timers,
@@ -133,6 +133,15 @@ fn encode_metrics(w: &mut ic_metrics_encoder::MetricsEncoder<Vec<u8>>) -> std::i
133133
"Number of currently ongoing migration requests.",
134134
)?;
135135

136+
// This gauge is not set if there is no request in flight.
137+
if let Some(age_nanos) = oldest_request_age_nanos() {
138+
w.encode_gauge(
139+
"migration_canister_oldest_request_in_flight_age_seconds",
140+
age_nanos as f64 / 1_000_000_000_f64,
141+
"Age in seconds of the migration request that has been in flight the longest.",
142+
)?;
143+
}
144+
136145
w.encode_gauge(
137146
"migration_canister_num_successes_in_past_24_h",
138147
num_successes_in_past_24_h() as f64,

rs/migration_canister/tests/tests.rs

Lines changed: 149 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use ic_transport_types::EnvelopeContent::Call;
1212
use ic_universal_canister::{CallArgs, UNIVERSAL_CANISTER_WASM, wasm};
1313
use itertools::Itertools;
1414
use pocket_ic::{
15-
CreateCanisterParams, CreateCanisterPlacement, PocketIcBuilder,
15+
CreateCanisterParams, CreateCanisterPlacement, PocketIcBuilder, Time,
1616
common::rest::{
1717
CanisterCyclesCostSchedule, ExtendedSubnetConfigSet, IcpFeatures, IcpFeaturesConfig,
1818
SubnetSpec,
@@ -815,6 +815,154 @@ async fn metrics() {
815815
get_gauge(&metrics, "migration_canister_migrations_enabled"),
816816
1.0
817817
);
818+
819+
assert_eq!(
820+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
821+
0.0
822+
);
823+
824+
assert_eq!(oldest_request_age(&metrics), None);
825+
}
826+
827+
const OLDEST_REQUEST_AGE_METRIC: &str = "migration_canister_oldest_request_in_flight_age_seconds";
828+
829+
/// Reads the age (in seconds) of the oldest request in flight.
830+
/// Returns `None` if the metric is not set, i.e., if no request is in flight.
831+
fn oldest_request_age(metrics: &Scrape) -> Option<f64> {
832+
let is_metric_set = metrics
833+
.samples
834+
.iter()
835+
.any(|sample| sample.metric == OLDEST_REQUEST_AGE_METRIC);
836+
if !is_metric_set {
837+
return None;
838+
}
839+
Some(get_gauge(metrics, OLDEST_REQUEST_AGE_METRIC))
840+
}
841+
842+
#[tokio::test]
843+
async fn oldest_request_in_flight_metric() {
844+
let Setup {
845+
pic,
846+
migrated_canisters,
847+
replaced_canisters,
848+
migrated_canister_controllers,
849+
..
850+
} = setup(Settings {
851+
num_migrations: 2,
852+
..Default::default()
853+
})
854+
.await;
855+
let sender = migrated_canister_controllers[0];
856+
let first = MigrateCanisterArgs {
857+
migrated_canister_id: migrated_canisters[0],
858+
replaced_canister_id: replaced_canisters[0],
859+
};
860+
let second = MigrateCanisterArgs {
861+
migrated_canister_id: migrated_canisters[1],
862+
replaced_canister_id: replaced_canisters[1],
863+
};
864+
865+
// Bump the time to 2022-01-01T00:00:00Z so that the requests are not
866+
// accepted at the UNIX epoch and thus their age is not equal to the
867+
// current IC time (which would make the assertions below vacuous).
868+
pic.set_time(Time::from_nanos_since_unix_epoch(
869+
1_640_995_200 * 1_000_000_000,
870+
))
871+
.await;
872+
873+
// Without any request in flight, the metric is not set.
874+
assert_eq!(oldest_request_age(&fetch_metrics(&pic).await), None);
875+
876+
migrate_canister(&pic, sender, &first).await.unwrap();
877+
pic.advance_time(Duration::from_secs(10)).await;
878+
pic.tick().await;
879+
880+
// The age of the (only) request in flight grows with time.
881+
let metrics = fetch_metrics(&pic).await;
882+
assert_eq!(
883+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
884+
1.0
885+
);
886+
let age = oldest_request_age(&metrics).unwrap();
887+
assert!((10.0..100.0).contains(&age), "unexpected age {age}");
888+
889+
// A second, younger request does not affect the metric:
890+
// it keeps reporting the age of the first request.
891+
migrate_canister(&pic, sender, &second).await.unwrap();
892+
let metrics = fetch_metrics(&pic).await;
893+
assert_eq!(
894+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
895+
2.0
896+
);
897+
let age = oldest_request_age(&metrics).unwrap();
898+
assert!((10.0..100.0).contains(&age), "unexpected age {age}");
899+
900+
// Drive both migrations to completion. We advance time by a lot so that
901+
// the task waiting for 6 minutes can succeed quickly.
902+
for _ in 0..100 {
903+
pic.advance_time(Duration::from_secs(250)).await;
904+
pic.tick().await;
905+
}
906+
for args in [&first, &second] {
907+
assert!(matches!(
908+
get_status(&pic, sender, args).await.unwrap(),
909+
MigrationStatus::Succeeded { .. }
910+
));
911+
}
912+
913+
// Successful requests are not in flight anymore.
914+
let metrics = fetch_metrics(&pic).await;
915+
assert_eq!(
916+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
917+
0.0
918+
);
919+
assert_eq!(oldest_request_age(&metrics), None);
920+
}
921+
922+
#[tokio::test]
923+
async fn oldest_request_in_flight_metric_reset_on_failure() {
924+
let Setup {
925+
pic,
926+
migrated_canisters,
927+
replaced_canisters,
928+
migrated_canister_controllers,
929+
..
930+
} = setup(Settings::default()).await;
931+
let sender = migrated_canister_controllers[0];
932+
let migrated_canister = migrated_canisters[0];
933+
let args = MigrateCanisterArgs {
934+
migrated_canister_id: migrated_canister,
935+
replaced_canister_id: replaced_canisters[0],
936+
};
937+
938+
migrate_canister(&pic, sender, &args).await.unwrap();
939+
940+
// The request has been accepted and thus its age is reported by the metric.
941+
let metrics = fetch_metrics(&pic).await;
942+
assert_eq!(
943+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
944+
1.0
945+
);
946+
assert!(oldest_request_age(&metrics).is_some());
947+
948+
// Validation succeeded. Now we break migration by interfering.
949+
pic.start_canister(migrated_canister, Some(sender))
950+
.await
951+
.unwrap();
952+
for _ in 0..4 {
953+
advance(&pic).await;
954+
}
955+
let MigrationStatus::Failed { .. } = get_status(&pic, sender, &args).await.unwrap() else {
956+
panic!("expected the migration to fail")
957+
};
958+
959+
// Failed requests are not in flight anymore either.
960+
let metrics = fetch_metrics(&pic).await;
961+
assert_eq!(
962+
get_gauge(&metrics, "migration_canister_requests_in_flight"),
963+
0.0
964+
);
965+
assert_eq!(oldest_request_age(&metrics), None);
818966
}
819967

820968
async fn concurrent_migration(

0 commit comments

Comments
 (0)