FEAT: hardware metrics #876
Conversation
| // Update CPU usage metric | ||
| let cpu_usage = system.global_cpu_usage() as i64; | ||
| crate::metrics::CPU_USAGE_PERCENTAGE | ||
| .with_label_values(&["global"]) |
There was a problem hiding this comment.
better also add account_id to the label values, like .with_label_values(&["global", my_account_id.as_str()]).
You'd likely need to pass in the account_id in the update_system_metrics() to be able to access it. The account_id value is available here:
mpc/chain-signatures/node/src/protocol/mod.rs
Line 204 in d8d8e38
There was a problem hiding this comment.
Should be fixed now I think
chain-signatures/node/src/metrics.rs
Outdated
| }); | ||
|
|
||
| // Total Memory Metric | ||
| pub(crate) static TOTAL_MEMORY_BYTES: Lazy<IntGaugeVec> = Lazy::new(|| { |
There was a problem hiding this comment.
Either this one or AVAILABLE_MEMORY_BYTES is redundant.
There was a problem hiding this comment.
Didn't we want a total to ensure partners have enough memory? I figured available would be useful to alert off of as well.
There was a problem hiding this comment.
Just saying that available + used = total
| @@ -227,6 +228,9 @@ impl MpcSignProtocol { | |||
| loop { | |||
| let protocol_time = Instant::now(); | |||
| tracing::trace!("trying to advance chain signatures protocol"); | |||
| // Hardware metric refresh | |||
| update_system_metrics(&my_account_id); | |||
There was a problem hiding this comment.
How often is this executed in practice?
There was a problem hiding this comment.
@ppca might know better than I 😂
There was a problem hiding this comment.
as often as a loop. Might be too often in this case. I think we can do once every minute?
add
let mut last_hardware_pull = Instant::now(); at around here:
mpc/chain-signatures/node/src/protocol/mod.rs
Line 215 in 11dd7e0
and
if last_hardware_pull.elapsed() > Duration::from_secs(60) {
update_system_metrics(&my_account_id);;
}
at around
mpc/chain-signatures/node/src/protocol/mod.rs
Line 229 in 11dd7e0
This will update the metrics every minute. @volovyks @kmaus-near
There was a problem hiding this comment.
Can we do that every ~10 seconds? I'm afraid we can lose CPU spikes.
There was a problem hiding this comment.
yeah, let's change to if last_hardware_pull.elapsed() > Duration::from_secs(10) then
There was a problem hiding this comment.
I think the example the crate uses is 5s, which would be pretty common practice for hardware metrics, and is the lowest refresh rate in grafana.
chain-signatures/node/src/metrics.rs
Outdated
| }); | ||
|
|
||
| // Disk Space Metric | ||
| pub(crate) static DISK_SPACE_BYTES: Lazy<IntGaugeVec> = Lazy::new(|| { |
There was a problem hiding this comment.
Should we also have available and used for the DISK_SPACE?
There was a problem hiding this comment.
This one is Available disk space, mostly for alerting purposes. We can also add a total disk space just so there's visibility into it.
There was a problem hiding this comment.
Let's specify in the name if it is used or remaining.
There was a problem hiding this comment.
Fixed, I will do a calculation in grafana for used disk space since only total and available is part of that library.
chain-signatures/node/src/metrics.rs
Outdated
| }); | ||
|
|
||
| // Total Memory Metric | ||
| pub(crate) static TOTAL_MEMORY_BYTES: Lazy<IntGaugeVec> = Lazy::new(|| { |
There was a problem hiding this comment.
Just saying that available + used = total
I think we are alright. I was slightly concerned with CPU metrics being inaccurate, but it seems they are a pretty decent representation of actual host metrics after watching the grafana metrics vs the host metrics. Let me convert this to a PR and remove the Total memory metric, if we need it we can calculate it from the other two. |
|
Please rebase latest develop and fix the test failures. |
| if last_hardware_pull.elapsed() > Duration::from_secs(5) { | ||
| update_system_metrics(&my_account_id); | ||
| } | ||
|
|
||
| tracing::debug!("trying to advance chain signatures protocol"); |
There was a problem hiding this comment.
maybe delete this one then? there are 2 tracing::debug!("trying to advance chain signatures protocol");
Still need to actually test if this does anything more than compile, but here are some eyes on what I came up with.