From f11f5f3b2164882e7d52adf1cc372eb507be61d9 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 19:21:40 -0400 Subject: [PATCH 01/21] fix Signed-off-by: Yuri Shkuro --- cmd/agent/app/reporter/grpc/builder.go | 3 +- cmd/agent/app/reporter/grpc/builder_test.go | 110 ++++++-------------- 2 files changed, 34 insertions(+), 79 deletions(-) diff --git a/cmd/agent/app/reporter/grpc/builder.go b/cmd/agent/app/reporter/grpc/builder.go index 5853ef7912b..9290107dd3a 100644 --- a/cmd/agent/app/reporter/grpc/builder.go +++ b/cmd/agent/app/reporter/grpc/builder.go @@ -102,8 +102,7 @@ func (b *ConnBuilder) CreateConnection(ctx context.Context, logger *zap.Logger, dialOptions = append(dialOptions, grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor(grpc_retry.WithMax(b.MaxRetry)))) dialOptions = append(dialOptions, b.AdditionalDialOptions...) - // TODO: Need to replace grpc.Dial with grpc.NewClient and pass test - conn, err := grpc.Dial(dialTarget, dialOptions...) + conn, err := grpc.NewClient(dialTarget, dialOptions...) if err != nil { return nil, err } diff --git a/cmd/agent/app/reporter/grpc/builder_test.go b/cmd/agent/app/reporter/grpc/builder_test.go index 1206cb6ab78..d241cd350e4 100644 --- a/cmd/agent/app/reporter/grpc/builder_test.go +++ b/cmd/agent/app/reporter/grpc/builder_test.go @@ -24,7 +24,6 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/zap" "google.golang.org/grpc" - "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" yaml "gopkg.in/yaml.v2" @@ -69,78 +68,52 @@ func TestBuilderFromConfig(t *testing.T) { func TestBuilderWithCollectors(t *testing.T) { spanHandler1 := &mockSpanHandler{} - s1, addr1 := initializeGRPCTestServer(t, func(s *grpc.Server) { + s1, _ := initializeGRPCTestServer(t, func(s *grpc.Server) { api_v2.RegisterCollectorServiceServer(s, spanHandler1) }) defer s1.Stop() tests := []struct { - target string - name string - hostPorts []string - checkSuffixOnly bool - notifier discovery.Notifier - discoverer discovery.Discoverer - expectedError string - checkConnectionState bool - expectedState string + target string + name string + hostPorts []string + checkSuffixOnly bool + notifier discovery.Notifier + discoverer discovery.Discoverer + expectedError string }{ { - target: "///round_robin", - name: "with roundrobin schema", - hostPorts: []string{"127.0.0.1:9876", "127.0.0.1:9877", "127.0.0.1:9878"}, - checkSuffixOnly: true, - notifier: nil, - discoverer: nil, - checkConnectionState: false, + target: "///round_robin", + name: "with roundrobin schema", + hostPorts: []string{"127.0.0.1:9876", "127.0.0.1:9877", "127.0.0.1:9878"}, + checkSuffixOnly: true, + notifier: nil, + discoverer: nil, }, { - target: "127.0.0.1:9876", - name: "with single host", - hostPorts: []string{"127.0.0.1:9876"}, - checkSuffixOnly: false, - notifier: nil, - discoverer: nil, - checkConnectionState: false, + target: "127.0.0.1:9876", + name: "with single host", + hostPorts: []string{"127.0.0.1:9876"}, + checkSuffixOnly: false, + notifier: nil, + discoverer: nil, }, { - target: "///round_robin", - name: "with custom resolver and fixed discoverer", - hostPorts: []string{"dns://random_stuff"}, - checkSuffixOnly: true, - notifier: noopNotifier{}, - discoverer: discovery.FixedDiscoverer{}, - checkConnectionState: false, + target: "///round_robin", + name: "with custom resolver and fixed discoverer", + hostPorts: []string{"dns://random_stuff"}, + checkSuffixOnly: true, + notifier: noopNotifier{}, + discoverer: discovery.FixedDiscoverer{}, }, { - target: "", - name: "without collectorPorts and resolver", - hostPorts: nil, - checkSuffixOnly: false, - notifier: nil, - discoverer: nil, - expectedError: "at least one collector hostPort address is required when resolver is not available", - checkConnectionState: false, - }, - { - target: addr1.String(), - name: "with collector connection status ready", - hostPorts: []string{addr1.String()}, - checkSuffixOnly: false, - notifier: nil, - discoverer: nil, - checkConnectionState: true, - expectedState: "READY", - }, - { - target: "random_stuff", - name: "with collector connection status failure", - hostPorts: []string{"random_stuff"}, - checkSuffixOnly: false, - notifier: nil, - discoverer: nil, - checkConnectionState: true, - expectedState: "TRANSIENT_FAILURE", + target: "", + name: "without collectorPorts and resolver", + hostPorts: nil, + checkSuffixOnly: false, + notifier: nil, + discoverer: nil, + expectedError: "at least one collector hostPort address is required when resolver is not available", }, } @@ -159,9 +132,6 @@ func TestBuilderWithCollectors(t *testing.T) { require.NoError(t, err) defer conn.Close() require.NotNil(t, conn) - if test.checkConnectionState { - assertConnectionState(t, conn, test.expectedState) - } if test.checkSuffixOnly { assert.True(t, strings.HasSuffix(conn.Target(), test.target)) } else { @@ -395,20 +365,6 @@ func TestProxyClientTLS(t *testing.T) { } } -func assertConnectionState(t *testing.T, conn *grpc.ClientConn, expectedState string) { - for { - s := conn.GetState() - if s == connectivity.Ready { - assert.Equal(t, expectedState, s.String()) - break - } - if s == connectivity.TransientFailure { - assert.Equal(t, expectedState, s.String()) - break - } - } -} - type fakeInterceptor struct { isCalled bool } From b8f750de81513d85263025d917ba27eca19562b2 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 19:24:01 -0400 Subject: [PATCH 02/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/grpc_handler_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/query/app/grpc_handler_test.go b/cmd/query/app/grpc_handler_test.go index 72c524f79af..bec65d371b3 100644 --- a/cmd/query/app/grpc_handler_test.go +++ b/cmd/query/app/grpc_handler_test.go @@ -174,10 +174,7 @@ func newGRPCServer(t *testing.T, q *querysvc.QueryService, mq querysvc.MetricsQu } func newGRPCClient(t *testing.T, addr string) *grpcClient { - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() - // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test - conn, err := grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) return &grpcClient{ From 384c6689f66d9c7609b518b4ffd985427644ef09 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 19:25:23 -0400 Subject: [PATCH 03/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 4a08c7b4f29..df1c8d6bc83 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -415,17 +415,13 @@ func TestServerHTTPTLS(t *testing.T) { } func newGRPCClientWithTLS(t *testing.T, addr string, creds credentials.TransportCredentials) *grpcClient { - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) - defer cancel() var conn *grpc.ClientConn var err error if creds != nil { - // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test - conn, err = grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(creds)) + conn, err = grpc.NewClient(addr, grpc.WithTransportCredentials(creds)) } else { - // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test - conn, err = grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err = grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) } require.NoError(t, err) From 5da003db9c6cf26ef7ef6ed0af5bb7ca0102468d Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 19:27:15 -0400 Subject: [PATCH 04/21] fix Signed-off-by: Yuri Shkuro --- plugin/storage/grpc/config/config.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/plugin/storage/grpc/config/config.go b/plugin/storage/grpc/config/config.go index c22e9f9d6f1..6585118b242 100644 --- a/plugin/storage/grpc/config/config.go +++ b/plugin/storage/grpc/config/config.go @@ -15,7 +15,6 @@ package config import ( - "context" "fmt" "os/exec" "time" @@ -109,17 +108,13 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) } - ctx, cancel := context.WithTimeout(context.Background(), c.RemoteConnectTimeout) - defer cancel() - tenancyMgr := tenancy.NewManager(&c.TenancyOpts) if tenancyMgr.Enabled { opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr))) opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr))) } var err error - // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test - c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) + c.remoteConn, err = grpc.NewClient(c.RemoteServerAddr, opts...) if err != nil { return nil, fmt.Errorf("error connecting to remote storage: %w", err) } From 053634ebf5a51bb2f75e2606eb053d48fa3c555a Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 21:01:43 -0400 Subject: [PATCH 05/21] fix Signed-off-by: Yuri Shkuro --- plugin/storage/grpc/config/config.go | 10 ++++++---- plugin/storage/grpc/config/config_test.go | 23 +++++++++++++++++++++++ plugin/storage/grpc/factory_test.go | 10 ++++------ 3 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 plugin/storage/grpc/config/config_test.go diff --git a/plugin/storage/grpc/config/config.go b/plugin/storage/grpc/config/config.go index 6585118b242..fe96ee35582 100644 --- a/plugin/storage/grpc/config/config.go +++ b/plugin/storage/grpc/config/config.go @@ -76,7 +76,7 @@ func (c *Configuration) Build(logger *zap.Logger, tracerProvider trace.TracerPro if c.PluginBinary != "" { return c.buildPlugin(logger, tracerProvider) } else { - return c.buildRemote(logger, tracerProvider) + return c.buildRemote(logger, tracerProvider, grpc.NewClient) } } @@ -92,7 +92,9 @@ func (c *Configuration) Close() error { return c.RemoteTLS.Close() } -func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) { +type newClientFn func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) + +func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider, newClient newClientFn) (*ClientPluginServices, error) { opts := []grpc.DialOption{ grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(tracerProvider))), grpc.WithBlock(), @@ -114,9 +116,9 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr))) } var err error - c.remoteConn, err = grpc.NewClient(c.RemoteServerAddr, opts...) + c.remoteConn, err = newClient(c.RemoteServerAddr, opts...) if err != nil { - return nil, fmt.Errorf("error connecting to remote storage: %w", err) + return nil, fmt.Errorf("error creating remote storage client: %w", err) } grpcClient := shared.NewGRPCClient(c.remoteConn) diff --git a/plugin/storage/grpc/config/config_test.go b/plugin/storage/grpc/config/config_test.go new file mode 100644 index 00000000000..64f54e58418 --- /dev/null +++ b/plugin/storage/grpc/config/config_test.go @@ -0,0 +1,23 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "google.golang.org/grpc" +) + +func TestBuildRemoteNewClientError(t *testing.T) { + // this is a silly test to verify handling of error from grpc.NewClient, which cannot be induced via params. + c := &Configuration{} + _, err := c.buildRemote(zap.NewNop(), nil, func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) { + return nil, errors.New("test error") + }) + require.Error(t, err) + require.Contains(t, err.Error(), "error creating remote storage client") +} diff --git a/plugin/storage/grpc/factory_test.go b/plugin/storage/grpc/factory_test.go index 62699eb2e2a..f9033964d4f 100644 --- a/plugin/storage/grpc/factory_test.go +++ b/plugin/storage/grpc/factory_test.go @@ -148,10 +148,6 @@ func TestGRPCStorageFactory(t *testing.T) { } func TestGRPCStorageFactoryWithConfig(t *testing.T) { - cfg := grpcConfig.Configuration{} - _, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) - require.ErrorContains(t, err, "grpc-plugin builder failed to create a store: error connecting to remote storage") - lis, err := net.Listen("tcp", ":0") require.NoError(t, err, "failed to listen") @@ -163,8 +159,10 @@ func TestGRPCStorageFactoryWithConfig(t *testing.T) { }() defer s.Stop() - cfg.RemoteServerAddr = lis.Addr().String() - cfg.RemoteConnectTimeout = 1 * time.Second + cfg := grpcConfig.Configuration{ + RemoteServerAddr: lis.Addr().String(), + RemoteConnectTimeout: 1 * time.Second, + } f, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) require.NoError(t, err) require.NoError(t, f.Close()) From 7b72e75258ac984af2a38a7f2ce1442aa0ab8a50 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Fri, 26 Apr 2024 21:32:51 -0400 Subject: [PATCH 06/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index df1c8d6bc83..3e8d82097d3 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -344,6 +344,7 @@ func TestServerHTTPTLS(t *testing.T) { jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) + defer server.Close() var clientError error var clientClose func() error @@ -408,8 +409,8 @@ func TestServerHTTPTLS(t *testing.T) { require.NoError(t, err2) } } - server.Close() - assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) + // server.Close() + // assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) }) } } @@ -490,6 +491,7 @@ func TestServerGRPCTLS(t *testing.T) { jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) + defer server.Close() var clientError error var client *grpcClient @@ -500,12 +502,11 @@ func TestServerGRPCTLS(t *testing.T) { defer test.clientTLS.Close() creds := credentials.NewTLS(clientTLSCfg) client = newGRPCClientWithTLS(t, ports.PortToHostPort(ports.QueryGRPC), creds) - } else { client = newGRPCClientWithTLS(t, ports.PortToHostPort(ports.QueryGRPC), nil) } - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() res, clientError := client.GetServices(ctx, &api_v2.GetServicesRequest{}) @@ -517,8 +518,8 @@ func TestServerGRPCTLS(t *testing.T) { assert.Equal(t, expectedServices, res.Services) } require.NoError(t, client.conn.Close()) - server.Close() - assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) + // server.Close() + // assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) }) } } From e942cf3ef33b741566a7f4b89321d034c3215d06 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 18:33:54 -0400 Subject: [PATCH 07/21] fix Signed-off-by: Yuri Shkuro --- plugin/storage/grpc/config/config.go | 15 +++++++++------ plugin/storage/grpc/factory_test.go | 10 ++++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/plugin/storage/grpc/config/config.go b/plugin/storage/grpc/config/config.go index fe96ee35582..c22e9f9d6f1 100644 --- a/plugin/storage/grpc/config/config.go +++ b/plugin/storage/grpc/config/config.go @@ -15,6 +15,7 @@ package config import ( + "context" "fmt" "os/exec" "time" @@ -76,7 +77,7 @@ func (c *Configuration) Build(logger *zap.Logger, tracerProvider trace.TracerPro if c.PluginBinary != "" { return c.buildPlugin(logger, tracerProvider) } else { - return c.buildRemote(logger, tracerProvider, grpc.NewClient) + return c.buildRemote(logger, tracerProvider) } } @@ -92,9 +93,7 @@ func (c *Configuration) Close() error { return c.RemoteTLS.Close() } -type newClientFn func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) - -func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider, newClient newClientFn) (*ClientPluginServices, error) { +func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.TracerProvider) (*ClientPluginServices, error) { opts := []grpc.DialOption{ grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(tracerProvider))), grpc.WithBlock(), @@ -110,15 +109,19 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) } + ctx, cancel := context.WithTimeout(context.Background(), c.RemoteConnectTimeout) + defer cancel() + tenancyMgr := tenancy.NewManager(&c.TenancyOpts) if tenancyMgr.Enabled { opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr))) opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr))) } var err error - c.remoteConn, err = newClient(c.RemoteServerAddr, opts...) + // TODO: Need to replace grpc.DialContext with grpc.NewClient and pass test + c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) if err != nil { - return nil, fmt.Errorf("error creating remote storage client: %w", err) + return nil, fmt.Errorf("error connecting to remote storage: %w", err) } grpcClient := shared.NewGRPCClient(c.remoteConn) diff --git a/plugin/storage/grpc/factory_test.go b/plugin/storage/grpc/factory_test.go index f9033964d4f..62699eb2e2a 100644 --- a/plugin/storage/grpc/factory_test.go +++ b/plugin/storage/grpc/factory_test.go @@ -148,6 +148,10 @@ func TestGRPCStorageFactory(t *testing.T) { } func TestGRPCStorageFactoryWithConfig(t *testing.T) { + cfg := grpcConfig.Configuration{} + _, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) + require.ErrorContains(t, err, "grpc-plugin builder failed to create a store: error connecting to remote storage") + lis, err := net.Listen("tcp", ":0") require.NoError(t, err, "failed to listen") @@ -159,10 +163,8 @@ func TestGRPCStorageFactoryWithConfig(t *testing.T) { }() defer s.Stop() - cfg := grpcConfig.Configuration{ - RemoteServerAddr: lis.Addr().String(), - RemoteConnectTimeout: 1 * time.Second, - } + cfg.RemoteServerAddr = lis.Addr().String() + cfg.RemoteConnectTimeout = 1 * time.Second f, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop()) require.NoError(t, err) require.NoError(t, f.Close()) From 4f6c4f9c45e614c4291ae8853df1ba3dcda5eed9 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 18:34:20 -0400 Subject: [PATCH 08/21] fix Signed-off-by: Yuri Shkuro --- plugin/storage/grpc/config/config_test.go | 23 ----------------------- 1 file changed, 23 deletions(-) delete mode 100644 plugin/storage/grpc/config/config_test.go diff --git a/plugin/storage/grpc/config/config_test.go b/plugin/storage/grpc/config/config_test.go deleted file mode 100644 index 64f54e58418..00000000000 --- a/plugin/storage/grpc/config/config_test.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright (c) 2024 The Jaeger Authors. -// SPDX-License-Identifier: Apache-2.0 - -package config - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/require" - "go.uber.org/zap" - "google.golang.org/grpc" -) - -func TestBuildRemoteNewClientError(t *testing.T) { - // this is a silly test to verify handling of error from grpc.NewClient, which cannot be induced via params. - c := &Configuration{} - _, err := c.buildRemote(zap.NewNop(), nil, func(target string, opts ...grpc.DialOption) (conn *grpc.ClientConn, err error) { - return nil, errors.New("test error") - }) - require.Error(t, err) - require.Contains(t, err.Error(), "error creating remote storage client") -} From 6907c86a5d63c141db6de69725a7e93a935c9526 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 18:45:47 -0400 Subject: [PATCH 09/21] add-logging Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 3e8d82097d3..9e1ee699040 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -27,6 +27,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zaptest" "go.uber.org/zap/zaptest/observer" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -478,7 +479,7 @@ func TestServerGRPCTLS(t *testing.T) { }, } flagsSvc := flags.NewService(ports.QueryAdminHTTP) - flagsSvc.Logger = zap.NewNop() + flagsSvc.Logger = zaptest.NewLogger(t) spanReader := &spanstoremocks.Reader{} dependencyReader := &depsmocks.Reader{} From 58c341a5c5430d9e6a6757f71b1407ea43376a25 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 19:01:26 -0400 Subject: [PATCH 10/21] add-logging Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 9e1ee699040..8a8cd90b297 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -510,7 +510,9 @@ func TestServerGRPCTLS(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + flagsSvc.Logger.Info("calling client.GetServices()") res, clientError := client.GetServices(ctx, &api_v2.GetServicesRequest{}) + flagsSvc.Logger.Info("returned from GetServices()") if test.expectClientError { require.Error(t, clientError) From 7de4e333774fb381e49df326dda607e51fb80e67 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 19:17:27 -0400 Subject: [PATCH 11/21] logging Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 8a8cd90b297..5f79e82835d 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -20,6 +20,8 @@ import ( "fmt" "net" "net/http" + "os" + "os/exec" "testing" "time" @@ -465,6 +467,19 @@ func TestServerGRPCTLS(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + { // TODO remove me + cmd := exec.Cmd{ + Path: "/usr/sbin/lsof", + Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, + // Change the working directory to the root of this project + // since the binary config file jaeger_query's ui_config points to + // "./cmd/jaeger/config-ui.json" + Dir: "../../../..", + Stdout: os.Stderr, + Stderr: os.Stderr, + } + require.NoError(t, cmd.Start()) + } TLSHTTP := disabledTLSCfg if test.HTTPTLSEnabled { TLSHTTP = enabledTLSCfg @@ -510,6 +525,23 @@ func TestServerGRPCTLS(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + { // TODO remove me + flagsSvc.Logger.Info("sleep 5sec to server to start") + time.Sleep(5 * time.Second) + cmd := exec.Cmd{ + Path: "/usr/sbin/lsof", + Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, + // Change the working directory to the root of this project + // since the binary config file jaeger_query's ui_config points to + // "./cmd/jaeger/config-ui.json" + Dir: "../../../..", + Stdout: os.Stderr, + Stderr: os.Stderr, + } + require.NoError(t, cmd.Start()) + } + + // TODO temporary ^ flagsSvc.Logger.Info("calling client.GetServices()") res, clientError := client.GetServices(ctx, &api_v2.GetServicesRequest{}) flagsSvc.Logger.Info("returned from GetServices()") From f9e0ddd3063058e0e3e61e156a5b1ceba89520b4 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 19:24:47 -0400 Subject: [PATCH 12/21] logging Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 5f79e82835d..b3452102110 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -529,7 +529,7 @@ func TestServerGRPCTLS(t *testing.T) { flagsSvc.Logger.Info("sleep 5sec to server to start") time.Sleep(5 * time.Second) cmd := exec.Cmd{ - Path: "/usr/sbin/lsof", + Path: "lsof", Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, // Change the working directory to the root of this project // since the binary config file jaeger_query's ui_config points to From ca81618d035b9a3157e2efd3aabbc266bb2bd5cd Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 19:34:56 -0400 Subject: [PATCH 13/21] logging Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index b3452102110..580521e917f 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -469,12 +469,8 @@ func TestServerGRPCTLS(t *testing.T) { t.Run(test.name, func(t *testing.T) { { // TODO remove me cmd := exec.Cmd{ - Path: "/usr/sbin/lsof", - Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, - // Change the working directory to the root of this project - // since the binary config file jaeger_query's ui_config points to - // "./cmd/jaeger/config-ui.json" - Dir: "../../../..", + Path: "lsof", + Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, Stdout: os.Stderr, Stderr: os.Stderr, } @@ -529,12 +525,8 @@ func TestServerGRPCTLS(t *testing.T) { flagsSvc.Logger.Info("sleep 5sec to server to start") time.Sleep(5 * time.Second) cmd := exec.Cmd{ - Path: "lsof", - Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, - // Change the working directory to the root of this project - // since the binary config file jaeger_query's ui_config points to - // "./cmd/jaeger/config-ui.json" - Dir: "../../../..", + Path: "lsof", + Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, Stdout: os.Stderr, Stderr: os.Stderr, } From 6641adc7ba901f97e40fee476c054364f1e5ef5e Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 20:10:58 -0400 Subject: [PATCH 14/21] lsof Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 580521e917f..79839d89ac4 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -469,8 +469,8 @@ func TestServerGRPCTLS(t *testing.T) { t.Run(test.name, func(t *testing.T) { { // TODO remove me cmd := exec.Cmd{ - Path: "lsof", - Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, + Path: "/bin/bash", + Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, Stdout: os.Stderr, Stderr: os.Stderr, } @@ -525,8 +525,8 @@ func TestServerGRPCTLS(t *testing.T) { flagsSvc.Logger.Info("sleep 5sec to server to start") time.Sleep(5 * time.Second) cmd := exec.Cmd{ - Path: "lsof", - Args: []string{"lsof", "-iTCP", "-sTCP:LISTEN", "-P", "+c0"}, + Path: "/bin/bash", + Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, Stdout: os.Stderr, Stderr: os.Stderr, } From 9ecc82e68b5304b9fcada7ea10ea21d824244bfb Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 20:20:59 -0400 Subject: [PATCH 15/21] lsof Signed-off-by: Yuri Shkuro --- .github/workflows/ci-unit-tests.yml | 6 +++++- cmd/query/app/server_test.go | 29 ++++++++++++++--------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci-unit-tests.yml b/.github/workflows/ci-unit-tests.yml index aa9961533c9..af626d7dd64 100644 --- a/.github/workflows/ci-unit-tests.yml +++ b/.github/workflows/ci-unit-tests.yml @@ -17,7 +17,7 @@ permissions: jobs: unit-tests: permissions: - checks: write + checks: write runs-on: ubuntu-latest steps: - name: Harden Runner @@ -32,6 +32,10 @@ jobs: go-version: 1.22.x cache-dependency-path: ./go.sum + # download dependencies separately to keep unit test step's output cleaner + - name: go mod download + run: go mod download + - name: Install test deps # even though the same target runs from test-ci, running it separately makes for cleaner log in GH workflow run: make install-test-tools diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 79839d89ac4..7a1685d108d 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -465,16 +465,22 @@ func TestServerGRPCTLS(t *testing.T) { ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", } + lsof := func(name string) { + println("::group::running lsof", name) + println("::endgroup::") + cmd := exec.Cmd{ + Path: "/bin/bash", + Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, + Stdout: os.Stderr, + Stderr: os.Stderr, + } + require.NoError(t, cmd.Start()) + } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { { // TODO remove me - cmd := exec.Cmd{ - Path: "/bin/bash", - Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, - Stdout: os.Stderr, - Stderr: os.Stderr, - } - require.NoError(t, cmd.Start()) + lsof("at the start of the test") } TLSHTTP := disabledTLSCfg if test.HTTPTLSEnabled { @@ -523,14 +529,7 @@ func TestServerGRPCTLS(t *testing.T) { { // TODO remove me flagsSvc.Logger.Info("sleep 5sec to server to start") - time.Sleep(5 * time.Second) - cmd := exec.Cmd{ - Path: "/bin/bash", - Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, - Stdout: os.Stderr, - Stderr: os.Stderr, - } - require.NoError(t, cmd.Start()) + lsof("before client call") } // TODO temporary ^ From 8e45cf4daed95e47399deb70ee36765efc557da4 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 20:29:04 -0400 Subject: [PATCH 16/21] lsof Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 7a1685d108d..a774841862f 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -471,8 +471,8 @@ func TestServerGRPCTLS(t *testing.T) { cmd := exec.Cmd{ Path: "/bin/bash", Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, - Stdout: os.Stderr, - Stderr: os.Stderr, + Stdout: os.Stdout, + Stderr: os.Stdout, } require.NoError(t, cmd.Start()) } From 50b9b8c4f7f10eeabab941f63cf05ce1861d3bb5 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 20:53:54 -0400 Subject: [PATCH 17/21] lsof Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index a774841862f..1260fff6016 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -474,7 +474,7 @@ func TestServerGRPCTLS(t *testing.T) { Stdout: os.Stdout, Stderr: os.Stdout, } - require.NoError(t, cmd.Start()) + require.NoError(t, cmd.Run()) } for _, test := range tests { From b1f11900ed8eb1fbe967a908e2f365ca5cdfca54 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sun, 28 Apr 2024 21:10:47 -0400 Subject: [PATCH 18/21] timeout Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 1260fff6016..76b51791792 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -524,7 +524,7 @@ func TestServerGRPCTLS(t *testing.T) { client = newGRPCClientWithTLS(t, ports.PortToHostPort(ports.QueryGRPC), nil) } - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() { // TODO remove me From 46d8bc85cca88d3b17169973e406fc5d17393fd4 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 11 May 2024 21:16:28 -0400 Subject: [PATCH 19/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 80 ++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 76b51791792..7ac594a1dd6 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -20,8 +20,6 @@ import ( "fmt" "net" "net/http" - "os" - "os/exec" "testing" "time" @@ -465,23 +463,8 @@ func TestServerGRPCTLS(t *testing.T) { ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", } - lsof := func(name string) { - println("::group::running lsof", name) - println("::endgroup::") - cmd := exec.Cmd{ - Path: "/bin/bash", - Args: []string{"bash", "-c", "sudo lsof -iTCP -sTCP:LISTEN -P +c0"}, - Stdout: os.Stdout, - Stderr: os.Stdout, - } - require.NoError(t, cmd.Run()) - } - for _, test := range tests { t.Run(test.name, func(t *testing.T) { - { // TODO remove me - lsof("at the start of the test") - } TLSHTTP := disabledTLSCfg if test.HTTPTLSEnabled { TLSHTTP = enabledTLSCfg @@ -509,11 +492,11 @@ func TestServerGRPCTLS(t *testing.T) { jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) - defer server.Close() + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) - var clientError error var client *grpcClient - if serverOptions.TLSGRPC.Enabled { clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop()) require.NoError(t, err0) @@ -523,16 +506,14 @@ func TestServerGRPCTLS(t *testing.T) { } else { client = newGRPCClientWithTLS(t, ports.PortToHostPort(ports.QueryGRPC), nil) } + t.Cleanup(func() { + require.NoError(t, client.conn.Close()) + }) - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + // using generous timeout since grpc.NewClient no longer does a handshake. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - { // TODO remove me - flagsSvc.Logger.Info("sleep 5sec to server to start") - lsof("before client call") - } - - // TODO temporary ^ flagsSvc.Logger.Info("calling client.GetServices()") res, clientError := client.GetServices(ctx, &api_v2.GetServicesRequest{}) flagsSvc.Logger.Info("returned from GetServices()") @@ -543,9 +524,6 @@ func TestServerGRPCTLS(t *testing.T) { require.NoError(t, clientError) assert.Equal(t, expectedServices, res.Services) } - require.NoError(t, client.conn.Close()) - // server.Close() - // assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) }) } } @@ -609,10 +587,7 @@ func TestServerInUseHostPort(t *testing.T) { jtracer.NoOp(), ) require.NoError(t, err) - - err = server.Start() - require.Error(t, err) - + require.Error(t, server.Start()) server.Close() }) } @@ -640,19 +615,22 @@ func TestServerSinglePort(t *testing.T) { jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) client := newGRPCClient(t, hostPort) - defer client.conn.Close() + t.Cleanup(func() { + require.NoError(t, client.conn.Close()) + }) - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + // using generous timeout since grpc.NewClient no longer does a handshake. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() res, err := client.GetServices(ctx, &api_v2.GetServicesRequest{}) require.NoError(t, err) assert.Equal(t, expectedServices, res.Services) - - server.Close() - assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) } func TestServerGracefulExit(t *testing.T) { @@ -664,20 +642,32 @@ func TestServerGracefulExit(t *testing.T) { flagsSvc.Logger = zap.New(zapCore) hostPort := ports.PortToHostPort(ports.QueryAdminHTTP) - querySvc := &querysvc.QueryService{} - tracer := jtracer.NoOp() + spanReader := &spanstoremocks.Reader{} + dependencyReader := &depsmocks.Reader{} + expectedServices := []string{"test"} + spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) + querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc, nil, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort}, - tenancy.NewManager(&tenancy.Options{}), tracer) + tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) // Wait for servers to come up before we can call .Close() - // TODO Find a way to wait only as long as necessary. Unconditional sleep slows down the tests. - time.Sleep(1 * time.Second) - server.Close() + { + client := newGRPCClient(t, hostPort) + t.Cleanup(func() { + require.NoError(t, client.conn.Close()) + }) + // using generous timeout since grpc.NewClient no longer does a handshake. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, err := client.GetServices(ctx, &api_v2.GetServicesRequest{}) + require.NoError(t, err) + } + server.Close() for _, logEntry := range logs.All() { assert.NotEqual(t, zap.ErrorLevel, logEntry.Level, "Error log found on server exit: %v", logEntry) From 404d01db68f61f2e74d627922fc4387f18999166 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 11 May 2024 21:45:09 -0400 Subject: [PATCH 20/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/server.go | 3 +- cmd/query/app/server_test.go | 115 +++++++++++++++++------------------ 2 files changed, 58 insertions(+), 60 deletions(-) diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 9c34bc26018..719f3957778 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -308,7 +308,7 @@ func (s *Server) Start() error { if err != nil && !errors.Is(err, http.ErrServerClosed) && !errors.Is(err, cmux.ErrListenerClosed) && !errors.Is(err, cmux.ErrServerClosed) { s.logger.Error("Could not start HTTP server", zap.Error(err)) } - + s.logger.Info("HTTP server stopped", zap.Int("port", httpPort), zap.String("addr", s.queryOptions.HTTPHostPort)) s.healthCheck.Set(healthcheck.Unavailable) s.bgFinished.Done() }() @@ -321,6 +321,7 @@ func (s *Server) Start() error { if err := s.grpcServer.Serve(s.grpcConn); err != nil { s.logger.Error("Could not start GRPC server", zap.Error(err)) } + s.logger.Info("GRPC server stopped", zap.Int("port", grpcPort), zap.String("addr", s.queryOptions.GRPCHostPort)) s.healthCheck.Set(healthcheck.Unavailable) s.bgFinished.Done() }() diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 7ac594a1dd6..dc95d43a4db 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -67,7 +67,7 @@ func TestCreateTLSServerSinglePortError(t *testing.T) { ClientCAPath: testCertKeyLocation + "/example-CA-cert.pem", } - _, err := NewServer(zap.NewNop(), healthcheck.New(), &querysvc.QueryService{}, nil, + _, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8080", TLSGRPC: tlsCfg, TLSHTTP: tlsCfg}, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.Error(t, err) @@ -81,7 +81,7 @@ func TestCreateTLSGrpcServerError(t *testing.T) { ClientCAPath: "invalid/path", } - _, err := NewServer(zap.NewNop(), healthcheck.New(), &querysvc.QueryService{}, nil, + _, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8081", TLSGRPC: tlsCfg}, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.Error(t, err) @@ -95,7 +95,7 @@ func TestCreateTLSHttpServerError(t *testing.T) { ClientCAPath: "invalid/path", } - _, err := NewServer(zap.NewNop(), healthcheck.New(), &querysvc.QueryService{}, nil, + _, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, &QueryOptions{HTTPHostPort: ":8080", GRPCHostPort: ":8081", TLSHTTP: tlsCfg}, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.Error(t, err) @@ -284,6 +284,27 @@ var testCases = []struct { }, } +type fakeQueryService struct { + qs *querysvc.QueryService + spanReader *spanstoremocks.Reader + dependencyReader *depsmocks.Reader + expectedServices []string +} + +func makeQuerySvc() *fakeQueryService { + spanReader := &spanstoremocks.Reader{} + dependencyReader := &depsmocks.Reader{} + expectedServices := []string{"test"} + spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) + qs := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) + return &fakeQueryService{ + qs: qs, + spanReader: spanReader, + dependencyReader: dependencyReader, + expectedServices: expectedServices, + } +} + func TestServerHTTPTLS(t *testing.T) { testlen := len(testCases) @@ -332,30 +353,25 @@ func TestServerHTTPTLS(t *testing.T) { }, } flagsSvc := flags.NewService(ports.QueryAdminHTTP) - flagsSvc.Logger = zap.NewNop() - - spanReader := &spanstoremocks.Reader{} - dependencyReader := &depsmocks.Reader{} - expectedServices := []string{"test"} - spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) + flagsSvc.Logger = zaptest.NewLogger(t) - querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc, + querySvc := makeQuerySvc() + server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc.qs, nil, serverOptions, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) - defer server.Close() + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) var clientError error var clientClose func() error var clientTLSCfg *tls.Config if serverOptions.TLSHTTP.Enabled { - var err0 error - - clientTLSCfg, err0 = test.clientTLS.Config(zap.NewNop()) + clientTLSCfg, err0 = test.clientTLS.Config(flagsSvc.Logger) defer test.clientTLS.Close() require.NoError(t, err0) @@ -392,7 +408,7 @@ func TestServerHTTPTLS(t *testing.T) { TLSClientConfig: clientTLSCfg, }, } - readMock := spanReader + readMock := querySvc.spanReader readMock.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")).Return([]*model.Trace{mockTrace}, nil).Once() queryString := "/api/traces?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20ms" req, err := http.NewRequest(http.MethodGet, "https://localhost:"+fmt.Sprintf("%d", ports.QueryHTTP)+queryString, nil) @@ -410,8 +426,6 @@ func TestServerHTTPTLS(t *testing.T) { require.NoError(t, err2) } } - // server.Close() - // assert.Equal(t, healthcheck.Unavailable, flagsSvc.HC().Get()) }) } } @@ -481,13 +495,8 @@ func TestServerGRPCTLS(t *testing.T) { flagsSvc := flags.NewService(ports.QueryAdminHTTP) flagsSvc.Logger = zaptest.NewLogger(t) - spanReader := &spanstoremocks.Reader{} - dependencyReader := &depsmocks.Reader{} - expectedServices := []string{"test"} - spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) - - querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc, + querySvc := makeQuerySvc() + server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc.qs, nil, serverOptions, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.NoError(t, err) @@ -498,7 +507,7 @@ func TestServerGRPCTLS(t *testing.T) { var client *grpcClient if serverOptions.TLSGRPC.Enabled { - clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop()) + clientTLSCfg, err0 := test.clientTLS.Config(flagsSvc.Logger) require.NoError(t, err0) defer test.clientTLS.Close() creds := credentials.NewTLS(clientTLSCfg) @@ -511,7 +520,7 @@ func TestServerGRPCTLS(t *testing.T) { }) // using generous timeout since grpc.NewClient no longer does a handshake. - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() flagsSvc.Logger.Info("calling client.GetServices()") @@ -522,16 +531,16 @@ func TestServerGRPCTLS(t *testing.T) { require.Error(t, clientError) } else { require.NoError(t, clientError) - assert.Equal(t, expectedServices, res.Services) + assert.Equal(t, querySvc.expectedServices, res.Services) } }) } } func TestServerBadHostPort(t *testing.T) { - _, err := NewServer(zap.NewNop(), healthcheck.New(), &querysvc.QueryService{}, nil, + _, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, &QueryOptions{ - HTTPHostPort: "8080", + HTTPHostPort: "8080", // bad string, not :port GRPCHostPort: "127.0.0.1:8081", QueryOptionsBase: QueryOptionsBase{ BearerTokenPropagation: true, @@ -539,12 +548,12 @@ func TestServerBadHostPort(t *testing.T) { }, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) - require.Error(t, err) - _, err = NewServer(zap.NewNop(), healthcheck.New(), &querysvc.QueryService{}, nil, + + _, err = NewServer(zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, &QueryOptions{ HTTPHostPort: "127.0.0.1:8081", - GRPCHostPort: "9123", + GRPCHostPort: "9123", // bad string, not :port QueryOptionsBase: QueryOptionsBase{ BearerTokenPropagation: true, }, @@ -572,7 +581,7 @@ func TestServerInUseHostPort(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { server, err := NewServer( - zap.NewNop(), + zaptest.NewLogger(t), healthcheck.New(), &querysvc.QueryService{}, nil, @@ -595,15 +604,10 @@ func TestServerInUseHostPort(t *testing.T) { func TestServerSinglePort(t *testing.T) { flagsSvc := flags.NewService(ports.QueryAdminHTTP) - flagsSvc.Logger = zap.NewNop() + flagsSvc.Logger = zaptest.NewLogger(t) hostPort := ports.GetAddressFromCLIOptions(ports.QueryHTTP, "") - spanReader := &spanstoremocks.Reader{} - dependencyReader := &depsmocks.Reader{} - expectedServices := []string{"test"} - spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) - - querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc, nil, + querySvc := makeQuerySvc() + server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc.qs, nil, &QueryOptions{ GRPCHostPort: hostPort, HTTPHostPort: hostPort, @@ -625,12 +629,12 @@ func TestServerSinglePort(t *testing.T) { }) // using generous timeout since grpc.NewClient no longer does a handshake. - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() res, err := client.GetServices(ctx, &api_v2.GetServicesRequest{}) require.NoError(t, err) - assert.Equal(t, expectedServices, res.Services) + assert.Equal(t, querySvc.expectedServices, res.Services) } func TestServerGracefulExit(t *testing.T) { @@ -642,13 +646,8 @@ func TestServerGracefulExit(t *testing.T) { flagsSvc.Logger = zap.New(zapCore) hostPort := ports.PortToHostPort(ports.QueryAdminHTTP) - spanReader := &spanstoremocks.Reader{} - dependencyReader := &depsmocks.Reader{} - expectedServices := []string{"test"} - spanReader.On("GetServices", mock.AnythingOfType("*context.valueCtx")).Return(expectedServices, nil) - querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - - server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc, nil, + querySvc := makeQuerySvc() + server, err := NewServer(flagsSvc.Logger, flagsSvc.HC(), querySvc.qs, nil, &QueryOptions{GRPCHostPort: hostPort, HTTPHostPort: hostPort}, tenancy.NewManager(&tenancy.Options{}), jtracer.NoOp()) require.NoError(t, err) @@ -661,7 +660,7 @@ func TestServerGracefulExit(t *testing.T) { require.NoError(t, client.conn.Close()) }) // using generous timeout since grpc.NewClient no longer does a handshake. - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() _, err := client.GetServices(ctx, &api_v2.GetServicesRequest{}) require.NoError(t, err) @@ -737,15 +736,14 @@ func TestServerHTTPTenancy(t *testing.T) { }, } tenancyMgr := tenancy.NewManager(&serverOptions.Tenancy) - - spanReader := &spanstoremocks.Reader{} - dependencyReader := &depsmocks.Reader{} - - querySvc := querysvc.NewQueryService(spanReader, dependencyReader, querysvc.QueryServiceOptions{}) - server, err := NewServer(zap.NewNop(), healthcheck.New(), querySvc, + querySvc := makeQuerySvc() + server, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), querySvc.qs, nil, serverOptions, tenancyMgr, jtracer.NoOp()) require.NoError(t, err) require.NoError(t, server.Start()) + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) for _, test := range testCases { t.Run(test.name, func(t *testing.T) { @@ -779,5 +777,4 @@ func TestServerHTTPTenancy(t *testing.T) { } }) } - server.Close() } From 8f69cee4ab05551e43aae8d519be99c82fa04da4 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 11 May 2024 21:50:51 -0400 Subject: [PATCH 21/21] fix Signed-off-by: Yuri Shkuro --- cmd/query/app/server_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index dc95d43a4db..cd6ab915b1e 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -408,8 +408,7 @@ func TestServerHTTPTLS(t *testing.T) { TLSClientConfig: clientTLSCfg, }, } - readMock := querySvc.spanReader - readMock.On("FindTraces", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("*spanstore.TraceQueryParameters")).Return([]*model.Trace{mockTrace}, nil).Once() + querySvc.spanReader.On("FindTraces", mock.Anything, mock.Anything).Return([]*model.Trace{mockTrace}, nil).Once() queryString := "/api/traces?service=service&start=0&end=0&operation=operation&limit=200&minDuration=20ms" req, err := http.NewRequest(http.MethodGet, "https://localhost:"+fmt.Sprintf("%d", ports.QueryHTTP)+queryString, nil) require.NoError(t, err) @@ -737,6 +736,7 @@ func TestServerHTTPTenancy(t *testing.T) { } tenancyMgr := tenancy.NewManager(&serverOptions.Tenancy) querySvc := makeQuerySvc() + querySvc.spanReader.On("FindTraces", mock.Anything, mock.Anything).Return([]*model.Trace{mockTrace}, nil).Once() server, err := NewServer(zaptest.NewLogger(t), healthcheck.New(), querySvc.qs, nil, serverOptions, tenancyMgr, jtracer.NoOp()) require.NoError(t, err)