From da636e3f791963df78cc19856fbb8759e51e4237 Mon Sep 17 00:00:00 2001 From: albertteoh Date: Sun, 13 Jun 2021 15:26:41 +1000 Subject: [PATCH 1/2] Convert MetricsQueryService to interface Signed-off-by: albertteoh --- cmd/all-in-one/main.go | 3 +- .../app/querysvc/metrics_query_service.go | 39 +------- .../querysvc/metrics_query_service_test.go | 96 ------------------- cmd/query/app/server.go | 6 +- cmd/query/app/server_test.go | 27 +++--- cmd/query/main.go | 4 +- 6 files changed, 20 insertions(+), 155 deletions(-) delete mode 100644 cmd/query/app/querysvc/metrics_query_service_test.go diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index 24e12ddfc1c..d6116c89ad9 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -249,8 +249,7 @@ func startQuery( ) *queryApp.Server { spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, baseFactory.Namespace(metrics.NSOptions{Name: "query"})) qs := querysvc.NewQueryService(spanReader, depReader, *queryOpts) - mqs := querysvc.NewMetricsQueryService(metricsReader) - server, err := queryApp.NewServer(svc.Logger, qs, mqs, qOpts, opentracing.GlobalTracer()) + server, err := queryApp.NewServer(svc.Logger, qs, metricsReader, qOpts, opentracing.GlobalTracer()) if err != nil { svc.Logger.Fatal("Could not start jaeger-query service", zap.Error(err)) } diff --git a/cmd/query/app/querysvc/metrics_query_service.go b/cmd/query/app/querysvc/metrics_query_service.go index 5596901bb5b..2fafac589fe 100644 --- a/cmd/query/app/querysvc/metrics_query_service.go +++ b/cmd/query/app/querysvc/metrics_query_service.go @@ -14,42 +14,9 @@ package querysvc -import ( - "context" - "time" - - "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" - "github.com/jaegertracing/jaeger/storage/metricsstore" -) +import "github.com/jaegertracing/jaeger/storage/metricsstore" // MetricsQueryService provides a means of querying R.E.D metrics from an underlying metrics store. -type MetricsQueryService struct { - metricsReader metricsstore.Reader -} - -// NewMetricsQueryService returns a new MetricsQueryService. -func NewMetricsQueryService(reader metricsstore.Reader) *MetricsQueryService { - return &MetricsQueryService{ - metricsReader: reader, - } -} - -// GetLatencies is the queryService implementation of metricsstore.Reader. -func (mqs MetricsQueryService) GetLatencies(ctx context.Context, params *metricsstore.LatenciesQueryParameters) (*metrics.MetricFamily, error) { - return mqs.metricsReader.GetLatencies(ctx, params) -} - -// GetCallRates is the queryService implementation of metricsstore.Reader. -func (mqs MetricsQueryService) GetCallRates(ctx context.Context, params *metricsstore.CallRateQueryParameters) (*metrics.MetricFamily, error) { - return mqs.metricsReader.GetCallRates(ctx, params) -} - -// GetErrorRates is the queryService implementation of metricsstore.Reader. -func (mqs MetricsQueryService) GetErrorRates(ctx context.Context, params *metricsstore.ErrorRateQueryParameters) (*metrics.MetricFamily, error) { - return mqs.metricsReader.GetErrorRates(ctx, params) -} - -// GetMinStepDuration is the queryService implementation of metricsstore.Reader. -func (mqs MetricsQueryService) GetMinStepDuration(ctx context.Context, params *metricsstore.MinStepDurationQueryParameters) (time.Duration, error) { - return mqs.metricsReader.GetMinStepDuration(ctx, params) +type MetricsQueryService interface { + metricsstore.Reader } diff --git a/cmd/query/app/querysvc/metrics_query_service_test.go b/cmd/query/app/querysvc/metrics_query_service_test.go deleted file mode 100644 index 5783f1e54a1..00000000000 --- a/cmd/query/app/querysvc/metrics_query_service_test.go +++ /dev/null @@ -1,96 +0,0 @@ -// Copyright (c) 2021 The Jaeger Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package querysvc - -import ( - "context" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - - protometrics "github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics" - "github.com/jaegertracing/jaeger/storage/metricsstore" - metricsmocks "github.com/jaegertracing/jaeger/storage/metricsstore/mocks" -) - -type testMetricsQueryService struct { - queryService *MetricsQueryService - metricsReader *metricsmocks.Reader -} - -func initializeTestMetricsQueryService() *testMetricsQueryService { - metricsReader := &metricsmocks.Reader{} - tqs := testMetricsQueryService{ - metricsReader: metricsReader, - } - tqs.queryService = NewMetricsQueryService(metricsReader) - return &tqs -} - -// Test QueryService.GetLatencies() -func TestGetLatencies(t *testing.T) { - tqs := initializeTestMetricsQueryService() - expectedLatencies := &protometrics.MetricFamily{ - Name: "latencies", - Metrics: []*protometrics.Metric{}, - } - qParams := &metricsstore.LatenciesQueryParameters{} - tqs.metricsReader.On("GetLatencies", mock.Anything, qParams).Return(expectedLatencies, nil).Times(1) - - actualLatencies, err := tqs.queryService.GetLatencies(context.Background(), qParams) - assert.NoError(t, err) - assert.Equal(t, expectedLatencies, actualLatencies) -} - -// Test QueryService.GetCallRates() -func TestGetCallRates(t *testing.T) { - tqs := initializeTestMetricsQueryService() - expectedCallRates := &protometrics.MetricFamily{ - Name: "call rates", - Metrics: []*protometrics.Metric{}, - } - qParams := &metricsstore.CallRateQueryParameters{} - tqs.metricsReader.On("GetCallRates", mock.Anything, qParams).Return(expectedCallRates, nil).Times(1) - - actualCallRates, err := tqs.queryService.GetCallRates(context.Background(), qParams) - assert.NoError(t, err) - assert.Equal(t, expectedCallRates, actualCallRates) -} - -// Test QueryService.GetErrorRates() -func TestGetErrorRates(t *testing.T) { - tqs := initializeTestMetricsQueryService() - expectedErrorRates := &protometrics.MetricFamily{} - qParams := &metricsstore.ErrorRateQueryParameters{} - tqs.metricsReader.On("GetErrorRates", mock.Anything, qParams).Return(expectedErrorRates, nil).Times(1) - - actualErrorRates, err := tqs.queryService.GetErrorRates(context.Background(), qParams) - assert.NoError(t, err) - assert.Equal(t, expectedErrorRates, actualErrorRates) -} - -// Test QueryService.GetMinStepDurations() -func TestGetMinStepDurations(t *testing.T) { - tqs := initializeTestMetricsQueryService() - expectedMinStep := time.Second - qParams := &metricsstore.MinStepDurationQueryParameters{} - tqs.metricsReader.On("GetMinStepDuration", mock.Anything, qParams).Return(expectedMinStep, nil).Times(1) - - actualMinStep, err := tqs.queryService.GetMinStepDuration(context.Background(), qParams) - assert.NoError(t, err) - assert.Equal(t, expectedMinStep, actualMinStep) -} diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 606f8b12766..5918dc7ca88 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -52,7 +52,7 @@ type Server struct { } // NewServer creates and initializes Server -func NewServer(logger *zap.Logger, querySvc *querysvc.QueryService, metricsQuerySvc *querysvc.MetricsQueryService, options *QueryOptions, tracer opentracing.Tracer) (*Server, error) { +func NewServer(logger *zap.Logger, querySvc *querysvc.QueryService, metricsQuerySvc querysvc.MetricsQueryService, options *QueryOptions, tracer opentracing.Tracer) (*Server, error) { _, httpPort, err := net.SplitHostPort(options.HTTPHostPort) if err != nil { @@ -94,7 +94,7 @@ func (s Server) HealthCheckStatus() chan healthcheck.Status { return s.unavailableChannel } -func createGRPCServer(querySvc *querysvc.QueryService, metricsQuerySvc *querysvc.MetricsQueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) { +func createGRPCServer(querySvc *querysvc.QueryService, metricsQuerySvc querysvc.MetricsQueryService, options *QueryOptions, logger *zap.Logger, tracer opentracing.Tracer) (*grpc.Server, error) { var grpcOpts []grpc.ServerOption if options.TLSGRPC.Enabled { @@ -118,7 +118,7 @@ func createGRPCServer(querySvc *querysvc.QueryService, metricsQuerySvc *querysvc return server, nil } -func createHTTPServer(querySvc *querysvc.QueryService, metricsQuerySvc *querysvc.MetricsQueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) (*http.Server, error) { +func createHTTPServer(querySvc *querysvc.QueryService, metricsQuerySvc querysvc.MetricsQueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) (*http.Server, error) { // TODO: Add HandlerOptions.MetricsQueryService apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(logger), diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 3ade4591b79..8ecc85d552c 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -64,7 +64,7 @@ func TestCreateTLSServerSinglePortError(t *testing.T) { ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", } - _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, querysvc.NewMetricsQueryService(nil), + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8080", TLSGRPC: tlsCfg, TLSHTTP: tlsCfg}, opentracing.NoopTracer{}) assert.NotNil(t, err) } @@ -77,7 +77,7 @@ func TestCreateTLSGrpcServerError(t *testing.T) { ClientCAPath: "invalid/path", } - _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, querysvc.NewMetricsQueryService(nil), + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8081", TLSGRPC: tlsCfg}, opentracing.NoopTracer{}) assert.NotNil(t, err) } @@ -90,7 +90,7 @@ func TestCreateTLSHttpServerError(t *testing.T) { ClientCAPath: "invalid/path", } - _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, querysvc.NewMetricsQueryService(nil), + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8081", TLSHTTP: tlsCfg}, opentracing.NoopTracer{}) assert.NotNil(t, err) } @@ -331,8 +331,7 @@ func TestServerHTTPTLS(t *testing.T) { spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - metricsQuerySvc := querysvc.NewMetricsQueryService(nil) - server, err := NewServer(flagsSvc.Logger, querySvc, metricsQuerySvc, + server, err := NewServer(flagsSvc.Logger, querySvc, nil, serverOptions, opentracing.NoopTracer{}) assert.Nil(t, err) @@ -492,8 +491,7 @@ func TestServerGRPCTLS(t *testing.T) { spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - metricsQuerySvc := querysvc.NewMetricsQueryService(nil) - server, err := NewServer(flagsSvc.Logger, querySvc, metricsQuerySvc, + server, err := NewServer(flagsSvc.Logger, querySvc, nil, serverOptions, opentracing.NoopTracer{}) assert.Nil(t, err) @@ -547,12 +545,12 @@ func TestServerGRPCTLS(t *testing.T) { } func TestServerBadHostPort(t *testing.T) { - _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, querysvc.NewMetricsQueryService(nil), + _, err := NewServer(zap.NewNop(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: "8080", GRPCHostPort: "127.0.0.1:8081", BearerTokenPropagation: true}, opentracing.NoopTracer{}) assert.NotNil(t, err) - _, err = NewServer(zap.NewNop(), &querysvc.QueryService{}, querysvc.NewMetricsQueryService(nil), + _, err = NewServer(zap.NewNop(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: "127.0.0.1:8081", GRPCHostPort: "9123", BearerTokenPropagation: true}, opentracing.NoopTracer{}) @@ -578,7 +576,7 @@ func TestServerInUseHostPort(t *testing.T) { server, err := NewServer( zap.NewNop(), &querysvc.QueryService{}, - querysvc.NewMetricsQueryService(nil), + nil, &QueryOptions{ HTTPHostPort: tc.httpHostPort, GRPCHostPort: tc.grpcHostPort, @@ -611,8 +609,7 @@ func TestServerSinglePort(t *testing.T) { spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - metricsQuerySvc := querysvc.NewMetricsQueryService(nil) - server, err := NewServer(flagsSvc.Logger, querySvc, metricsQuerySvc, + server, err := NewServer(flagsSvc.Logger, querySvc, nil, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort, BearerTokenPropagation: true}, opentracing.NoopTracer{}) assert.Nil(t, err) @@ -661,10 +658,9 @@ func TestServerGracefulExit(t *testing.T) { hostPort := ports.PortToHostPort(ports.QueryAdminHTTP) querySvc := &querysvc.QueryService{} - metricsQuerySvc := querysvc.NewMetricsQueryService(nil) tracer := opentracing.NoopTracer{} - server, err := NewServer(flagsSvc.Logger, querySvc, metricsQuerySvc, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort}, tracer) + server, err := NewServer(flagsSvc.Logger, querySvc, nil, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort}, tracer) assert.Nil(t, err) assert.NoError(t, server.Start()) go func() { @@ -690,9 +686,8 @@ func TestServerHandlesPortZero(t *testing.T) { flagsSvc.Logger = zap.New(zapCore) querySvc := &querysvc.QueryService{} - metricsQuerySvc := querysvc.NewMetricsQueryService(nil) tracer := opentracing.NoopTracer{} - server, err := NewServer(flagsSvc.Logger, querySvc, metricsQuerySvc, &QueryOptions{GRPCHostPort: ":0", HTTPHostPort: ":0"}, tracer) + server, err := NewServer(flagsSvc.Logger, querySvc, nil, &QueryOptions{GRPCHostPort: ":0", HTTPHostPort: ":0"}, tracer) assert.Nil(t, err) assert.NoError(t, server.Start()) server.Close() diff --git a/cmd/query/main.go b/cmd/query/main.go index eec8879b8c2..a0c1eda3fd1 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -163,7 +163,7 @@ func main() { } } -func createMetricsQueryService(factory *metricsPlugin.Factory, v *viper.Viper, logger *zap.Logger) (*querysvc.MetricsQueryService, error) { +func createMetricsQueryService(factory *metricsPlugin.Factory, v *viper.Viper, logger *zap.Logger) (querysvc.MetricsQueryService, error) { if err := factory.Initialize(logger); err != nil { return nil, fmt.Errorf("failed to init metrics factory: %w", err) } @@ -174,5 +174,5 @@ func createMetricsQueryService(factory *metricsPlugin.Factory, v *viper.Viper, l if err != nil { return nil, fmt.Errorf("failed to create metrics reader: %w", err) } - return querysvc.NewMetricsQueryService(metricsReader), nil + return metricsReader, nil } From c865d7da57029d45e6702729535b684993062e77 Mon Sep 17 00:00:00 2001 From: albertteoh Date: Sun, 13 Jun 2021 15:47:14 +1000 Subject: [PATCH 2/2] Simplify and make consistent Signed-off-by: albertteoh --- cmd/all-in-one/main.go | 11 +++++------ cmd/query/main.go | 8 ++------ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index d6116c89ad9..8571f829f12 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -50,7 +50,6 @@ import ( "github.com/jaegertracing/jaeger/plugin/storage" "github.com/jaegertracing/jaeger/ports" "github.com/jaegertracing/jaeger/storage/dependencystore" - "github.com/jaegertracing/jaeger/storage/metricsstore" "github.com/jaegertracing/jaeger/storage/spanstore" storageMetrics "github.com/jaegertracing/jaeger/storage/spanstore/metrics" ) @@ -116,7 +115,7 @@ by default uses only in-memory database.`, logger.Fatal("Failed to create dependency reader", zap.Error(err)) } - metricsReader, err := createMetricsReader(metricsReaderFactory, v, logger) + metricsQueryService, err := createMetricsQueryService(metricsReaderFactory, v, logger) if err != nil { logger.Fatal("Failed to create metrics reader", zap.Error(err)) } @@ -171,7 +170,7 @@ by default uses only in-memory database.`, // query querySrv := startQuery( svc, qOpts, qOpts.BuildQueryServiceOptions(storageFactory, logger), - spanReader, dependencyReader, metricsReader, + spanReader, dependencyReader, metricsQueryService, metricsFactory, ) @@ -244,12 +243,12 @@ func startQuery( queryOpts *querysvc.QueryServiceOptions, spanReader spanstore.Reader, depReader dependencystore.Reader, - metricsReader metricsstore.Reader, + metricsQueryService querysvc.MetricsQueryService, baseFactory metrics.Factory, ) *queryApp.Server { spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, baseFactory.Namespace(metrics.NSOptions{Name: "query"})) qs := querysvc.NewQueryService(spanReader, depReader, *queryOpts) - server, err := queryApp.NewServer(svc.Logger, qs, metricsReader, qOpts, opentracing.GlobalTracer()) + server, err := queryApp.NewServer(svc.Logger, qs, metricsQueryService, qOpts, opentracing.GlobalTracer()) if err != nil { svc.Logger.Fatal("Could not start jaeger-query service", zap.Error(err)) } @@ -288,7 +287,7 @@ func initTracer(metricsFactory metrics.Factory, logger *zap.Logger) io.Closer { return closer } -func createMetricsReader(factory *metricsPlugin.Factory, v *viper.Viper, logger *zap.Logger) (metricsstore.Reader, error) { +func createMetricsQueryService(factory *metricsPlugin.Factory, v *viper.Viper, logger *zap.Logger) (querysvc.MetricsQueryService, error) { if err := factory.Initialize(logger); err != nil { return nil, fmt.Errorf("failed to init metrics reader factory: %w", err) } diff --git a/cmd/query/main.go b/cmd/query/main.go index a0c1eda3fd1..cc012180597 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -165,14 +165,10 @@ func main() { func createMetricsQueryService(factory *metricsPlugin.Factory, v *viper.Viper, logger *zap.Logger) (querysvc.MetricsQueryService, error) { if err := factory.Initialize(logger); err != nil { - return nil, fmt.Errorf("failed to init metrics factory: %w", err) + return nil, fmt.Errorf("failed to init metrics reader factory: %w", err) } // Ensure default parameter values are loaded correctly. factory.InitFromViper(v) - metricsReader, err := factory.CreateMetricsReader() - if err != nil { - return nil, fmt.Errorf("failed to create metrics reader: %w", err) - } - return metricsReader, nil + return factory.CreateMetricsReader() }