Skip to content
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

Return appropriate gRPC errors/codes to indicate request status #2132

Merged
merged 4 commits into from
Mar 18, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 27 additions & 21 deletions cmd/query/app/grpc_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,22 @@ import (

"github.com/opentracing/opentracing-go"
"go.uber.org/zap"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
"github.com/jaegertracing/jaeger/model"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/storage/spanstore"
)

const maxSpanCountInChunk = 10
const (
maxSpanCountInChunk = 10

// GRPCHandler implements the GRPC endpoint of the query service.
msgTraceNotFound = "trace not found"
)

// GRPCHandler implements the gRPC endpoint of the query service.
type GRPCHandler struct {
queryService *querysvc.QueryService
logger *zap.Logger
Expand All @@ -46,36 +52,36 @@ func NewGRPCHandler(queryService *querysvc.QueryService, logger *zap.Logger, tra
return gH
}

// GetTrace is the GRPC handler to fetch traces based on trace-id.
// GetTrace is the gRPC handler to fetch traces based on trace-id.
func (g *GRPCHandler) GetTrace(r *api_v2.GetTraceRequest, stream api_v2.QueryService_GetTraceServer) error {
trace, err := g.queryService.GetTrace(stream.Context(), r.TraceID)
if err == spanstore.ErrTraceNotFound {
g.logger.Error("trace not found", zap.Error(err))
return err
g.logger.Error(msgTraceNotFound, zap.Error(err))
return status.Errorf(codes.NotFound, "%s: %v", msgTraceNotFound, err)
}
if err != nil {
g.logger.Error("Could not fetch spans from backend", zap.Error(err))
return err
g.logger.Error("failed to fetch spans from the backend", zap.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Logs may be capitalized and with periods while error strings need not (because they are pulled into stack traces)

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, but I felt it's simpler to keep strings consistent between errors and logs

return status.Errorf(codes.Internal, "failed to fetch spans from the backend: %v", err)
}
return g.sendSpanChunks(trace.Spans, stream.Send)
}

// ArchiveTrace is the GRPC handler to archive traces.
// ArchiveTrace is the gRPC handler to archive traces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is everything consistently using this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing as I see them. Not important, but less complaints from IDE.

func (g *GRPCHandler) ArchiveTrace(ctx context.Context, r *api_v2.ArchiveTraceRequest) (*api_v2.ArchiveTraceResponse, error) {
err := g.queryService.ArchiveTrace(ctx, r.TraceID)
if err == spanstore.ErrTraceNotFound {
g.logger.Error("trace not found", zap.Error(err))
return nil, err
return nil, status.Errorf(codes.NotFound, "%s: %v", msgTraceNotFound, err)
}
if err != nil {
g.logger.Error("Could not fetch spans from backend", zap.Error(err))
return nil, err
g.logger.Error("failed to archive trace", zap.Error(err))
return nil, status.Errorf(codes.Internal, "failed to archive trace: %v", err)
}

return &api_v2.ArchiveTraceResponse{}, nil
}

// FindTraces is the GRPC handler to fetch traces based on TraceQueryParameters.
// FindTraces is the gRPC handler to fetch traces based on TraceQueryParameters.
func (g *GRPCHandler) FindTraces(r *api_v2.FindTracesRequest, stream api_v2.QueryService_FindTracesServer) error {
query := r.GetQuery()
queryParams := spanstore.TraceQueryParameters{
Expand All @@ -90,8 +96,8 @@ func (g *GRPCHandler) FindTraces(r *api_v2.FindTracesRequest, stream api_v2.Quer
}
traces, err := g.queryService.FindTraces(stream.Context(), &queryParams)
if err != nil {
g.logger.Error("Error fetching traces", zap.Error(err))
return err
g.logger.Error("failed when searching for traces", zap.Error(err))
return status.Errorf(codes.Internal, "failed when searching for traces: %v", err)
}
for _, trace := range traces {
if err := g.sendSpanChunks(trace.Spans, stream.Send); err != nil {
Expand All @@ -116,12 +122,12 @@ func (g *GRPCHandler) sendSpanChunks(spans []*model.Span, sendFn func(*api_v2.Sp
return nil
}

// GetServices is the GRPC handler to fetch services.
// GetServices is the gRPC handler to fetch services.
func (g *GRPCHandler) GetServices(ctx context.Context, r *api_v2.GetServicesRequest) (*api_v2.GetServicesResponse, error) {
services, err := g.queryService.GetServices(ctx)
if err != nil {
g.logger.Error("Error fetching services", zap.Error(err))
return nil, err
g.logger.Error("failed to fetch services", zap.Error(err))
return nil, status.Errorf(codes.Internal, "failed to fetch services: %v", err)
}

return &api_v2.GetServicesResponse{Services: services}, nil
Expand All @@ -137,8 +143,8 @@ func (g *GRPCHandler) GetOperations(
SpanKind: r.SpanKind,
})
if err != nil {
g.logger.Error("Error fetching operations", zap.Error(err))
return nil, err
g.logger.Error("failed to fetch operations", zap.Error(err))
return nil, status.Errorf(codes.Internal, "failed to fetch operations: %v", err)
}

result := make([]*api_v2.Operation, len(operations))
Expand All @@ -161,8 +167,8 @@ func (g *GRPCHandler) GetDependencies(ctx context.Context, r *api_v2.GetDependen
endTime := r.EndTime
dependencies, err := g.queryService.GetDependencies(startTime, endTime.Sub(startTime))
if err != nil {
g.logger.Error("Error fetching dependencies", zap.Error(err))
return nil, err
g.logger.Error("failed to fetch dependencies", zap.Error(err))
return nil, status.Errorf(codes.Internal, "failed to fetch dependencies: %v", err)
}

return &api_v2.GetDependenciesResponse{Dependencies: dependencies}, nil
Expand Down
26 changes: 14 additions & 12 deletions cmd/query/app/grpc_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
Expand All @@ -40,9 +41,7 @@ import (
)

var (
errStorageMsgGRPC = "Storage error"
errStorageGRPC = errors.New(errStorageMsgGRPC)
errStatusStorageGRPC = status.Error(2, errStorageMsgGRPC)
errStorageGRPC = errors.New("Storage error")

mockTraceIDgrpc = model.NewTraceID(0, 123456)
mockTraceGRPC = &model.Trace{
Expand Down Expand Up @@ -218,6 +217,13 @@ func TestGetTraceSuccessGRPC(t *testing.T) {
})
}

func assertGRPCError(t *testing.T, err error, code codes.Code, msg string) {
s, ok := status.FromError(err)
require.True(t, ok, "expecting gRPC status")
assert.Equal(t, code, s.Code())
assert.Contains(t, s.Message(), msg)
}

func TestGetTraceDBFailureGRPC(t *testing.T) {
withServerAndClient(t, func(server *grpcServer, client *grpcClient) {

Expand All @@ -230,10 +236,8 @@ func TestGetTraceDBFailureGRPC(t *testing.T) {
assert.NoError(t, err)

spanResChunk, err := res.Recv()

assert.EqualError(t, err, errStatusStorageGRPC.Error())
assertGRPCError(t, err, codes.Internal, "failed to fetch spans from the backend")
assert.Nil(t, spanResChunk)

})

}
Expand Down Expand Up @@ -302,9 +306,7 @@ func TestArchiveTraceFailureGRPC(t *testing.T) {
TraceID: mockTraceIDgrpc,
})

storageErr := status.Error(2, "[Storage error, Storage error]")
assert.EqualError(t, err, storageErr.Error())

assertGRPCError(t, err, codes.Internal, "failed to archive trace")
})
}

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

assert.EqualError(t, err, errStatusStorageGRPC.Error())
assertGRPCError(t, err, codes.Internal, "failed to fetch services")
})
}

Expand Down Expand Up @@ -450,7 +452,7 @@ func TestGetOperationsFailureGRPC(t *testing.T) {
Service: "trifle",
})

assert.EqualError(t, err, errStatusStorageGRPC.Error())
assertGRPCError(t, err, codes.Internal, "failed to fetch operations")
})
}

Expand Down Expand Up @@ -482,7 +484,7 @@ func TestGetDependenciesFailureGRPC(t *testing.T) {
EndTime: endTs,
})

assert.EqualError(t, err, errStatusStorageGRPC.Error())
assertGRPCError(t, err, codes.Internal, "failed to fetch dependencies")
})
}

Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ require (
golang.org/x/lint v0.0.0-20200130185559-910be7a94367
golang.org/x/net v0.0.0-20200202094626-16171245cfb2
golang.org/x/sys v0.0.0-20200217220822-9197077df867
golang.org/x/tools v0.0.0-20200227222343-706bc42d1f0d
golang.org/x/tools v0.0.0-20200227222343-706bc42d1f0d // indirect
Copy link
Member Author

Choose a reason for hiding this comment

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

I just ran go mod tidy

google.golang.org/genproto v0.0.0-20200218151345-dad8c97a84f5 // indirect
google.golang.org/grpc v1.27.1
gopkg.in/ini.v1 v1.52.0 // indirect
Expand Down