Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

fix error assertions in the tracer #234

Merged
merged 1 commit into from
Oct 19, 2021
Merged

Conversation

marten-seemann
Copy link
Collaborator

Fixes #233.

errors.Is and errors.As still confuses me. What was happening here was that the case for the version negotiation errors didn't recognize a quic.VersionNegotiationError because... well, I don't really know.
I assume it's because a *quic.VersionNegotiationError has (exported) fields, and errors.Is was looking for an exact match, not just a type match. This is what errors.As apparently does.
The same applies to a bunch of other error types as well.

With this PR, we should be able to properly increment the counter for version negotiation errors (and the other error types), instead of creating a new Prometheus counter for every error.

Copy link

@mvdan mvdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, Is is roughly equivalent to err == ErrSentinel, and As is roughly equivalent to err.(ErrType), but both supporting error wrapping.

FYI, if you don't actually need to use the variables like applicationErr, you could keep the code as it was before, like:

case errors.As(e, &quic.ApplicationError{}):

Though most times one uses As, it's also to inspect the details of the typed error. Up to you.

@marten-seemann
Copy link
Collaborator Author

FYI, if you don't actually need to use the variables like applicationErr, you could keep the code as it was before, like:

case errors.As(e, &quic.ApplicationError{}):

Though most times one uses As, it's also to inspect the details of the typed error. Up to you.

Unfortunately, that doesn't work (and is one of the reasons I find working with errors.As so annoying). You need a pointer to a pointer:

# github.com/libp2p/go-libp2p-quic-transport
./tracer_metrics.go:243:7: second argument to errors.As must be a non-nil pointer to either a type that implements error, or to any interface type

Or is there something I'm missing?

@marten-seemann marten-seemann merged commit 54ec635 into master Oct 19, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing of version negotiation failures is wasteful
2 participants