Propagate TraceNotFound error from grpc storage plugins #2455
Conversation
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
| return nil, spanstore.ErrTraceNotFound | ||
| } | ||
| traceNotFoundStatus, _ := status.FromError(spanstore.ErrTraceNotFound) | ||
| if err.Error() == traceNotFoundStatus.Err().Error() { |
There was a problem hiding this comment.
traceNotFoundStatus.Err().Error() prints as:
rpc error: code = Unknown desc = trace not found
I wonder if there's a better way than comparing strings. Especially suspicious is code = Unknown.
There was a problem hiding this comment.
Agree, pushed something that i like better. It's still string comparison, but it is now comparing directly "trace not found".
The original error was created in a different process so I don't think there's any clean way to compare error equality. I believe string comparison is as good as it gets here.
| withGRPCClient(func(r *grpcClientTest) { | ||
| traceClient := new(grpcMocks.SpanReaderPlugin_GetTraceClient) | ||
| traceClient.On("Recv").Return(nil, spanstore.ErrTraceNotFound) | ||
| traceClient.On("Recv").Return(nil, s.Err()) |
There was a problem hiding this comment.
I think we should write a test that goes through client and server, not just mocking the client.
There was a problem hiding this comment.
Agree. I can't say I have time right now to knock it out. Perhaps if i find some time in the next week or two I'll dig into it.
Codecov Report
@@ Coverage Diff @@
## master #2455 +/- ##
=======================================
Coverage 95.56% 95.56%
=======================================
Files 208 208
Lines 10690 10690
=======================================
Hits 10216 10216
Misses 401 401
Partials 73 73
Continue to review full report at Codecov.
|
Signed-off-by: Joe Elliott <number101010@gmail.com>
| @@ -77,10 +77,9 @@ func (c *grpcClient) GetTrace(ctx context.Context, traceID model.TraceID) (*mode | |||
| trace := model.Trace{} | |||
| for received, err := stream.Recv(); err != io.EOF; received, err = stream.Recv() { | |||
| if err != nil { | |||
| if e, ok := status.FromError(err); !ok { | |||
There was a problem hiding this comment.
wasn't this if statement mostly correct, except the condition should be ok instead of !ok?
There was a problem hiding this comment.
Yup, that's true. I prefer what I have pushed b/c it doesn't matter if err is an error or a status.Status if the message is "trace not found" then it will return the correct error.
We can do the minimal change if it's preferred though.
There was a problem hiding this comment.
the original version (aside from !ok) seems better, because your version is ignoring the error, so s may be undefined.
There was a problem hiding this comment.
good point. I am relying on status.FromError to return a non-nil status. I think this would the best construction
if s, _ := status.FromError(err); s != nil {
// check if s is a trace not found error.
}
ok just indicates if FromError had to create a new status.Status or if it was able to retrieve one using the GRPCStatus() method. For our purposes we don't care, we're just going to compare s.Message() to "trace not found" either way.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
A previous attempt to fix this was made here: #1814
Original issue: #1741
However, this is currently not working b/c on this line
okis set to true which skips the code that checks to see if the string equals "trace not found".jaeger/plugin/storage/grpc/shared/grpc_client.go
Line 80 in 4120220
Tracking it down:
RecvMsg
https://github.com/grpc/grpc-go/blob/8630cac324bf02ea0edfba758c2b2f3344af7bf7/stream.go#L749
recvMsg
https://github.com/grpc/grpc-go/blob/master/stream.go#L886
toRPCErr
https://github.com/grpc/grpc-go/blob/8630cac324bf02ea0edfba758c2b2f3344af7bf7/rpc_util.go#L775
toRPCErrcallsstatus.FromErrorwhich wraps the error in aGRPCStatus. This causes theFromErroringrpc_client.goto recognize the error asGRPCStatusand returntrue.Fix:
status.FromErrorcheck and test the error directly.GRPCStatuswrappedspanstore.ErrTraceNotFoundI have confirmed this works manually by building jaeger-query with a custom GRPC plugin.