Address review comments

Signed-off-by: albertteoh <albert.teoh@logz.io>

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

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) {

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)
 

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{})