feat: Add counter metrics for consumed cycles#9922
feat: Add counter metrics for consumed cycles#9922dsarlis wants to merge 1 commit intodfinity:masterfrom
Conversation
dsarlis
left a comment
dsarlis
left a comment
There was a problem hiding this comment.
Open points:
- Naming is something that I deliberately did not spend too much on. I can totally understand if a suffix
_as_countersis not considered good enough and open to ideas for alternate names. - In tests you'll notice that I've added a few places where I check the new counters additionally to old ones. One might wonder "what if we try to add such checks universally in
ExecutionTest". I tried it and as it turns out it's not easy to make all tests work as some of them cut corners and you're not always able to do the check at "clear points", i.e. points where you know that there can be no outstanding callbacks (execute_allwould be the best candidate and you get majority of tests to work but you still miss some special ones, highly concentrated inhypervisor_tests). I'd leave as an improvement if anyone wants to pick it up.
|
|
||
| // Canister A calls canister B. | ||
| let cycles_sent = Cycles::new(1_000_000); | ||
| let b_callback = wasm() |
There was a problem hiding this comment.
Drive-by improvement: using actual callbacks and canister requests (instead of injecting a response) to make the test more robust.
| // Skip if the consumed cycles are zero and no metric updates are needed. | ||
| if prepayment - refund == NominalCycles::zero() { |
There was a problem hiding this comment.
The previous one was not good enough after the changes here as it'd miss cases where you really used all of the prepayment (i.e. refund would be 0). Instead simplify the condition to be "consumption happened" whether it was during prepayment or refund phase.
| match use_case { | ||
| CyclesUseCase::Instructions | CyclesUseCase::RequestAndResponseTransmission => { | ||
| // These use cases are accounted for during refund | ||
| // for the counter metrics. | ||
| } | ||
| CyclesUseCase::Memory | ||
| | CyclesUseCase::ComputeAllocation | ||
| | CyclesUseCase::Uninstall | ||
| | CyclesUseCase::IngressInduction | ||
| | CyclesUseCase::CanisterCreation | ||
| | CyclesUseCase::BurnedCycles => { | ||
| *use_case_consumption_as_counter += prepayment; | ||
| } | ||
|
|
||
| CyclesUseCase::ECDSAOutcalls | ||
| | CyclesUseCase::SchnorrOutcalls | ||
| | CyclesUseCase::VetKd | ||
| | CyclesUseCase::HTTPOutcalls | ||
| | CyclesUseCase::DeletedCanisters | ||
| | CyclesUseCase::DroppedMessages | ||
| | CyclesUseCase::NonConsumed => { | ||
| // These use cases should not be tracked on the canister level. | ||
| } | ||
| } |
There was a problem hiding this comment.
I know it's not ideal that we need to "redo" a match here when we've had debug_asserts earlier in this function. I didn't have any quick ideas for improvements, besides trying to introduce even more type hierarchy and sort of differentiate types more from each other (e.g. can they be canister level or not). It's unclear to me how much effort that would be so opted for a match instead.
basvandijk
added
the
security-review-passed
2 participants
Add counter versions of metrics for consumed cycles that are stored in
ReplicatedState. The existing ones behave like gauges (so their values can go down when prepayments are made and up when refunds are issued) which makes it more challenging for consumers to build automated monitoring tools to perform aggregations over them. By having them monotonically increase, it's easier to calculate rates of change, show aggregates over time etc.The key idea is to introduce a second map of
<CyclesUseCase, NominalCycles>in theReplicatedStatethat will only be updated once per use case: either at the payment stage if we know the precise amount or only at refund stage if a prepayment is made with an expected refund later. The second map is quite similar to the existing in all other aspects (how they are stored in checkpoints or how they are exposed as prometheus metrics) besides how the values are updated.A new map is introduced to ease the transition as migrating from the old map to new is non-trivial given that a proper cutoff point needs to be introduced to handle outstanding callbacks that might have been created before the metric introduction. This is left for a follow-up if and when people decide to do it. The new map will be used in a follow up that will implement the new management canister endpoint to retrieve canister level metrics.
Additionally, the new metrics include the use case
HttpsOutcallsin the canister level metrics as it's useful to determine how much each canister uses this feature. I've opted to not change existing metrics to do the same as it would make things less clean imo than the current approach -- a single specific API is used to perform this update in exactly one place where it's needed.The changes in the PR are mostly driven by the addition of the new map of metrics, updates in protobuf files to store the new metrics, the changes to support having the
HttpsOutcallsuse case additionally included as well as some changes in tests to support the new metrics.