-
Notifications
You must be signed in to change notification settings - Fork 327
feat: basic holepunch metrics #3748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat-multipath
Are you sure you want to change the base?
Conversation
eff1c4b to
acbc048
Compare
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3748/docs/iroh/ Last updated: 2025-12-10T12:24:53Z |
ca85a9f to
f6c5e49
Compare
0a4ba70 to
fed03ef
Compare
|
Help, I find this very hard to think about. What do we really want to measure here? I don't have a good answer to this so find commenting on the PR hard. @Arqu maybe has input on what we really need to have? Things that are easy to measure:
Things that are hard(er) to measure:
My inclination is to concentrate on the easy ones, but do you learn anything useful from those? You can't do any maths to tell you how good your holepunching is I think, so what's the point even? This goes back to, what are we trying to measure? |
| /// The number of NAT traversal attempts initiated. | ||
| pub nat_traversal: Counter, | ||
| /// The number of remote endpoints for which of NAT traversal was initiated. | ||
| pub remote_holepunch_attempts: Counter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "remote_" prefix makes me think this was initiated by the remote.
Essentially the metrics you added are:
- remotes connected to ("remotes_connection_established"?)
- remotes holepunched ("remotes_connection_direct"?)
IIUC. This could be useful I guess. Not sure, it's also a bit weird (see separate comment).
| && hp.remote_candidates.contains(ip_addr) | ||
| { | ||
| self.has_holepunched = true; | ||
| self.metrics.remote_holepunch_success.inc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue with this impl is that this metric would only be incremented on the client, and not the server. But that's a bit counter-intuitive from the user point of view.
|
So I think the original intention was that you could have two metrics like:
And now you can do maths on them because you can compute the number of connections that did not holepunch and have a holepunching rate. But these metrics fall under the "hard" metrics 😞 I'm not sure how to do them. |
Description
This adds two metrics:
remote_holepunch_attemptcounts the number of remotes for which we initiated holepunching.remote_holepunch_successcounts the number of remotes where we initiated holepunching and had successBoth are either increased by 0 or 1 during the lifetime of a
RemoteStateActor, never more. We incrase_attemptonce on the first time when holepunching is initiated. We increase_successonce if a new IP path is opened whose remote address is part of the last holepunching round.I'm not entirely sure if this logic is fully sound, but it's the best I could come up with so far.
Fixes #3695
Breaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme