From 4f4a14acd91e2a9ef9b7a825da77dcfd54ee885e Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 13:55:38 +0100 Subject: [PATCH 1/8] FFM-11660 Add size and clearing interval to safe seen targets FFM-11660 Update tests FFM-11660 Make seen targets max size and clearing schedule configurable with defaults FFM-11660 Change level of bucketby missing log to debug FFM-11660 Add size and clearing interval to safe seen targets --- analyticsservice/analytics.go | 11 ++- analyticsservice/analytics_test.go | 2 +- analyticsservice/safe_maps_test.go | 88 ++++++++++++++++++++++- analyticsservice/safe_seen_targets_map.go | 49 +++++++++++-- client/client.go | 2 +- client/config.go | 79 ++++++++++---------- client/options.go | 22 +++++- evaluation/util.go | 2 +- 8 files changed, 203 insertions(+), 52 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 9caf6ccc..0b9204bf 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -46,6 +46,13 @@ type SafeAnalyticsCache[K comparable, V any] interface { iterate(func(K, V)) } +// SafeSeenTargetsCache extends SafeAnalyticsCache and adds behavior specific to seen targets +type SafeSeenTargetsCache[K comparable, V any] interface { + SafeAnalyticsCache[K, V] + setWithLimit(key K, value V) + isLimitExceeded() bool +} + type analyticsEvent struct { target *evaluation.Target featureConfig *rest.FeatureConfig @@ -68,7 +75,7 @@ type AnalyticsService struct { } // NewAnalyticsService creates and starts a analytics service to send data to the client -func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *AnalyticsService { +func NewAnalyticsService(timeout time.Duration, logger logger.Logger, seenTargetsMaxSize int, seenTargetsClearingSchedule time.Duration) *AnalyticsService { serviceTimeout := timeout if timeout < 60*time.Second { serviceTimeout = 60 * time.Second @@ -79,7 +86,7 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics analyticsChan: make(chan analyticsEvent), evaluationAnalytics: newSafeEvaluationAnalytics(), targetAnalytics: newSafeTargetAnalytics(), - seenTargets: newSafeSeenTargets(), + seenTargets: newSafeSeenTargets(seenTargetsMaxSize, seenTargetsClearingSchedule), timeout: serviceTimeout, logger: logger, } diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 07dd7e2f..bccda2ec 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -120,7 +120,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - service := NewAnalyticsService(1*time.Minute, noOpLogger) + service := NewAnalyticsService(1*time.Minute, noOpLogger, 10, time.Hour) defer close(service.analyticsChan) // Start the listener in a goroutine diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index fb25e1d9..9d9af2ad 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -1,6 +1,7 @@ package analyticsservice import ( + "fmt" "reflect" "sync" "testing" @@ -81,13 +82,94 @@ func TestSafeTargetAnalytics(t *testing.T) { } func TestSafeSeenTargets(t *testing.T) { - s := newSafeSeenTargets() + // Initialize with a small maxSize for testing + maxSize := 3 + s := newSafeSeenTargets(maxSize, 0).(SafeSeenTargetsCache[string, bool]) + testData := map[string]bool{ "target1": true, "target21": true, "target3": true, - "target4": true, } - testMapOperations[string, bool](t, s, testData) + // Insert items and ensure limit is not exceeded + for key, value := range testData { + s.set(key, value) + } + + if s.isLimitExceeded() { + t.Errorf("Limit should not have been exceeded yet") + } + + // Add one more item to exceed the limit + s.setWithLimit("target4", true) + + // Ensure limitExceeded is true after exceeding the limit + if !s.isLimitExceeded() { + t.Errorf("Limit should be exceeded after adding target4") + } + + // Ensure that new items are not added once the limit is exceeded + s.setWithLimit("target5", true) + if _, exists := s.get("target5"); exists { + t.Errorf("target5 should not have been added as the limit was exceeded") + } + + // Clear the map and ensure limit is reset + s.clear() + + if s.isLimitExceeded() { + t.Errorf("Limit should have been reset after clearing the map") + } + + // Add items again after clearing + s.setWithLimit("target6", true) + if _, exists := s.get("target6"); !exists { + t.Errorf("target6 should have been added after clearing the map") + } + + // Concurrency test + t.Run("ConcurrencyTest", func(t *testing.T) { + var wg sync.WaitGroup + concurrencyLevel := 100 + + // Re-initialize the map for concurrency testing + s = newSafeSeenTargets(100, 0).(SafeSeenTargetsCache[string, bool]) + + // Concurrently set keys + for i := 0; i < concurrencyLevel; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + key := "target" + fmt.Sprint(i) + s.setWithLimit(key, true) + }(i) + } + + // Concurrently get keys + for i := 0; i < concurrencyLevel; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + key := "target" + fmt.Sprint(i) + s.get(key) + }(i) + } + + // Concurrently clear the map + for i := 0; i < concurrencyLevel/2; i++ { + wg.Add(1) + go func() { + defer wg.Done() + s.clear() + }() + } + + wg.Wait() + + // Ensure the map is cleared after the concurrency operations + if s.size() > 0 { + t.Errorf("Map size should be 0 after clearing, got %d", s.size()) + } + }) } diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 94e4a8d0..4dabcb3e 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -2,25 +2,59 @@ package analyticsservice import ( "sync" + "sync/atomic" + "time" ) type safeSeenTargets struct { sync.RWMutex - data map[string]bool + data map[string]bool + maxSize int + limitExceeded atomic.Bool + clearingTimer *time.Ticker } -func newSafeSeenTargets() SafeAnalyticsCache[string, bool] { - return &safeSeenTargets{ - data: make(map[string]bool), +// Implements SafeSeenTargetsCache +func newSafeSeenTargets(maxSize int, clearingInterval time.Duration) SafeSeenTargetsCache[string, bool] { + st := &safeSeenTargets{ + data: make(map[string]bool), + maxSize: maxSize, } + + if clearingInterval > 0 { + st.clearingTimer = time.NewTicker(clearingInterval) + + // Start a goroutine to clear the cache at a set interval + go func() { + for range st.clearingTimer.C { + st.clear() + } + }() + } + + return st } -func (s *safeSeenTargets) set(key string, seen bool) { +func (s *safeSeenTargets) setWithLimit(key string, seen bool) { + if s.limitExceeded.Load() { + return + } + s.Lock() defer s.Unlock() + + if len(s.data) >= s.maxSize { + s.limitExceeded.Store(true) + } + s.data[key] = seen } +// The regular set method just calls SetWithLimit +func (s *safeSeenTargets) set(key string, seen bool) { + s.setWithLimit(key, seen) +} + func (s *safeSeenTargets) get(key string) (bool, bool) { s.RLock() defer s.RUnlock() @@ -44,6 +78,7 @@ func (s *safeSeenTargets) clear() { s.Lock() defer s.Unlock() s.data = make(map[string]bool) + s.limitExceeded.Store(false) } func (s *safeSeenTargets) iterate(f func(string, bool)) { @@ -53,3 +88,7 @@ func (s *safeSeenTargets) iterate(f func(string, bool)) { f(key, value) } } + +func (s *safeSeenTargets) isLimitExceeded() bool { + return s.limitExceeded.Load() +} diff --git a/client/client.go b/client/client.go index 60679e07..efc7f9db 100644 --- a/client/client.go +++ b/client/client.go @@ -79,7 +79,7 @@ func NewCfClient(sdkKey string, options ...ConfigOption) (*CfClient, error) { opt(config) } - analyticsService := analyticsservice.NewAnalyticsService(time.Minute, config.Logger) + analyticsService := analyticsservice.NewAnalyticsService(time.Minute, config.Logger, config.seenTargetsMaxSize, config.seenTargetsClearInterval) client := &CfClient{ sdkKey: sdkKey, diff --git a/client/config.go b/client/config.go index c3fcb273..dc221896 100644 --- a/client/config.go +++ b/client/config.go @@ -18,26 +18,28 @@ import ( ) type config struct { - url string - eventsURL string - pullInterval uint // in seconds - Cache cache.Cache - Store storage.Storage - Logger logger.Logger - httpClient *http.Client - authHttpClient *http.Client - enableStream bool - enableStore bool - target evaluation.Target - eventStreamListener stream.EventStreamListener - enableAnalytics bool - proxyMode bool - waitForInitialized bool - maxAuthRetries int - authRetryStrategy *backoff.ExponentialBackOff - streamingRetryStrategy *backoff.ExponentialBackOff - sleeper types.Sleeper - apiConfig *apiConfiguration + url string + eventsURL string + pullInterval uint // in seconds + Cache cache.Cache + Store storage.Storage + Logger logger.Logger + httpClient *http.Client + authHttpClient *http.Client + enableStream bool + enableStore bool + target evaluation.Target + eventStreamListener stream.EventStreamListener + enableAnalytics bool + proxyMode bool + waitForInitialized bool + maxAuthRetries int + authRetryStrategy *backoff.ExponentialBackOff + streamingRetryStrategy *backoff.ExponentialBackOff + sleeper types.Sleeper + apiConfig *apiConfiguration + seenTargetsMaxSize int + seenTargetsClearInterval time.Duration } type apiConfiguration struct { @@ -87,24 +89,25 @@ func newDefaultConfig(log logger.Logger) *config { } return &config{ - url: "https://config.ff.harness.io/api/1.0", - eventsURL: "https://events.ff.harness.io/api/1.0", - pullInterval: 60, - Cache: defaultCache, - Store: defaultStore, - Logger: log, - authHttpClient: authHttpClient, - httpClient: requestHttpClient.StandardClient(), - enableStream: true, - enableStore: true, - enableAnalytics: true, - proxyMode: false, - // Indicate that we should retry forever by default - maxAuthRetries: -1, - authRetryStrategy: getDefaultExpBackoff(), - streamingRetryStrategy: getDefaultExpBackoff(), - sleeper: &types.RealClock{}, - apiConfig: apiConfig, + url: "https://config.ff.harness.io/api/1.0", + eventsURL: "https://events.ff.harness.io/api/1.0", + pullInterval: 60, + Cache: defaultCache, + Store: defaultStore, + Logger: log, + authHttpClient: authHttpClient, + httpClient: requestHttpClient.StandardClient(), + enableStream: true, + enableStore: true, + enableAnalytics: true, + proxyMode: false, + maxAuthRetries: -1, // Indicate that we should retry forever by default + authRetryStrategy: getDefaultExpBackoff(), + streamingRetryStrategy: getDefaultExpBackoff(), + sleeper: &types.RealClock{}, + apiConfig: apiConfig, + seenTargetsMaxSize: 500000, + seenTargetsClearInterval: 24 * time.Hour, } } diff --git a/client/options.go b/client/options.go index dc73b23c..fc9a037f 100644 --- a/client/options.go +++ b/client/options.go @@ -1,6 +1,9 @@ package client import ( + "net/http" + "time" + "github.com/cenkalti/backoff/v4" "github.com/harness/ff-golang-server-sdk/cache" "github.com/harness/ff-golang-server-sdk/evaluation" @@ -8,7 +11,6 @@ import ( "github.com/harness/ff-golang-server-sdk/storage" "github.com/harness/ff-golang-server-sdk/stream" "github.com/harness/ff-golang-server-sdk/types" - "net/http" ) // ConfigOption is used as return value for advanced client configuration @@ -142,3 +144,21 @@ func WithSleeper(sleeper types.Sleeper) ConfigOption { config.sleeper = sleeper } } + +// WithSeenTargetsMaxSize sets the maximum size for the seen targets map. +// The SeenTargetsCache helps to reduce the size of the analytics payload that the SDK sends to the Feature Flags Service. +// This method allows you to set the maximum number of unique targets that will be stored in the SeenTargets cache. +// By default, the limit is set to 500,000 unique targets. You can increase this number if you need to handle more than +// 500,000 targets, which will reduce the payload size but will also increase memory usage. +func WithSeenTargetsMaxSize(maxSize int) ConfigOption { + return func(config *config) { + config.seenTargetsMaxSize = maxSize + } +} + +// WithSeenTargetsClearInterval sets the clearing interval for the seen targets map. +func WithSeenTargetsClearInterval(interval time.Duration) ConfigOption { + return func(config *config) { + config.seenTargetsClearInterval = interval + } +} diff --git a/evaluation/util.go b/evaluation/util.go index dddb17be..25501390 100644 --- a/evaluation/util.go +++ b/evaluation/util.go @@ -80,7 +80,7 @@ func isEnabled(target *Target, bucketBy string, percentage int) bool { if value == "" { return false } - log.Warnf("%s BucketBy attribute not found in target attributes, falling back to 'identifier': missing=%s, using value=%s", sdk_codes.MissingBucketBy, bucketBy, value) + log.Debugf("%s BucketBy attribute not found in target attributes, falling back to 'identifier': missing=%s, using value=%s", sdk_codes.MissingBucketBy, bucketBy, value) bucketBy = "identifier" } From 35422d5faff2a690e9c10ff029bc6aa52f2fcd5e Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 16:40:59 +0100 Subject: [PATCH 2/8] FFM-11660 Test for safe maps --- analyticsservice/analytics.go | 11 +++++++++-- analyticsservice/safe_maps_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 0b9204bf..93e2a0ac 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -65,7 +65,7 @@ type AnalyticsService struct { analyticsChan chan analyticsEvent evaluationAnalytics SafeAnalyticsCache[string, analyticsEvent] targetAnalytics SafeAnalyticsCache[string, evaluation.Target] - seenTargets SafeAnalyticsCache[string, bool] + seenTargets SafeSeenTargetsCache[string, bool] logEvaluationLimitReached atomic.Bool logTargetLimitReached atomic.Bool timeout time.Duration @@ -162,8 +162,15 @@ func (as *AnalyticsService) listener() { continue } + // Check if seen targets limit has been hit + limitExceeded := as.seenTargets.isLimitExceeded() + + if limitExceeded { + continue + } + // Update seen targets - as.seenTargets.set(ad.target.Identifier, true) + as.seenTargets.setWithLimit(ad.target.Identifier, true) // Update target metrics if as.targetAnalytics.size() < maxTargetEntries { diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 9d9af2ad..97ab77b7 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -5,6 +5,7 @@ import ( "reflect" "sync" "testing" + "time" "github.com/harness/ff-golang-server-sdk/evaluation" ) @@ -172,4 +173,32 @@ func TestSafeSeenTargets(t *testing.T) { t.Errorf("Map size should be 0 after clearing, got %d", s.size()) } }) + + // Add test for clearing based on interval + t.Run("IntervalClearingTest", func(t *testing.T) { + // Re-initialize the map with a clearing interval + s = newSafeSeenTargets(10, 100*time.Millisecond) + + for key, value := range testData { + s.set(key, value) + } + + // Ensure the map has items initially + if s.size() != len(testData) { + t.Errorf("Expected map size to be %d, got %d", len(testData), s.size()) + } + + // Wait for the clearing to clear the map + time.Sleep(300 * time.Millisecond) + + // Ensure the map is cleared after the interval + if s.size() != 0 { + t.Errorf("Expected map size to be 0 after clearing interval, got %d", s.size()) + } + + // Ensure the limitExceeded flag is reset + if s.isLimitExceeded() { + t.Errorf("Expected limitExceeded to be reset after clearing") + } + }) } From 28b5889135a5f93bd4c8ed3d0e53a0f706b94e82 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 17:02:49 +0100 Subject: [PATCH 3/8] FFM-11660 Logic tweak --- analyticsservice/analytics.go | 8 ++------ analyticsservice/safe_seen_targets_map.go | 1 + 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 93e2a0ac..b0c1bfa0 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -156,16 +156,12 @@ func (as *AnalyticsService) listener() { } // Check if target has been seen - _, seen := as.seenTargets.get(ad.target.Identifier) - - if seen { + if _, seen := as.seenTargets.get(ad.target.Identifier); seen { continue } // Check if seen targets limit has been hit - limitExceeded := as.seenTargets.isLimitExceeded() - - if limitExceeded { + if as.seenTargets.isLimitExceeded() { continue } diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 4dabcb3e..e56ab00d 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -45,6 +45,7 @@ func (s *safeSeenTargets) setWithLimit(key string, seen bool) { if len(s.data) >= s.maxSize { s.limitExceeded.Store(true) + return } s.data[key] = seen From 0e189dab8627d3c77398ee0ee11446ac14741fb1 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 17:15:12 +0100 Subject: [PATCH 4/8] FFM-11660 Move seen targets clear interval to analytics FFM-11660 Add comment FFM-11660 Add Bump version FFM-11660 Add a stop clearing method --- .harness/ffgolangserversdk.yaml | 2 +- analyticsservice/analytics.go | 54 ++++++++++++++++------- analyticsservice/safe_seen_targets_map.go | 19 +------- client/options.go | 3 +- 4 files changed, 42 insertions(+), 36 deletions(-) diff --git a/.harness/ffgolangserversdk.yaml b/.harness/ffgolangserversdk.yaml index 96ee3334..4861a058 100644 --- a/.harness/ffgolangserversdk.yaml +++ b/.harness/ffgolangserversdk.yaml @@ -176,7 +176,7 @@ pipeline: dockerfile: ff-sdk-testgrid/go/Dockerfile context: ff-sdk-testgrid/go buildArgs: - SDK_VERSION: v0.1.24 + SDK_VERSION: v0.1.25 BUILD_MODE: local resources: limits: diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index b0c1bfa0..4cad9831 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -26,7 +26,7 @@ const ( variationValueAttribute string = "featureValue" targetAttribute string = "target" sdkVersionAttribute string = "SDK_VERSION" - SdkVersion string = "0.1.24" + SdkVersion string = "0.1.25" sdkTypeAttribute string = "SDK_TYPE" sdkType string = "server" sdkLanguageAttribute string = "SDK_LANGUAGE" @@ -62,16 +62,17 @@ type analyticsEvent struct { // AnalyticsService provides a way to cache and send analytics to the server type AnalyticsService struct { - analyticsChan chan analyticsEvent - evaluationAnalytics SafeAnalyticsCache[string, analyticsEvent] - targetAnalytics SafeAnalyticsCache[string, evaluation.Target] - seenTargets SafeSeenTargetsCache[string, bool] - logEvaluationLimitReached atomic.Bool - logTargetLimitReached atomic.Bool - timeout time.Duration - logger logger.Logger - metricsClient metricsclient.ClientWithResponsesInterface - environmentID string + analyticsChan chan analyticsEvent + evaluationAnalytics SafeAnalyticsCache[string, analyticsEvent] + targetAnalytics SafeAnalyticsCache[string, evaluation.Target] + seenTargets SafeSeenTargetsCache[string, bool] + logEvaluationLimitReached atomic.Bool + logTargetLimitReached atomic.Bool + timeout time.Duration + logger logger.Logger + metricsClient metricsclient.ClientWithResponsesInterface + environmentID string + seenTargetsClearingInterval time.Duration } // NewAnalyticsService creates and starts a analytics service to send data to the client @@ -83,12 +84,13 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger, seenTarget serviceTimeout = 1 * time.Hour } as := AnalyticsService{ - analyticsChan: make(chan analyticsEvent), - evaluationAnalytics: newSafeEvaluationAnalytics(), - targetAnalytics: newSafeTargetAnalytics(), - seenTargets: newSafeSeenTargets(seenTargetsMaxSize, seenTargetsClearingSchedule), - timeout: serviceTimeout, - logger: logger, + analyticsChan: make(chan analyticsEvent), + evaluationAnalytics: newSafeEvaluationAnalytics(), + targetAnalytics: newSafeTargetAnalytics(), + seenTargets: newSafeSeenTargets(seenTargetsMaxSize), + timeout: serviceTimeout, + logger: logger, + seenTargetsClearingInterval: seenTargetsClearingSchedule, } go as.listener() @@ -101,6 +103,7 @@ func (as *AnalyticsService) Start(ctx context.Context, client metricsclient.Clie as.metricsClient = client as.environmentID = environmentID go as.startTimer(ctx) + go as.startSeenTargetsClearingSchedule(ctx, as.seenTargetsClearingInterval) } func (as *AnalyticsService) startTimer(ctx context.Context) { @@ -110,6 +113,7 @@ func (as *AnalyticsService) startTimer(ctx context.Context) { timeStamp := time.Now().UnixNano() / (int64(time.Millisecond) / int64(time.Nanosecond)) as.sendDataAndResetCache(ctx, timeStamp) case <-ctx.Done(): + close(as.analyticsChan) as.logger.Infof("%s Metrics stopped", sdk_codes.MetricsStopped) return } @@ -324,6 +328,22 @@ func (as *AnalyticsService) processTargetMetrics(targetAnalytics SafeAnalyticsCa return targetData } +func (as *AnalyticsService) startSeenTargetsClearingSchedule(ctx context.Context, clearingInterval time.Duration) { + ticker := time.NewTicker(clearingInterval) + + for { + select { + case <-ticker.C: + as.logger.Infof("Clearing seen targets") + as.seenTargets.clear() + + case <-ctx.Done(): + ticker.Stop() + return + } + } +} + func getEvaluationAnalyticKey(event analyticsEvent) string { return fmt.Sprintf("%s-%s-%s-%s", event.featureConfig.Feature, event.variation.Identifier, event.variation.Value, globalTarget) } diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index e56ab00d..bf8c0830 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -3,7 +3,6 @@ package analyticsservice import ( "sync" "sync/atomic" - "time" ) type safeSeenTargets struct { @@ -11,28 +10,14 @@ type safeSeenTargets struct { data map[string]bool maxSize int limitExceeded atomic.Bool - clearingTimer *time.Ticker } // Implements SafeSeenTargetsCache -func newSafeSeenTargets(maxSize int, clearingInterval time.Duration) SafeSeenTargetsCache[string, bool] { - st := &safeSeenTargets{ +func newSafeSeenTargets(maxSize int) SafeSeenTargetsCache[string, bool] { + return &safeSeenTargets{ data: make(map[string]bool), maxSize: maxSize, } - - if clearingInterval > 0 { - st.clearingTimer = time.NewTicker(clearingInterval) - - // Start a goroutine to clear the cache at a set interval - go func() { - for range st.clearingTimer.C { - st.clear() - } - }() - } - - return st } func (s *safeSeenTargets) setWithLimit(key string, seen bool) { diff --git a/client/options.go b/client/options.go index fc9a037f..1a5c0807 100644 --- a/client/options.go +++ b/client/options.go @@ -156,7 +156,8 @@ func WithSeenTargetsMaxSize(maxSize int) ConfigOption { } } -// WithSeenTargetsClearInterval sets the clearing interval for the seen targets map. +// WithSeenTargetsClearInterval sets the clearing interval for the seen targets map. By default, the interval +// is set to 24 hours. func WithSeenTargetsClearInterval(interval time.Duration) ConfigOption { return func(config *config) { config.seenTargetsClearInterval = interval From 368d9788340bf6d0de7f006797c48091c7fdfd05 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 17:59:23 +0100 Subject: [PATCH 5/8] FFM-11660 Change info log to debug log --- analyticsservice/analytics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 4cad9831..8a391fd5 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -334,7 +334,7 @@ func (as *AnalyticsService) startSeenTargetsClearingSchedule(ctx context.Context for { select { case <-ticker.C: - as.logger.Infof("Clearing seen targets") + as.logger.Debugf("Clearing seen targets") as.seenTargets.clear() case <-ctx.Done(): From 751afa39b2e3af876273c5ddd50246d5cc8d1048 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 18:02:35 +0100 Subject: [PATCH 6/8] FFM-11660 Fix tests --- analyticsservice/safe_maps_test.go | 34 +++--------------------------- 1 file changed, 3 insertions(+), 31 deletions(-) diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 97ab77b7..00ae394f 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -5,7 +5,6 @@ import ( "reflect" "sync" "testing" - "time" "github.com/harness/ff-golang-server-sdk/evaluation" ) @@ -85,7 +84,7 @@ func TestSafeTargetAnalytics(t *testing.T) { func TestSafeSeenTargets(t *testing.T) { // Initialize with a small maxSize for testing maxSize := 3 - s := newSafeSeenTargets(maxSize, 0).(SafeSeenTargetsCache[string, bool]) + s := newSafeSeenTargets(maxSize).(SafeSeenTargetsCache[string, bool]) testData := map[string]bool{ "target1": true, @@ -135,7 +134,7 @@ func TestSafeSeenTargets(t *testing.T) { concurrencyLevel := 100 // Re-initialize the map for concurrency testing - s = newSafeSeenTargets(100, 0).(SafeSeenTargetsCache[string, bool]) + s = newSafeSeenTargets(100).(SafeSeenTargetsCache[string, bool]) // Concurrently set keys for i := 0; i < concurrencyLevel; i++ { @@ -173,32 +172,5 @@ func TestSafeSeenTargets(t *testing.T) { t.Errorf("Map size should be 0 after clearing, got %d", s.size()) } }) - - // Add test for clearing based on interval - t.Run("IntervalClearingTest", func(t *testing.T) { - // Re-initialize the map with a clearing interval - s = newSafeSeenTargets(10, 100*time.Millisecond) - - for key, value := range testData { - s.set(key, value) - } - - // Ensure the map has items initially - if s.size() != len(testData) { - t.Errorf("Expected map size to be %d, got %d", len(testData), s.size()) - } - - // Wait for the clearing to clear the map - time.Sleep(300 * time.Millisecond) - - // Ensure the map is cleared after the interval - if s.size() != 0 { - t.Errorf("Expected map size to be 0 after clearing interval, got %d", s.size()) - } - - // Ensure the limitExceeded flag is reset - if s.isLimitExceeded() { - t.Errorf("Expected limitExceeded to be reset after clearing") - } - }) + } From 8eff45c6da93c5808250fdd808858a0ff0564acc Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Fri, 6 Sep 2024 18:32:43 +0100 Subject: [PATCH 7/8] FFM-11660 Use set instead of set with limit --- analyticsservice/analytics.go | 3 +-- analyticsservice/safe_maps_test.go | 10 +++++----- analyticsservice/safe_seen_targets_map.go | 12 ++---------- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 8a391fd5..eb6ff040 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -49,7 +49,6 @@ type SafeAnalyticsCache[K comparable, V any] interface { // SafeSeenTargetsCache extends SafeAnalyticsCache and adds behavior specific to seen targets type SafeSeenTargetsCache[K comparable, V any] interface { SafeAnalyticsCache[K, V] - setWithLimit(key K, value V) isLimitExceeded() bool } @@ -170,7 +169,7 @@ func (as *AnalyticsService) listener() { } // Update seen targets - as.seenTargets.setWithLimit(ad.target.Identifier, true) + as.seenTargets.set(ad.target.Identifier, true) // Update target metrics if as.targetAnalytics.size() < maxTargetEntries { diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 00ae394f..6c7213c2 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -102,7 +102,7 @@ func TestSafeSeenTargets(t *testing.T) { } // Add one more item to exceed the limit - s.setWithLimit("target4", true) + s.set("target4", true) // Ensure limitExceeded is true after exceeding the limit if !s.isLimitExceeded() { @@ -110,7 +110,7 @@ func TestSafeSeenTargets(t *testing.T) { } // Ensure that new items are not added once the limit is exceeded - s.setWithLimit("target5", true) + s.set("target5", true) if _, exists := s.get("target5"); exists { t.Errorf("target5 should not have been added as the limit was exceeded") } @@ -123,7 +123,7 @@ func TestSafeSeenTargets(t *testing.T) { } // Add items again after clearing - s.setWithLimit("target6", true) + s.set("target6", true) if _, exists := s.get("target6"); !exists { t.Errorf("target6 should have been added after clearing the map") } @@ -142,7 +142,7 @@ func TestSafeSeenTargets(t *testing.T) { go func(i int) { defer wg.Done() key := "target" + fmt.Sprint(i) - s.setWithLimit(key, true) + s.set(key, true) }(i) } @@ -172,5 +172,5 @@ func TestSafeSeenTargets(t *testing.T) { t.Errorf("Map size should be 0 after clearing, got %d", s.size()) } }) - + } diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index bf8c0830..2c199e9b 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -20,11 +20,8 @@ func newSafeSeenTargets(maxSize int) SafeSeenTargetsCache[string, bool] { } } -func (s *safeSeenTargets) setWithLimit(key string, seen bool) { - if s.limitExceeded.Load() { - return - } - +// The regular set method just calls SetWithLimit +func (s *safeSeenTargets) set(key string, seen bool) { s.Lock() defer s.Unlock() @@ -36,11 +33,6 @@ func (s *safeSeenTargets) setWithLimit(key string, seen bool) { s.data[key] = seen } -// The regular set method just calls SetWithLimit -func (s *safeSeenTargets) set(key string, seen bool) { - s.setWithLimit(key, seen) -} - func (s *safeSeenTargets) get(key string) (bool, bool) { s.RLock() defer s.RUnlock() From 63c9a10fb22c4d5db8365457c6f7192f50f46b67 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Mon, 9 Sep 2024 11:45:16 +0100 Subject: [PATCH 8/8] FFM-11660 Remove comments --- analyticsservice/safe_seen_targets_map.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 2c199e9b..6b580f8b 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -12,7 +12,6 @@ type safeSeenTargets struct { limitExceeded atomic.Bool } -// Implements SafeSeenTargetsCache func newSafeSeenTargets(maxSize int) SafeSeenTargetsCache[string, bool] { return &safeSeenTargets{ data: make(map[string]bool), @@ -20,7 +19,6 @@ func newSafeSeenTargets(maxSize int) SafeSeenTargetsCache[string, bool] { } } -// The regular set method just calls SetWithLimit func (s *safeSeenTargets) set(key string, seen bool) { s.Lock() defer s.Unlock()