Skip to content

Commit 83b2679

Browse files
yurishkuroyurishkuro
authored
Return appropriate gRPC errors/codes to indicate request status (#2132)
* User gRPC codes to indicate request status Signed-off-by: Yuri Shkuro <ys@uber.com> * test for not found code Signed-off-by: Yuri Shkuro <ys@uber.com> * fix capitalization Signed-off-by: Yuri Shkuro <ys@uber.com> * cleanup Signed-off-by: Yuri Shkuro <ys@uber.com>
1 parent 6149038 commit 83b2679

File tree

3 files changed

+53
-49
lines changed

3 files changed

+53
-49
lines changed

‎cmd/query/app/grpc_handler.go

+29-23
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,22 @@ import (
1919

2020
"github.com/opentracing/opentracing-go"
2121
"go.uber.org/zap"
22+
"google.golang.org/grpc/codes"
23+
"google.golang.org/grpc/status"
2224

2325
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
2426
"github.com/jaegertracing/jaeger/model"
2527
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
2628
"github.com/jaegertracing/jaeger/storage/spanstore"
2729
)
2830

29-
const maxSpanCountInChunk = 10
31+
const (
32+
maxSpanCountInChunk = 10
3033

31-
// GRPCHandler implements the GRPC endpoint of the query service.
34+
msgTraceNotFound = "trace not found"
35+
)
36+
37+
// GRPCHandler implements the gRPC endpoint of the query service.
3238
type GRPCHandler struct {
3339
queryService *querysvc.QueryService
3440
logger *zap.Logger
@@ -46,36 +52,36 @@ func NewGRPCHandler(queryService *querysvc.QueryService, logger *zap.Logger, tra
4652
return gH
4753
}
4854

49-
// GetTrace is the GRPC handler to fetch traces based on trace-id.
55+
// GetTrace is the gRPC handler to fetch traces based on trace-id.
5056
func (g *GRPCHandler) GetTrace(r *api_v2.GetTraceRequest, stream api_v2.QueryService_GetTraceServer) error {
5157
trace, err := g.queryService.GetTrace(stream.Context(), r.TraceID)
5258
if err == spanstore.ErrTraceNotFound {
53-
g.logger.Error("trace not found", zap.Error(err))
54-
return err
59+
g.logger.Error(msgTraceNotFound, zap.Error(err))
60+
return status.Errorf(codes.NotFound, "%s: %v", msgTraceNotFound, err)
5561
}
5662
if err != nil {
57-
g.logger.Error("Could not fetch spans from backend", zap.Error(err))
58-
return err
63+
g.logger.Error("failed to fetch spans from the backend", zap.Error(err))
64+
return status.Errorf(codes.Internal, "failed to fetch spans from the backend: %v", err)
5965
}
6066
return g.sendSpanChunks(trace.Spans, stream.Send)
6167
}
6268

63-
// ArchiveTrace is the GRPC handler to archive traces.
69+
// ArchiveTrace is the gRPC handler to archive traces.
6470
func (g *GRPCHandler) ArchiveTrace(ctx context.Context, r *api_v2.ArchiveTraceRequest) (*api_v2.ArchiveTraceResponse, error) {
6571
err := g.queryService.ArchiveTrace(ctx, r.TraceID)
6672
if err == spanstore.ErrTraceNotFound {
6773
g.logger.Error("trace not found", zap.Error(err))
68-
return nil, err
74+
return nil, status.Errorf(codes.NotFound, "%s: %v", msgTraceNotFound, err)
6975
}
7076
if err != nil {
71-
g.logger.Error("Could not fetch spans from backend", zap.Error(err))
72-
return nil, err
77+
g.logger.Error("failed to archive trace", zap.Error(err))
78+
return nil, status.Errorf(codes.Internal, "failed to archive trace: %v", err)
7379
}
7480

7581
return &api_v2.ArchiveTraceResponse{}, nil
7682
}
7783

78-
// FindTraces is the GRPC handler to fetch traces based on TraceQueryParameters.
84+
// FindTraces is the gRPC handler to fetch traces based on TraceQueryParameters.
7985
func (g *GRPCHandler) FindTraces(r *api_v2.FindTracesRequest, stream api_v2.QueryService_FindTracesServer) error {
8086
query := r.GetQuery()
8187
queryParams := spanstore.TraceQueryParameters{
@@ -90,8 +96,8 @@ func (g *GRPCHandler) FindTraces(r *api_v2.FindTracesRequest, stream api_v2.Quer
9096
}
9197
traces, err := g.queryService.FindTraces(stream.Context(), &queryParams)
9298
if err != nil {
93-
g.logger.Error("Error fetching traces", zap.Error(err))
94-
return err
99+
g.logger.Error("failed when searching for traces", zap.Error(err))
100+
return status.Errorf(codes.Internal, "failed when searching for traces: %v", err)
95101
}
96102
for _, trace := range traces {
97103
if err := g.sendSpanChunks(trace.Spans, stream.Send); err != nil {
@@ -116,18 +122,18 @@ func (g *GRPCHandler) sendSpanChunks(spans []*model.Span, sendFn func(*api_v2.Sp
116122
return nil
117123
}
118124

119-
// GetServices is the GRPC handler to fetch services.
125+
// GetServices is the gRPC handler to fetch services.
120126
func (g *GRPCHandler) GetServices(ctx context.Context, r *api_v2.GetServicesRequest) (*api_v2.GetServicesResponse, error) {
121127
services, err := g.queryService.GetServices(ctx)
122128
if err != nil {
123-
g.logger.Error("Error fetching services", zap.Error(err))
124-
return nil, err
129+
g.logger.Error("failed to fetch services", zap.Error(err))
130+
return nil, status.Errorf(codes.Internal, "failed to fetch services: %v", err)
125131
}
126132

127133
return &api_v2.GetServicesResponse{Services: services}, nil
128134
}
129135

130-
// GetOperations is the GRPC handler to fetch operations.
136+
// GetOperations is the gRPC handler to fetch operations.
131137
func (g *GRPCHandler) GetOperations(
132138
ctx context.Context,
133139
r *api_v2.GetOperationsRequest,
@@ -137,8 +143,8 @@ func (g *GRPCHandler) GetOperations(
137143
SpanKind: r.SpanKind,
138144
})
139145
if err != nil {
140-
g.logger.Error("Error fetching operations", zap.Error(err))
141-
return nil, err
146+
g.logger.Error("failed to fetch operations", zap.Error(err))
147+
return nil, status.Errorf(codes.Internal, "failed to fetch operations: %v", err)
142148
}
143149

144150
result := make([]*api_v2.Operation, len(operations))
@@ -155,14 +161,14 @@ func (g *GRPCHandler) GetOperations(
155161
}, nil
156162
}
157163

158-
// GetDependencies is the GRPC handler to fetch dependencies.
164+
// GetDependencies is the gRPC handler to fetch dependencies.
159165
func (g *GRPCHandler) GetDependencies(ctx context.Context, r *api_v2.GetDependenciesRequest) (*api_v2.GetDependenciesResponse, error) {
160166
startTime := r.StartTime
161167
endTime := r.EndTime
162168
dependencies, err := g.queryService.GetDependencies(startTime, endTime.Sub(startTime))
163169
if err != nil {
164-
g.logger.Error("Error fetching dependencies", zap.Error(err))
165-
return nil, err
170+
g.logger.Error("failed to fetch dependencies", zap.Error(err))
171+
return nil, status.Errorf(codes.Internal, "failed to fetch dependencies: %v", err)
166172
}
167173

168174
return &api_v2.GetDependenciesResponse{Dependencies: dependencies}, nil

‎cmd/query/app/grpc_handler_test.go

+23-25
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"github.com/stretchr/testify/require"
3030
"go.uber.org/zap"
3131
"google.golang.org/grpc"
32+
"google.golang.org/grpc/codes"
3233
"google.golang.org/grpc/status"
3334

3435
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
@@ -40,12 +41,9 @@ import (
4041
)
4142

4243
var (
43-
errStorageMsgGRPC = "Storage error"
44-
errStorageGRPC = errors.New(errStorageMsgGRPC)
45-
errStatusStorageGRPC = status.Error(2, errStorageMsgGRPC)
44+
errStorageGRPC = errors.New("storage error")
4645

47-
mockTraceIDgrpc = model.NewTraceID(0, 123456)
48-
mockTraceGRPC = &model.Trace{
46+
mockTraceGRPC = &model.Trace{
4947
Spans: []*model.Span{
5048
{
5149
TraceID: mockTraceID,
@@ -207,7 +205,7 @@ func TestGetTraceSuccessGRPC(t *testing.T) {
207205
Return(mockTrace, nil).Once()
208206

209207
res, err := client.GetTrace(context.Background(), &api_v2.GetTraceRequest{
210-
TraceID: mockTraceIDgrpc,
208+
TraceID: mockTraceID,
211209
})
212210

213211
spanResChunk, _ := res.Recv()
@@ -218,22 +216,27 @@ func TestGetTraceSuccessGRPC(t *testing.T) {
218216
})
219217
}
220218

219+
func assertGRPCError(t *testing.T, err error, code codes.Code, msg string) {
220+
s, ok := status.FromError(err)
221+
require.True(t, ok, "expecting gRPC status")
222+
assert.Equal(t, code, s.Code())
223+
assert.Contains(t, s.Message(), msg)
224+
}
225+
221226
func TestGetTraceDBFailureGRPC(t *testing.T) {
222227
withServerAndClient(t, func(server *grpcServer, client *grpcClient) {
223228

224229
server.spanReader.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")).
225230
Return(nil, errStorageGRPC).Once()
226231

227232
res, err := client.GetTrace(context.Background(), &api_v2.GetTraceRequest{
228-
TraceID: mockTraceIDgrpc,
233+
TraceID: mockTraceID,
229234
})
230235
assert.NoError(t, err)
231236

232237
spanResChunk, err := res.Recv()
233-
234-
assert.EqualError(t, err, errStatusStorageGRPC.Error())
238+
assertGRPCError(t, err, codes.Internal, "failed to fetch spans from the backend")
235239
assert.Nil(t, spanResChunk)
236-
237240
})
238241

239242
}
@@ -248,14 +251,12 @@ func TestGetTraceNotFoundGRPC(t *testing.T) {
248251
Return(nil, spanstore.ErrTraceNotFound).Once()
249252

250253
res, err := client.GetTrace(context.Background(), &api_v2.GetTraceRequest{
251-
TraceID: mockTraceIDgrpc,
254+
TraceID: mockTraceID,
252255
})
253256
assert.NoError(t, err)
254257
spanResChunk, err := res.Recv()
255-
256-
assert.Errorf(t, err, spanstore.ErrTraceNotFound.Error())
258+
assertGRPCError(t, err, codes.NotFound, "trace not found")
257259
assert.Nil(t, spanResChunk)
258-
259260
})
260261
}
261262

@@ -267,7 +268,7 @@ func TestArchiveTraceSuccessGRPC(t *testing.T) {
267268
Return(nil).Times(2)
268269

269270
_, err := client.ArchiveTrace(context.Background(), &api_v2.ArchiveTraceRequest{
270-
TraceID: mockTraceIDgrpc,
271+
TraceID: mockTraceID,
271272
})
272273

273274
assert.NoError(t, err)
@@ -282,11 +283,10 @@ func TestArchiveTraceNotFoundGRPC(t *testing.T) {
282283
Return(nil, spanstore.ErrTraceNotFound).Once()
283284

284285
_, err := client.ArchiveTrace(context.Background(), &api_v2.ArchiveTraceRequest{
285-
TraceID: mockTraceIDgrpc,
286+
TraceID: mockTraceID,
286287
})
287288

288-
assert.Errorf(t, err, spanstore.ErrTraceNotFound.Error())
289-
289+
assertGRPCError(t, err, codes.NotFound, "trace not found")
290290
})
291291
}
292292

@@ -299,12 +299,10 @@ func TestArchiveTraceFailureGRPC(t *testing.T) {
299299
Return(errStorageGRPC).Times(2)
300300

301301
_, err := client.ArchiveTrace(context.Background(), &api_v2.ArchiveTraceRequest{
302-
TraceID: mockTraceIDgrpc,
302+
TraceID: mockTraceID,
303303
})
304304

305-
storageErr := status.Error(2, "[Storage error, Storage error]")
306-
assert.EqualError(t, err, storageErr.Error())
307-
305+
assertGRPCError(t, err, codes.Internal, "failed to archive trace")
308306
})
309307
}
310308

@@ -408,7 +406,7 @@ func TestGetServicesFailureGRPC(t *testing.T) {
408406
server.spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(nil, errStorageGRPC).Once()
409407
_, err := client.GetServices(context.Background(), &api_v2.GetServicesRequest{})
410408

411-
assert.EqualError(t, err, errStatusStorageGRPC.Error())
409+
assertGRPCError(t, err, codes.Internal, "failed to fetch services")
412410
})
413411
}
414412

@@ -450,7 +448,7 @@ func TestGetOperationsFailureGRPC(t *testing.T) {
450448
Service: "trifle",
451449
})
452450

453-
assert.EqualError(t, err, errStatusStorageGRPC.Error())
451+
assertGRPCError(t, err, codes.Internal, "failed to fetch operations")
454452
})
455453
}
456454

@@ -482,7 +480,7 @@ func TestGetDependenciesFailureGRPC(t *testing.T) {
482480
EndTime: endTs,
483481
})
484482

485-
assert.EqualError(t, err, errStatusStorageGRPC.Error())
483+
assertGRPCError(t, err, codes.Internal, "failed to fetch dependencies")
486484
})
487485
}
488486

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ require (
8484
golang.org/x/lint v0.0.0-20200130185559-910be7a94367
8585
golang.org/x/net v0.0.0-20200202094626-16171245cfb2
8686
golang.org/x/sys v0.0.0-20200217220822-9197077df867
87-
golang.org/x/tools v0.0.0-20200227222343-706bc42d1f0d
87+
golang.org/x/tools v0.0.0-20200227222343-706bc42d1f0d // indirect
8888
google.golang.org/genproto v0.0.0-20200218151345-dad8c97a84f5 // indirect
8989
google.golang.org/grpc v1.27.1
9090
gopkg.in/ini.v1 v1.52.0 // indirect

0 commit comments

Comments
 (0)