feat(cli): add metrics for download command #19870
Conversation
Rjected
left a comment
Rjected
left a comment
There was a problem hiding this comment.
I think adding metrics for this is reasonable, I have a few suggestions
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
unsure if we have have a prometheus supported for this command
so we should check this as well
crates/cli/commands/src/download.rs
Outdated
| let metrics = DownloadMetrics::global(); | ||
| let overall_start = Instant::now(); | ||
|
|
||
| metrics.downloads_started_total.increment(1); |
There was a problem hiding this comment.
this metric is only useful if we make use of this command a lot?
because this command impl only ever does one download
There was a problem hiding this comment.
yep removed both - global static is gone and dropped downloads_started_total since it doesn't make sense here
good point about prometheus though, this command doesn't actually expose metrics (no --metrics flag). so kinda pointless right now?
should i just remove them or add the endpoint like stage run has?
There was a problem hiding this comment.
should i just remove them or add the endpoint like stage run has?
if the motivation for this pr is to make them observable via prometheus then we should add support for this via a metrics arg imo and launch the prometheus listener if set
- no more global static, just pass it around - removed unused _started_at field - simplified the blocking_download function (no closure needed) - dropped downloads_started_total metric, doesn't make sense here
3 participants
We're downloading 2TB+ mainnet snapshots that take 48+ hours but had zero visibility into what's happening.
Before:
Downloading and extracting... 42.31% (904.23 GB / 2.08 TB)That's it. No way to track via Prometheus, no failure stats, nothing.
After:
Can now properly monitor downloads:
rate(cli_download_downloaded_bytes_total[1m]) < 10MB)