feat(multipath): add back basic metrics#3672
Conversation
| pub actor_tick_other: Counter, | ||
|
|
||
| /// Number of endpoints we have attempted to contact. | ||
| pub endpoints_contacted: Counter, |
There was a problem hiding this comment.
I don't think we ever could do this in a good way, because we don't keep lists of endpoint ids over restarts, so this was already a bad metric for a while - removed it.
| /// Number of endpoints we have attempted to contact. | ||
| pub endpoints_contacted: Counter, | ||
| /// Number of endpoints we have managed to contact directly. | ||
| pub endpoints_contacted_directly: Counter, |
There was a problem hiding this comment.
same as above, I don't think there's a good way to do it meaningfully.
| /// Number of connections with a successful handshake. | ||
| pub connection_handshake_success: Counter, | ||
| /// Number of connections with a successful handshake that became direct. | ||
| pub connection_became_direct: Counter, |
There was a problem hiding this comment.
These two are better handled by the new conn_ metrics above
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3672/docs/iroh/ Last updated: 2025-12-09T11:27:18Z |
| metrics.num_conns_transport_ip_only.inc(); | ||
| } | ||
| TransportSummary::RelayOnly => { | ||
| metrics.num_conns_transport_relay_only.inc(); | ||
| } | ||
| TransportSummary::IpAndRelay => { | ||
| metrics.num_conns_transport_ip_and_relay.inc(); |
There was a problem hiding this comment.
Talked with @Arqu and we agreed to remove these 3 metrics and instead have these 4:
ip_transport_paths_added: counterip_transport_paths_removed: counterrelay_transport_paths_added: counterrelay_transport_paths_removed: counter
That way we can increment counters when paths are opened and closed. It doesn't give you full view into what connections have been doing, but neither does this, because typically nearly everything would have been num_conns_transport_ip_and_relay.
|
I rebased this branch, removed the TransportSummary metrics and added the paths counters. I named the counters to |
flub
left a comment
There was a problem hiding this comment.
Generally LGTM though would appreciate fixing the nits.
Thanks for getting back to this!
| &mut self, | ||
| addr: transports::Addr, | ||
| source: Source, | ||
| metrics: &MagicsockMetrics, |
There was a problem hiding this comment.
I'd suggest to clone MagicsockMetrics into here, it exists for the same duration as the actor anyway. But this is optional, as you wish.
| metrics: &MagicsockMetrics, | ||
| ) { | ||
| match addr { | ||
| transports::Addr::Ip(_) => metrics.transport_paths_ip_added.inc(), |
There was a problem hiding this comment.
Names are pretty subjective, I think I'll argue for transport_{ip|relay}_paths_added. Reason being that paths_added is the counter unit and tranport_ip belongs together as ip is the qualifier to transports. Good call to put transports at the front though.
Now what I really want is labels so we can do paths_added{transport=ip} but hey.
iroh/src/magicsock/metrics.rs
Outdated
| // pub path_rtt_variance_ms: Histogram, | ||
| // /// Histogram of path quality scores (0.0-1.0). | ||
| // #[default(Histogram::new(vec![0.0, 0.3, 0.5, 0.7, 0.85, 0.95, 1.0]))] | ||
| // pub path_quality_score: Histogram, |
There was a problem hiding this comment.
Shall we remove the commented out stuff? I think we can come up with this again if we do end up needing something like it (well, I'm hoping we'd come up with something better 😉 )
Description
This adds back basic metrics to
feat-multipath.conn_metrics (they were all currently unused onfeat-multipath) into something meaningful: Track the total number of opened and closed connections - their diff at any time is the number of currently-active connetransport_ip_paths_added,transport_ip_paths_removed,transport_relay_paths_added,transport_relay_paths_removedcountersBreaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme