Report context cancelation as non-error - #1
Conversation
Someone else canceled this operation; we should not flag it as an error. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
charleskorn
left a comment
charleskorn
left a comment
There was a problem hiding this comment.
Thanks for this.
For consistency, should we also change the events added to the span when a cancellation "error" is received?
Currently, the code here, here and here will log all Golang error values with event=error, even if they represent a cancellation. Perhaps we should log them with event=cancelled instead?
| code := codes.Unknown | ||
| if s, ok := status.FromError(err); ok { | ||
| code = s.Code() | ||
| } else if errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
We also need to check for code == codes.Canceled here - the gRPC client libraries translate client-side context cancellations into gRPC errors with code Canceled using either FromContextError or toRPCErr depending on when the error is observed by the client.
TestStreamingContextCancellationOpenTracing doesn't cover this case because it doesn't test cancelling the context while a client is in a Recv() or Send() call - it cancels the context while the stream is open, but not while the client is in Recv() or Send().
There was a problem hiding this comment.
Thanks; I have modified the code to check codes.Canceled
There was a problem hiding this comment.
Thanks. Could you please add a test to cover this case as well?
Nice, but not important to me since nobody is putting a big red mark on the screen based on events. |
2 participants
Someone else canceled this operation; we should not flag it as an error.
Example of a span flagged as an error when the operation was cancelled as un-needed:
Copied from opentracing-contrib#13