p2p: fix dial metrics not picking up the right error #31621
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original error was not wrapped, so the caller function had no chance of picking it up. Signed-off-by: Csaba Kiraly <[email protected]>
fjl
reviewed
Apr 14, 2025
Signed-off-by: Csaba Kiraly <[email protected]>
cskiraly
force-pushed
the
fix-dial-meters
branch
from
3a8c359 to
f202047
Compare
fjl
reviewed
Apr 14, 2025
| @@ -67,23 +67,22 @@ func markDialError(err error) { | |||
| if !metrics.Enabled() { | |||
| return | |||
| } | |||
| if err2 := errors.Unwrap(err); err2 != nil { | |||
There was a problem hiding this comment.
I think what we really want here is
var reason DiscReason
if errors.As(err, &reason) {
switch reason {
...
}
}
var phe *protoHandshakeError
if errors.As(err, &phe) {
dialProtoHandshakeError.Mark(1)
}There was a problem hiding this comment.
Sounds reasonable, and also cleaner. I will verify how it works, and come back with a patch.
There was a problem hiding this comment.
The dialProtoHandshakeError would have to be in an else branch, and we would still need to handle dialEncHandshakeError separately.
I would say the original with Is and As is better. I've added a catch-all and some comments to make it even cleaner.
Signed-off-by: Csaba Kiraly <[email protected]>
Signed-off-by: Csaba Kiraly <[email protected]>
We can't pass errProtoHandshakeError here, because the framework uses `Is` for testing, and that would need `As`. We can however test how it works for internal errors, which is the most typical case. Signed-off-by: Csaba Kiraly <[email protected]>
Signed-off-by: Csaba Kiraly <[email protected]>
|
LGTM |
fjl
approved these changes
Apr 15, 2025
sivaratrisrinivas
pushed a commit
to sivaratrisrinivas/go-ethereum
that referenced
this pull request
Apr 21, 2025
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted. The metrics were originally introduced in ethereum#27621 I was thinking of various possible solutions. - the one proposed here wraps both the new error and the origial error. It is not a pattern we use in other parts of the code, but works. This is maybe the smallest possible change. - as an alternate, I could write a proper `errProtoHandshakeError` with it's own wrapped error - finally, I'm not even sure we need `errProtoHandshakeError`, maybe we could just pass up the original error. --------- Signed-off-by: Csaba Kiraly <[email protected]> Co-authored-by: Felix Lange <[email protected]>
0g-wh
pushed a commit
to 0glabs/0g-geth
that referenced
this pull request
Apr 22, 2025
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted. The metrics were originally introduced in ethereum#27621 I was thinking of various possible solutions. - the one proposed here wraps both the new error and the origial error. It is not a pattern we use in other parts of the code, but works. This is maybe the smallest possible change. - as an alternate, I could write a proper `errProtoHandshakeError` with it's own wrapped error - finally, I'm not even sure we need `errProtoHandshakeError`, maybe we could just pass up the original error. --------- Signed-off-by: Csaba Kiraly <[email protected]> Co-authored-by: Felix Lange <[email protected]>
jakub-freebit
pushed a commit
to fblch/go-ethereum
that referenced
this pull request
Jul 3, 2025
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted. The metrics were originally introduced in ethereum#27621 I was thinking of various possible solutions. - the one proposed here wraps both the new error and the origial error. It is not a pattern we use in other parts of the code, but works. This is maybe the smallest possible change. - as an alternate, I could write a proper `errProtoHandshakeError` with it's own wrapped error - finally, I'm not even sure we need `errProtoHandshakeError`, maybe we could just pass up the original error. --------- Signed-off-by: Csaba Kiraly <[email protected]> Co-authored-by: Felix Lange <[email protected]>
jakub-freebit
pushed a commit
to fblch/go-ethereum
that referenced
this pull request
Jul 23, 2025
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted. The metrics were originally introduced in ethereum#27621 I was thinking of various possible solutions. - the one proposed here wraps both the new error and the origial error. It is not a pattern we use in other parts of the code, but works. This is maybe the smallest possible change. - as an alternate, I could write a proper `errProtoHandshakeError` with it's own wrapped error - finally, I'm not even sure we need `errProtoHandshakeError`, maybe we could just pass up the original error. --------- Signed-off-by: Csaba Kiraly <[email protected]> Co-authored-by: Felix Lange <[email protected]>
howjmay
pushed a commit
to iotaledger/go-ethereum
that referenced
this pull request
Aug 27, 2025
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted. The metrics were originally introduced in ethereum#27621 I was thinking of various possible solutions. - the one proposed here wraps both the new error and the origial error. It is not a pattern we use in other parts of the code, but works. This is maybe the smallest possible change. - as an alternate, I could write a proper `errProtoHandshakeError` with it's own wrapped error - finally, I'm not even sure we need `errProtoHandshakeError`, maybe we could just pass up the original error. --------- Signed-off-by: Csaba Kiraly <[email protected]> Co-authored-by: Felix Lange <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our metrics related to dial errors were off. The original error was not wrapped, so the caller function had no chance of picking it up. Therefore the most common error, which is "TooManyPeers", was not correctly counted.
The metrics were originally introduced in #27621
I was thinking of various possible solutions.
errProtoHandshakeErrorwith it's own wrapped errorerrProtoHandshakeError, maybe we could just pass up the original error.