From ea1dcac73c6d304f1896a0c637daf7b86bfd54c8 Mon Sep 17 00:00:00 2001 From: albertteoh Date: Wed, 16 Jun 2021 10:59:49 +1000 Subject: [PATCH] Address review comments Signed-off-by: albertteoh --- cmd/query/app/grpc_handler.go | 23 ++++++---------------- cmd/query/app/grpc_handler_test.go | 31 ++++++++++++------------------ cmd/query/app/server.go | 9 ++++++++- cmd/query/app/util.go | 11 ----------- 4 files changed, 26 insertions(+), 48 deletions(-) diff --git a/cmd/query/app/grpc_handler.go b/cmd/query/app/grpc_handler.go index 454a11ecbf9..34d510a0b21 100644 --- a/cmd/query/app/grpc_handler.go +++ b/cmd/query/app/grpc_handler.go @@ -17,6 +17,7 @@ package app import ( "context" "errors" + "time" "github.com/opentracing/opentracing-go" "go.uber.org/zap" @@ -51,19 +52,7 @@ type GRPCHandler struct { metricsQueryService querysvc.MetricsQueryService logger *zap.Logger tracer opentracing.Tracer - clock clock -} - -// NewGRPCHandler returns a GRPCHandler -func NewGRPCHandler(queryService *querysvc.QueryService, metricsQueryService querysvc.MetricsQueryService, logger *zap.Logger, tracer opentracing.Tracer, clock clock) *GRPCHandler { - gH := &GRPCHandler{ - queryService: queryService, - metricsQueryService: metricsQueryService, - logger: logger, - tracer: tracer, - clock: clock, - } - return gH + nowFn func() time.Time } // GetTrace is the gRPC handler to fetch traces based on trace-id. @@ -260,10 +249,10 @@ func (g *GRPCHandler) handleErr(msg string, err error) error { g.logger.Error(msg, zap.Error(err)) // Avoid wrapping "expected" errors with an "Internal Server" error. - switch { - case errors.Is(err, disabled.ErrDisabled): + if errors.Is(err, disabled.ErrDisabled) { return errGRPCMetricsQueryDisabled - case errors.Is(err, errMissingServiceNames), errors.Is(err, errMissingQuantile): + } + if _, ok := status.FromError(err); ok { return err } @@ -281,7 +270,7 @@ func (g *GRPCHandler) newBaseQueryParameters(r *metrics.MetricsQueryBaseRequest) bqp.ServiceNames = r.ServiceNames // Initialize nullable params with defaults. - defaultEndTime := g.clock.Now() + defaultEndTime := g.nowFn() bqp.EndTime = &defaultEndTime bqp.Lookback = &defaultMetricsQueryLookbackDuration bqp.RatePer = &defaultMetricsQueryRateDuration diff --git a/cmd/query/app/grpc_handler_test.go b/cmd/query/app/grpc_handler_test.go index 5bc3c98a44f..be27a6b0807 100644 --- a/cmd/query/app/grpc_handler_test.go +++ b/cmd/query/app/grpc_handler_test.go @@ -142,16 +142,18 @@ type grpcClient struct { conn *grpc.ClientConn } -type simulatedClock struct{} - -func (s simulatedClock) Now() time.Time { - return now -} - -func newGRPCServer(t *testing.T, q *querysvc.QueryService, mq querysvc.MetricsQueryService, logger *zap.Logger, tracer opentracing.Tracer, clock clock) (*grpc.Server, net.Addr) { +func newGRPCServer(t *testing.T, q *querysvc.QueryService, mq querysvc.MetricsQueryService, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, net.Addr) { lis, _ := net.Listen("tcp", ":0") grpcServer := grpc.NewServer() - grpcHandler := NewGRPCHandler(q, mq, logger, tracer, clock) + grpcHandler := &GRPCHandler{ + queryService: q, + metricsQueryService: mq, + logger: logger, + tracer: tracer, + nowFn: func() time.Time { + return now + }, + } api_v2.RegisterQueryServiceServer(grpcServer, grpcHandler) metrics.RegisterMetricsQueryServiceServer(grpcServer, grpcHandler) @@ -181,8 +183,6 @@ type testOption func(*testQueryService) type testQueryService struct { // metricsQueryService is used when creating a new GRPCHandler. metricsQueryService querysvc.MetricsQueryService - - clock clock } func withMetricsQuery() testOption { @@ -192,12 +192,6 @@ func withMetricsQuery() testOption { } } -func withClock(clock clock) testOption { - return func(ts *testQueryService) { - ts.clock = clock - } -} - func initializeTestServerGRPCWithOptions(t *testing.T, options ...testOption) *grpcServer { archiveSpanReader := &spanstoremocks.Reader{} archiveSpanWriter := &spanstoremocks.Writer{} @@ -216,7 +210,6 @@ func initializeTestServerGRPCWithOptions(t *testing.T, options ...testOption) *g tqs := &testQueryService{ // Disable metrics query by default. metricsQueryService: disabledReader, - clock: realClock{}, } for _, opt := range options { opt(tqs) @@ -225,7 +218,7 @@ func initializeTestServerGRPCWithOptions(t *testing.T, options ...testOption) *g logger := zap.NewNop() tracer := opentracing.NoopTracer{} - server, addr := newGRPCServer(t, q, tqs.metricsQueryService, logger, tracer, tqs.clock) + server, addr := newGRPCServer(t, q, tqs.metricsQueryService, logger, tracer) return &grpcServer{ server: server, @@ -672,7 +665,7 @@ func TestGetMetricsUseDefaultParamsGRPC(t *testing.T) { res, err := client.GetCallRates(context.Background(), request) require.NoError(t, err) assert.Equal(t, expectedMetrics, &res.Metrics) - }, withMetricsQuery(), withClock(simulatedClock{})) + }, withMetricsQuery()) } func TestGetMetricsOverrideDefaultParamsGRPC(t *testing.T) { diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index ce2f0455304..be9a562d7b7 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "strings" + "time" "github.com/gorilla/handlers" "github.com/opentracing/opentracing-go" @@ -111,7 +112,13 @@ func createGRPCServer(querySvc *querysvc.QueryService, metricsQuerySvc querysvc. server := grpc.NewServer(grpcOpts...) - handler := NewGRPCHandler(querySvc, metricsQuerySvc, logger, tracer, realClock{}) + handler := &GRPCHandler{ + queryService: querySvc, + metricsQueryService: metricsQuerySvc, + logger: logger, + tracer: tracer, + nowFn: time.Now, + } api_v2.RegisterQueryServiceServer(server, handler) metrics.RegisterMetricsQueryServiceServer(server, handler) diff --git a/cmd/query/app/util.go b/cmd/query/app/util.go index 00f7fab8dd1..29e1bc7f7b8 100644 --- a/cmd/query/app/util.go +++ b/cmd/query/app/util.go @@ -15,20 +15,9 @@ package app import ( - "time" - "github.com/jaegertracing/jaeger/storage/spanstore" ) -// clock is a wrapper around the time package to allow mocking of the current time. -type clock interface { - Now() time.Time -} - -type realClock struct{} - -func (realClock) Now() time.Time { return time.Now() } - func getUniqueOperationNames(operations []spanstore.Operation) []string { // only return unique operation names set := make(map[string]struct{})