Companion PR for removing Prometheus metrics prefix #3623
Conversation
tomaka
added
the
A4-companion
tomaka
added
B0-silent
|
Sorry I've been missing a bunch of metrics because the code style isn't the same as Substrate. Will do another thorough pass. |
There was a problem hiding this comment.
If you're curious about the origin of metric_name, this is the relevant PR: paritytech/parity-bridges-common#878
bridges/relays/utils/src/metrics.rs
Outdated
| } else { | ||
| name.into() | ||
| format!("polkadot_{}", name) |
There was a problem hiding this comment.
This piece of code isn't Polkadot specific (and in fact, isn't technically used by the Polkadot repo), so there's no need for this change
| @@ -270,7 +270,8 @@ fn create_metrics_registry(prefix: Option<String>) -> Registry { | |||
| match prefix { | |||
| Some(prefix) => { | |||
| assert!(!prefix.is_empty(), "Metrics prefix can not be empty"); | |||
| Registry::new_custom(Some(prefix), None).expect("only fails if prefix is empty; prefix is not empty; qed") | |||
| Registry::new_custom(None, Some(iter::once((String::from("chain"), prefix)).collect())) | |||
There was a problem hiding this comment.
I'm not too familar with Prometheus, so why is it better to use a label instead of a prefix here?
There was a problem hiding this comment.
With a prefix you can't easily use the same Grafana dashboard across multiple prefixes.
See paritytech/substrate#9543
bridges/relays/utils/src/metrics.rs
Outdated
| @@ -121,9 +121,9 @@ impl From<Option<MetricsAddress>> for MetricsParams { | |||
| /// Returns metric name optionally prefixed with given prefix. | |||
| pub fn metric_name(prefix: Option<&str>, name: &str) -> String { | |||
| if let Some(prefix) = prefix { | |||
| format!("{}_{}", prefix, name) | |||
| format!("polkadot_{}_{}", prefix, name) | |||
There was a problem hiding this comment.
Same comment here (I forgot to highlight the block)
|
@tomaka this also needs to be updated. |
| bob: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds | ||
| charlie: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds | ||
| charlie: reports parachain_candidate_dispute_concluded{validity="valid"} is at least 1 within 90 seconds | ||
| alice: reports polkadot_parachain_candidate_disputes_total is at least 1 within 250 seconds |
There was a problem hiding this comment.
this doesn't need to be updated I believe cc @pepoviola
There was a problem hiding this comment.
Let's wait what the test is saying.
There was a problem hiding this comment.
Hi @ordian, no. The prefix isn't expected.
Thx!
There was a problem hiding this comment.
@pepoviola it seems there are some hard coded metrics. See the tests below. Can you fix this?
There was a problem hiding this comment.
Test are failing, let me review how the metrics are reported with this changes.
Thx!
There was a problem hiding this comment.
Hi @bkchr, I check why the test are failing and with this changes metrics are reported like:
substrate_sub_libp2p_peers_count{chain="/cfg/wococo-local.json"} 5
The issue is the suffix {chain="/cfg/wococo-local.json"} is this needed? Currently the metrics are reported without that.
polkadot_sub_libp2p_peers_count 3
Zombiente will try to get the metric without looking the prefix (polkadot/substrate) but don't have the logic to add the chain suffix.
Thanks!
There was a problem hiding this comment.
The issue is the suffix {chain="/cfg/wococo-local.json"}
This is a bug and should be only chain=wococo. I'm working on a fix. However, this chain arg stays.
There was a problem hiding this comment.
Oks, this will need some changes in zombienet to match the metrics. I will work on that asap and update the runner image in CI to review.
Thanks!
There was a problem hiding this comment.
Hi @bkchr, I already added support for the suffix in zombienet and now test are ok.
Thanks!
pepyakin
added
B1-releasenotes
and removed
B0-silent
|
I think this PR was mislabeled: I think this is a change that worth to put into polkadot changelogs. |
pepyakin
added
B0-silent
|
nevermind, actually it doesn't change the metric names. |
Companion for paritytech/substrate#9543
I've removed the
set_prometheus_registryfunction altogether, as Substrate automatically customizes the registry with the chain id.cc @HCastano I've tweaked the
metric_namefunction.I don't really understand the reason for this function (it was introduced in 9781c79 😬 ), so let me know if this is correct.