From 258397a5a2bf83e78628c8f23dfb23eb92b0f1c7 Mon Sep 17 00:00:00 2001 From: Owen Cabalceta Date: Thu, 7 Nov 2024 19:25:45 -0500 Subject: [PATCH 1/3] chore: refactor listener client --- chrysom/listenerClient.go | 32 ++++++--------------- chrysom/listenerClient_test.go | 49 +++++--------------------------- go.mod | 3 +- go.sum | 1 + service.go | 52 ++++++++++++---------------------- service_test.go | 12 ++++---- 6 files changed, 41 insertions(+), 108 deletions(-) diff --git a/chrysom/listenerClient.go b/chrysom/listenerClient.go index 27a476a..82861d7 100644 --- a/chrysom/listenerClient.go +++ b/chrysom/listenerClient.go @@ -39,7 +39,7 @@ const ( ) // ListenerConfig contains config data for polling the Argus client. -type ListenerClientConfig struct { +type ListenerConfig struct { // Listener provides a mechanism to fetch a copy of all items within a bucket on // an interval. // (Optional). If not provided, listening won't be enabled for this client. @@ -73,21 +73,18 @@ type observerConfig struct { // NewListenerClient creates a new ListenerClient to be used to poll Argus // for updates. -func NewListenerClient(config ListenerClientConfig, +func NewListenerClient(config ListenerConfig, setLogger func(context.Context, *zap.Logger) context.Context, measures *Measures, r Reader, ) (*ListenerClient, error) { - err := validateListenerConfig(&config) - if err != nil { - return nil, err + if config.Listener == nil { + return nil, ErrNoListenerProvided } - if measures == nil { - return nil, ErrNilMeasures + if config.Logger == nil { + config.Logger = sallust.Default() } - if setLogger == nil { - setLogger = func(ctx context.Context, _ *zap.Logger) context.Context { - return ctx - } + if config.PullInterval == 0 { + config.PullInterval = defaultPullInterval } if r == nil { return nil, ErrNoReaderProvided @@ -168,16 +165,3 @@ func (c *ListenerClient) Stop(ctx context.Context) error { atomic.SwapInt32(&c.observer.state, stopped) return nil } - -func validateListenerConfig(config *ListenerClientConfig) error { - if config.Listener == nil { - return ErrNoListenerProvided - } - if config.Logger == nil { - config.Logger = sallust.Default() - } - if config.PullInterval == 0 { - config.PullInterval = defaultPullInterval - } - return nil -} diff --git a/chrysom/listenerClient_test.go b/chrysom/listenerClient_test.go index 3a4e6f4..541becb 100644 --- a/chrysom/listenerClient_test.go +++ b/chrysom/listenerClient_test.go @@ -32,7 +32,7 @@ var ( }, []string{OutcomeLabel}, )} - happyListenerClientConfig = ListenerClientConfig{ + happyListenerConfig = ListenerConfig{ Listener: mockListener, PullInterval: time.Second, Logger: sallust.Default(), @@ -109,7 +109,7 @@ func newStartStopClient(includeListener bool) (*ListenerClient, func(), error) { rw.Write(getItemsValidPayload()) })) - config := ListenerClientConfig{ + config := ListenerConfig{ PullInterval: time.Millisecond * 200, Logger: sallust.Default(), } @@ -124,68 +124,33 @@ func newStartStopClient(includeListener bool) (*ListenerClient, func(), error) { return client, server.Close, nil } -func TestValidateListenerConfig(t *testing.T) { - tcs := []struct { - desc string - expectedErr error - config ListenerClientConfig - }{ - { - desc: "Happy case Success", - config: happyListenerClientConfig, - }, - { - desc: "No listener Failure", - config: ListenerClientConfig{}, - expectedErr: ErrNoListenerProvided, - }, - { - desc: "No logger and no pull interval Success", - config: ListenerClientConfig{ - Listener: mockListener, - }, - }, - } - for _, tc := range tcs { - t.Run(tc.desc, func(t *testing.T) { - assert := assert.New(t) - c := tc.config - err := validateListenerConfig(&c) - assert.True(errors.Is(err, tc.expectedErr), - fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain", - err, tc.expectedErr), - ) - }) - } -} - func TestNewListenerClient(t *testing.T) { tcs := []struct { desc string - config ListenerClientConfig + config ListenerConfig expectedErr error measures *Measures reader Reader }{ { desc: "Listener Config Failure", - config: ListenerClientConfig{}, + config: ListenerConfig{}, expectedErr: ErrNoListenerProvided, }, { desc: "No measures Failure", - config: happyListenerClientConfig, + config: happyListenerConfig, expectedErr: ErrNilMeasures, }, { desc: "No reader Failure", - config: happyListenerClientConfig, + config: happyListenerConfig, measures: mockMeasures, expectedErr: ErrNoReaderProvided, }, { desc: "Happy case Success", - config: happyListenerClientConfig, + config: happyListenerConfig, measures: mockMeasures, reader: &BasicClient{}, }, diff --git a/go.mod b/go.mod index c44f61e..db32f51 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/prometheus/client_model v0.6.1 github.com/spf13/cast v1.6.0 github.com/stretchr/testify v1.9.0 + github.com/xmidt-org/arrange v0.4.0 github.com/xmidt-org/bascule v0.11.6 github.com/xmidt-org/httpaux v0.4.0 github.com/xmidt-org/sallust v0.2.2 @@ -31,6 +32,7 @@ require ( github.com/go-logfmt/logfmt v0.6.0 // indirect github.com/gorilla/mux v1.8.1 // indirect github.com/hashicorp/hcl v1.0.0 // indirect + github.com/justinas/alice v1.2.0 // indirect github.com/magiconair/properties v1.8.7 // indirect github.com/mitchellh/mapstructure v1.5.0 // indirect github.com/pelletier/go-toml/v2 v2.1.0 // indirect @@ -45,7 +47,6 @@ require ( github.com/spf13/viper v1.18.2 // indirect github.com/stretchr/objx v0.5.2 // indirect github.com/subosito/gotenv v1.6.0 // indirect - github.com/xmidt-org/arrange v0.4.0 // indirect go.uber.org/dig v1.17.1 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect diff --git a/go.sum b/go.sum index 0044173..30510e9 100644 --- a/go.sum +++ b/go.sum @@ -274,6 +274,7 @@ github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1 github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk= github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= +github.com/justinas/alice v1.2.0 h1:+MHSA/vccVCF4Uq37S42jwlkvI2Xzl7zTPCN5BnZNVo= github.com/justinas/alice v1.2.0/go.mod h1:fN5HRH/reO/zrUflLfTN43t3vXvKzvZIENsNEe7i7qA= github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= diff --git a/service.go b/service.go index a770ea1..213a28d 100644 --- a/service.go +++ b/service.go @@ -66,20 +66,6 @@ type Config struct { Validation ValidatorConfig } -// ListenerConfig contains information needed to initialize the Listener Client service. -type ListenerConfig struct { - Config chrysom.ListenerClientConfig - - // Logger for this package. - // Gets passed to Argus config before initializing the client. - // (Optional). Defaults to a no op logger. - Logger *zap.Logger - - // Measures for instrumenting this package. - // Gets passed to Argus config before initializing the client. - Measures chrysom.Measures -} - type ClientService struct { argus chrysom.PushReader logger *zap.Logger @@ -109,12 +95,12 @@ func NewService(cfg Config, getLogger func(context.Context) *zap.Logger) (*Clien // StartListener builds the Argus listener client service from the given configuration. // It allows adding watchers for the internal subscription state. Call the returned // function when you are done watching for updates. -func (s *ClientService) StartListener(cfg ListenerConfig, setLogger func(context.Context, *zap.Logger) context.Context, watches ...Watch) (func(), error) { +func (s *ClientService) StartListener(cfg chrysom.ListenerConfig, setLogger func(context.Context, *zap.Logger) context.Context, metrics chrysom.Measures, watches ...Watch) (func(), error) { if cfg.Logger == nil { cfg.Logger = sallust.Default() } - prepArgusListenerClientConfig(&cfg, watches...) - listener, err := chrysom.NewListenerClient(cfg.Config, setLogger, &cfg.Measures, s.argus) + cfg = prepArgusListenerConfig(cfg, metrics, watches...) + listener, err := chrysom.NewListenerClient(cfg, setLogger, metrics, s.argus) if err != nil { return nil, fmt.Errorf("failed to create chrysom listener client: %v", err) } @@ -170,10 +156,10 @@ func prepArgusBasicClientConfig(cfg *Config) error { return nil } -func prepArgusListenerClientConfig(cfg *ListenerConfig, watches ...Watch) { +func prepArgusListenerConfig(cfg chrysom.ListenerConfig, metrics chrysom.Measures, watches ...Watch) chrysom.ListenerConfig { logger := cfg.Logger - watches = append(watches, webhookListSizeWatch(cfg.Measures.WebhookListSizeGauge)) - cfg.Config.Listener = chrysom.ListenerFunc(func(items chrysom.Items) { + watches = append(watches, webhookListSizeWatch(metrics.WebhookListSizeGauge)) + cfg.Listener = chrysom.ListenerFunc(func(items chrysom.Items) { iws, err := ItemsToInternalWebhooks(items) if err != nil { logger.Error("Failed to convert items to webhooks", zap.Error(err)) @@ -183,6 +169,8 @@ func prepArgusListenerClientConfig(cfg *ListenerConfig, watches ...Watch) { watch.Update(iws) } }) + + return cfg } type ServiceIn struct { @@ -208,29 +196,25 @@ func ProvideService() fx.Option { type ListenerIn struct { fx.In - Measures chrysom.Measures - Logger *zap.Logger - Svc *ClientService - Watcher Watch - LC fx.Lifecycle + Measures chrysom.Measures + Logger *zap.Logger + Svc *ClientService + listenerConfig chrysom.ListenerConfig + Watcher Watch + LC fx.Lifecycle } func ProvideListener() fx.Option { return fx.Options( fx.Provide( - func(in ListenerIn) (listener ListenerConfig, err error) { - listener = ListenerConfig{ - Measures: in.Measures, - Logger: in.Logger, - } - - stopWatches, err := in.Svc.StartListener(listener, setLoggerInContext(), in.Watcher) + func(in ListenerIn) (err error) { + stopWatches, err := in.Svc.StartListener(in.listenerConfig, setLoggerInContext(), in.Measures, in.Watcher) if err != nil { - return listener, fmt.Errorf("webhook service start listener error: %v", err) + return fmt.Errorf("webhook service start listener error: %v", err) } in.LC.Append(fx.StopHook(stopWatches)) - return listener, nil + return nil }, ), ) diff --git a/service_test.go b/service_test.go index e87a66a..1d7f894 100644 --- a/service_test.go +++ b/service_test.go @@ -62,16 +62,14 @@ func TestStartListener(t *testing.T) { tcs := []struct { desc string serviceConfig Config - listenerConfig ListenerConfig + listenerConfig chrysom.ListenerConfig svc ClientService expectedErr bool }{ { - desc: "Success Case", - svc: *mockService, - listenerConfig: ListenerConfig{ - Config: chrysom.ListenerClientConfig{}, - }, + desc: "Success Case", + svc: *mockService, + listenerConfig: chrysom.ListenerConfig{}, }, { desc: "Chrysom Listener Client Creation Failure", @@ -81,7 +79,7 @@ func TestStartListener(t *testing.T) { for _, tc := range tcs { t.Run(tc.desc, func(t *testing.T) { assert := assert.New(t) - _, err := tc.svc.StartListener(tc.listenerConfig, nil) + _, err := tc.svc.StartListener(tc.listenerConfig, nil, chrysom.Measures{}) if tc.expectedErr { assert.NotNil(err) return From 012144b1b5e1064bd6746cb38c88cf70b80a66ef Mon Sep 17 00:00:00 2001 From: Owen Cabalceta Date: Thu, 7 Nov 2024 19:35:48 -0500 Subject: [PATCH 2/3] chore: typo --- service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service.go b/service.go index 1893a01..6840734 100644 --- a/service.go +++ b/service.go @@ -156,7 +156,7 @@ func prepArgusBasicClientConfig(cfg *Config) error { return nil } -func prepArgusListenerConfig(cfg *chrysom.ListenerConfig, metrics chrysom.Measures, watches ...Watch) chrysom.ListenerConfig { +func prepArgusListenerConfig(cfg *chrysom.ListenerConfig, metrics chrysom.Measures, watches ...Watch) { logger := cfg.Logger watches = append(watches, webhookListSizeWatch(metrics.WebhookListSizeGauge)) cfg.Listener = chrysom.ListenerFunc(func(items chrysom.Items) { From 0177d40d3d0d6a61a2bfee934fe563bcf5b83805 Mon Sep 17 00:00:00 2001 From: Owen Cabalceta Date: Fri, 8 Nov 2024 09:34:36 -0500 Subject: [PATCH 3/3] chore: fix `chrysom/listenerClient_test.go` tests --- chrysom/listenerClient_test.go | 4 ++-- service_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/chrysom/listenerClient_test.go b/chrysom/listenerClient_test.go index 741be42..ca72525 100644 --- a/chrysom/listenerClient_test.go +++ b/chrysom/listenerClient_test.go @@ -116,7 +116,7 @@ func newStartStopClient(includeListener bool) (*ListenerClient, func(), error) { if includeListener { config.Listener = mockListener } - client, err := NewListenerClient(config, nil, mockMeasures, &BasicClient{}) + client, err := NewListenerClient(config, sallust.With, mockMeasures, &BasicClient{}) if err != nil { return nil, nil, err } @@ -153,7 +153,7 @@ func TestNewListenerClient(t *testing.T) { for _, tc := range tcs { t.Run(tc.desc, func(t *testing.T) { assert := assert.New(t) - _, err := NewListenerClient(tc.config, nil, tc.measures, tc.reader) + _, err := NewListenerClient(tc.config, sallust.With, tc.measures, tc.reader) assert.True(errors.Is(err, tc.expectedErr), fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain", err, tc.expectedErr), diff --git a/service_test.go b/service_test.go index 1d7f894..42eba9d 100644 --- a/service_test.go +++ b/service_test.go @@ -79,7 +79,7 @@ func TestStartListener(t *testing.T) { for _, tc := range tcs { t.Run(tc.desc, func(t *testing.T) { assert := assert.New(t) - _, err := tc.svc.StartListener(tc.listenerConfig, nil, chrysom.Measures{}) + _, err := tc.svc.StartListener(tc.listenerConfig, sallust.With, chrysom.Measures{}) if tc.expectedErr { assert.NotNil(err) return