-
Notifications
You must be signed in to change notification settings - Fork 109
Reduce 5xx spam #496
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: main
Are you sure you want to change the base?
Reduce 5xx spam #496
Conversation
| Status: reason, | ||
| } | ||
| result = psrpc.NewErrorf(psrpc.Canceled, "call transfer failed: %w", st) | ||
| psrpcErrCode := psrpc.UpstreamServerError |
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.
Going over twirp, this will still produce a bad error, due to Twirp not having the appropriate response codes to convert this to. However, data indicates the vast majority of errors are not 5XX errors and are going to be UpstreamClientError. which would convert to twirp.InvalidArgument.
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.
this isn't related to Twirp, but our error code mapping in psrpc? why don't we just map it to the right thing there?
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.
This completely ignores the existing mapping to gRPC/PSRPC/Twirp error codes that we have. Maybe we should update the mapping there instead?
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.
do we also handle other common incorrect error mapping? for example, CreateSIPParticipant is also throwing some of these errors
| Status: reason, | ||
| } | ||
| result = psrpc.NewErrorf(psrpc.Canceled, "call transfer failed: %w", st) | ||
| psrpcErrCode := psrpc.UpstreamServerError |
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.
this isn't related to Twirp, but our error code mapping in psrpc? why don't we just map it to the right thing there?
Today we return
psrpc.Canceled, which then gets translated to HTTP code 500. This can be quite problematic when we get return codes like:480 Temporarily Unavailable,603 Decline,486 Busy Here,404 Not Found. These are either user errors, outright rejections, or numbers incapable of handling two simultaneous calls.This change will get us to emit
UpstreamServerErrorandUpstreamClientErroras responses, that would translate to HTTP codes 502 and 424 respectively, correctly identifying failures in resources upstream of the sip server.