From 6ed8dabe2c56c46ae662ee930857194659f47a27 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 17:41:30 +0100 Subject: [PATCH 01/44] FFM-11212 Start splitting metrics caches --- analyticsservice/analytics.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index e3b26b8..3c1c2a1 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -46,6 +46,8 @@ type AnalyticsService struct { mx *sync.Mutex analyticsChan chan analyticsEvent analyticsData map[string]analyticsEvent + targetMetrics map[string]evaluation.Target + seenTargets map[string]bool timeout time.Duration logger logger.Logger metricsClient *metricsclient.ClientWithResponsesInterface @@ -64,6 +66,8 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics mx: &sync.Mutex{}, analyticsChan: make(chan analyticsEvent), analyticsData: map[string]analyticsEvent{}, + targetMetrics: map[string]evaluation.Target{}, + seenTargets: map[string]bool{}, timeout: serviceTimeout, logger: logger, } From 1c25d71f851067855449a8e8e10a876767651af5 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 17:48:03 +0100 Subject: [PATCH 02/44] FFM-11212 Start splitting metrics caches --- analyticsservice/analytics.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 3c1c2a1..22bae64 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -32,6 +32,8 @@ const ( sdkLanguageAttribute string = "SDK_LANGUAGE" sdkLanguage string = "go" globalTarget string = "global" + maxAnalyticsEntries int = 10000 + maxTargetEntries int = 100000 ) type analyticsEvent struct { @@ -113,6 +115,12 @@ func (as *AnalyticsService) listener() { key := getEventSummaryKey(ad) as.mx.Lock() + + if !as.seenTargets[ad.target.Identifier] { + as.seenTargets[ad.target.Identifier] = true + as.targetMetrics[ad.target.Identifier] = *ad.target + } + analytic, ok := as.analyticsData[key] if !ok { ad.count = 1 From 125f743247c02d74f3c6b091a4eace936b425630 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:00:05 +0100 Subject: [PATCH 03/44] FFM-11212 Start splitting metrics caches --- analyticsservice/analytics.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 22bae64..5e6da3e 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -45,7 +45,9 @@ type analyticsEvent struct { // AnalyticsService provides a way to cache and send analytics to the server type AnalyticsService struct { - mx *sync.Mutex + analyticsMx *sync.Mutex + targetsMx *sync.Mutex + seenTargetsMx *sync.RWMutex analyticsChan chan analyticsEvent analyticsData map[string]analyticsEvent targetMetrics map[string]evaluation.Target @@ -65,7 +67,7 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics serviceTimeout = 1 * time.Hour } as := AnalyticsService{ - mx: &sync.Mutex{}, + analyticsMx: &sync.Mutex{}, analyticsChan: make(chan analyticsEvent), analyticsData: map[string]analyticsEvent{}, targetMetrics: map[string]evaluation.Target{}, @@ -112,15 +114,16 @@ func (as *AnalyticsService) PushToQueue(featureConfig *rest.FeatureConfig, targe func (as *AnalyticsService) listener() { as.logger.Info("Analytics cache successfully initialized") for ad := range as.analyticsChan { - key := getEventSummaryKey(ad) - - as.mx.Lock() - + //as.analyticsMx.Lock() + as.targetsMx.Lock() if !as.seenTargets[ad.target.Identifier] { as.seenTargets[ad.target.Identifier] = true as.targetMetrics[ad.target.Identifier] = *ad.target } + as.targetsMx.Unlock() + key := getEventSummaryKey(ad) + as.analyticsMx.Lock() analytic, ok := as.analyticsData[key] if !ok { ad.count = 1 @@ -129,7 +132,7 @@ func (as *AnalyticsService) listener() { ad.count = (analytic.count + 1) as.analyticsData[key] = ad } - as.mx.Unlock() + as.analyticsMx.Unlock() } } @@ -162,14 +165,14 @@ func convertInterfaceToString(i interface{}) string { } func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - as.mx.Lock() + as.analyticsMx.Lock() // copy cache to send to server analyticsData := as.analyticsData // clear cache. As metrics is secondary to the flags, we do it this way // so it doesn't effect the performance of our users code. Even if it means // we lose metrics the odd time. as.analyticsData = map[string]analyticsEvent{} - as.mx.Unlock() + as.analyticsMx.Unlock() metricData := make([]metricsclient.MetricsData, 0, len(as.analyticsData)) targetData := map[string]metricsclient.TargetData{} From 133d5c52703c6d32761e24b4387f6b136142236c Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:14:09 +0100 Subject: [PATCH 04/44] FFM-11212 Use separate locks per map --- analyticsservice/analytics.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 5e6da3e..4b0dd42 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -114,15 +114,8 @@ func (as *AnalyticsService) PushToQueue(featureConfig *rest.FeatureConfig, targe func (as *AnalyticsService) listener() { as.logger.Info("Analytics cache successfully initialized") for ad := range as.analyticsChan { - //as.analyticsMx.Lock() - as.targetsMx.Lock() - if !as.seenTargets[ad.target.Identifier] { - as.seenTargets[ad.target.Identifier] = true - as.targetMetrics[ad.target.Identifier] = *ad.target - } - as.targetsMx.Unlock() - key := getEventSummaryKey(ad) + as.analyticsMx.Lock() analytic, ok := as.analyticsData[key] if !ok { @@ -133,6 +126,21 @@ func (as *AnalyticsService) listener() { as.analyticsData[key] = ad } as.analyticsMx.Unlock() + + as.seenTargetsMx.RLock() + _, seen := as.seenTargets[ad.target.Identifier] + as.seenTargetsMx.RUnlock() + + if !seen { + // Write to seenTargets + as.seenTargetsMx.Lock() + as.seenTargets[ad.target.Identifier] = true + as.seenTargetsMx.Unlock() + + as.targetsMx.Lock() + as.targetMetrics[ad.target.Identifier] = *ad.target + as.targetsMx.Unlock() + } } } From b0d66f184e60f26440de6d2ea11c7b3dfc4b2ed7 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:16:42 +0100 Subject: [PATCH 05/44] FFM-11212 Comments for clarity --- analyticsservice/analytics.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 4b0dd42..862a341 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -114,29 +114,32 @@ func (as *AnalyticsService) PushToQueue(featureConfig *rest.FeatureConfig, targe func (as *AnalyticsService) listener() { as.logger.Info("Analytics cache successfully initialized") for ad := range as.analyticsChan { - key := getEventSummaryKey(ad) + analyticsKey := getEventSummaryKey(ad) + // Update evaluation metrics as.analyticsMx.Lock() - analytic, ok := as.analyticsData[key] + analytic, ok := as.analyticsData[analyticsKey] if !ok { ad.count = 1 - as.analyticsData[key] = ad + as.analyticsData[analyticsKey] = ad } else { ad.count = (analytic.count + 1) - as.analyticsData[key] = ad + as.analyticsData[analyticsKey] = ad } as.analyticsMx.Unlock() + // Check if target has been seen as.seenTargetsMx.RLock() _, seen := as.seenTargets[ad.target.Identifier] as.seenTargetsMx.RUnlock() if !seen { - // Write to seenTargets + // Update seen targets as.seenTargetsMx.Lock() as.seenTargets[ad.target.Identifier] = true as.seenTargetsMx.Unlock() + // Update target metrics as.targetsMx.Lock() as.targetMetrics[ad.target.Identifier] = *ad.target as.targetsMx.Unlock() From 1b56990a59925c9d969d779546aecec46f2602c5 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:19:34 +0100 Subject: [PATCH 06/44] FFM-11212 Early return --- analyticsservice/analytics.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 862a341..b18cff9 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -133,17 +133,19 @@ func (as *AnalyticsService) listener() { _, seen := as.seenTargets[ad.target.Identifier] as.seenTargetsMx.RUnlock() - if !seen { - // Update seen targets - as.seenTargetsMx.Lock() - as.seenTargets[ad.target.Identifier] = true - as.seenTargetsMx.Unlock() - - // Update target metrics - as.targetsMx.Lock() - as.targetMetrics[ad.target.Identifier] = *ad.target - as.targetsMx.Unlock() + if seen { + return } + + // Update seen targets + as.seenTargetsMx.Lock() + as.seenTargets[ad.target.Identifier] = true + as.seenTargetsMx.Unlock() + + // Update target metrics + as.targetsMx.Lock() + as.targetMetrics[ad.target.Identifier] = *ad.target + as.targetsMx.Unlock() } } From a96c1072b3451e34ff2fbe91c9387a3cee4e79c3 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:19:59 +0100 Subject: [PATCH 07/44] FFM-11212 Change return to continue --- analyticsservice/analytics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index b18cff9..bd9bb02 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -134,7 +134,7 @@ func (as *AnalyticsService) listener() { as.seenTargetsMx.RUnlock() if seen { - return + continue } // Update seen targets From 690d8f41a5747b9a16d1f065a8a2904614d07abd Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:27:47 +0100 Subject: [PATCH 08/44] FFM-11212 Init locks --- analyticsservice/analytics.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index bd9bb02..bbfd463 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -68,6 +68,8 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics } as := AnalyticsService{ analyticsMx: &sync.Mutex{}, + targetsMx: &sync.Mutex{}, + seenTargetsMx: &sync.RWMutex{}, analyticsChan: make(chan analyticsEvent), analyticsData: map[string]analyticsEvent{}, targetMetrics: map[string]evaluation.Target{}, From 0f1512d8a0ef393957ea715de6e85bd7bdc36e49 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:46:15 +0100 Subject: [PATCH 09/44] FFM-11212 Rename and reorg --- analyticsservice/analytics.go | 68 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index bbfd463..6096043 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -45,17 +45,17 @@ type analyticsEvent struct { // AnalyticsService provides a way to cache and send analytics to the server type AnalyticsService struct { - analyticsMx *sync.Mutex - targetsMx *sync.Mutex - seenTargetsMx *sync.RWMutex - analyticsChan chan analyticsEvent - analyticsData map[string]analyticsEvent - targetMetrics map[string]evaluation.Target - seenTargets map[string]bool - timeout time.Duration - logger logger.Logger - metricsClient *metricsclient.ClientWithResponsesInterface - environmentID string + analyticsChan chan analyticsEvent + evaluationsAnalyticsMx *sync.Mutex + targetAnalyticsMx *sync.Mutex + seenTargetsMx *sync.RWMutex + evaluationAnalytics map[string]analyticsEvent + targetAnalytics map[string]evaluation.Target + seenTargets map[string]bool + timeout time.Duration + logger logger.Logger + metricsClient *metricsclient.ClientWithResponsesInterface + environmentID string } // NewAnalyticsService creates and starts a analytics service to send data to the client @@ -67,15 +67,15 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics serviceTimeout = 1 * time.Hour } as := AnalyticsService{ - analyticsMx: &sync.Mutex{}, - targetsMx: &sync.Mutex{}, - seenTargetsMx: &sync.RWMutex{}, - analyticsChan: make(chan analyticsEvent), - analyticsData: map[string]analyticsEvent{}, - targetMetrics: map[string]evaluation.Target{}, - seenTargets: map[string]bool{}, - timeout: serviceTimeout, - logger: logger, + evaluationsAnalyticsMx: &sync.Mutex{}, + targetAnalyticsMx: &sync.Mutex{}, + seenTargetsMx: &sync.RWMutex{}, + analyticsChan: make(chan analyticsEvent), + evaluationAnalytics: map[string]analyticsEvent{}, + targetAnalytics: map[string]evaluation.Target{}, + seenTargets: map[string]bool{}, + timeout: serviceTimeout, + logger: logger, } go as.listener() @@ -119,16 +119,16 @@ func (as *AnalyticsService) listener() { analyticsKey := getEventSummaryKey(ad) // Update evaluation metrics - as.analyticsMx.Lock() - analytic, ok := as.analyticsData[analyticsKey] + as.evaluationsAnalyticsMx.Lock() + analytic, ok := as.evaluationAnalytics[analyticsKey] if !ok { ad.count = 1 - as.analyticsData[analyticsKey] = ad + as.evaluationAnalytics[analyticsKey] = ad } else { ad.count = (analytic.count + 1) - as.analyticsData[analyticsKey] = ad + as.evaluationAnalytics[analyticsKey] = ad } - as.analyticsMx.Unlock() + as.evaluationsAnalyticsMx.Unlock() // Check if target has been seen as.seenTargetsMx.RLock() @@ -145,9 +145,9 @@ func (as *AnalyticsService) listener() { as.seenTargetsMx.Unlock() // Update target metrics - as.targetsMx.Lock() - as.targetMetrics[ad.target.Identifier] = *ad.target - as.targetsMx.Unlock() + as.targetAnalyticsMx.Lock() + as.targetAnalytics[ad.target.Identifier] = *ad.target + as.targetAnalyticsMx.Unlock() } } @@ -180,16 +180,16 @@ func convertInterfaceToString(i interface{}) string { } func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - as.analyticsMx.Lock() - // copy cache to send to server - analyticsData := as.analyticsData + as.evaluationsAnalyticsMx.Lock() + // Copy a + analyticsData := as.evaluationAnalytics // clear cache. As metrics is secondary to the flags, we do it this way // so it doesn't effect the performance of our users code. Even if it means // we lose metrics the odd time. - as.analyticsData = map[string]analyticsEvent{} - as.analyticsMx.Unlock() + as.evaluationAnalytics = map[string]analyticsEvent{} + as.evaluationsAnalyticsMx.Unlock() - metricData := make([]metricsclient.MetricsData, 0, len(as.analyticsData)) + metricData := make([]metricsclient.MetricsData, 0, len(as.evaluationAnalytics)) targetData := map[string]metricsclient.TargetData{} for _, analytic := range analyticsData { From 973f81d7fb04d44fb2123e77400cde10df29a6e6 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 18:50:00 +0100 Subject: [PATCH 10/44] FFM-11212 Copy and clear targets --- analyticsservice/analytics.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 6096043..498b137 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -180,19 +180,25 @@ func convertInterfaceToString(i interface{}) string { } func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - as.evaluationsAnalyticsMx.Lock() - // Copy a - analyticsData := as.evaluationAnalytics - // clear cache. As metrics is secondary to the flags, we do it this way - // so it doesn't effect the performance of our users code. Even if it means + + // Copy and reset evaluation analytics cache. As metrics is secondary to the flags, we do it this way + // so it doesn't affect the performance of our users code. Even if it means // we lose metrics the odd time. + as.evaluationsAnalyticsMx.Lock() + evaluationAnalytics := as.evaluationAnalytics as.evaluationAnalytics = map[string]analyticsEvent{} as.evaluationsAnalyticsMx.Unlock() + // Copy and reset target analytics cache + as.targetAnalyticsMx.Lock() + targetAnalytics := as.targetAnalytics + as.targetAnalytics = make(map[string]evaluation.Target) + as.targetAnalyticsMx.Unlock() + metricData := make([]metricsclient.MetricsData, 0, len(as.evaluationAnalytics)) targetData := map[string]metricsclient.TargetData{} - for _, analytic := range analyticsData { + for _, analytic := range evaluationAnalytics { if analytic.target != nil { if analytic.target.Anonymous == nil || !*analytic.target.Anonymous { targetAttributes := make([]metricsclient.KeyValue, 0) From c29b8e7ddb35a3ced7b574ead2d4c4472830f15f Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:00:21 +0100 Subject: [PATCH 11/44] FFM-11212 Refactor sendDataAndResetCache --- analyticsservice/analytics.go | 109 ++++++++++------------------------ 1 file changed, 32 insertions(+), 77 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 498b137..0f57ff8 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -181,90 +181,36 @@ func convertInterfaceToString(i interface{}) string { func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - // Copy and reset evaluation analytics cache. As metrics is secondary to the flags, we do it this way + // Clone and reset evaluation analytics cache. As metrics is secondary to the flags, we do it this way // so it doesn't affect the performance of our users code. Even if it means // we lose metrics the odd time. as.evaluationsAnalyticsMx.Lock() - evaluationAnalytics := as.evaluationAnalytics + evaluationAnalyticsClone := as.evaluationAnalytics as.evaluationAnalytics = map[string]analyticsEvent{} as.evaluationsAnalyticsMx.Unlock() - // Copy and reset target analytics cache + // Clone and reset target analytics cache as.targetAnalyticsMx.Lock() - targetAnalytics := as.targetAnalytics + targetAnalyticsClone := as.targetAnalytics as.targetAnalytics = make(map[string]evaluation.Target) as.targetAnalyticsMx.Unlock() - metricData := make([]metricsclient.MetricsData, 0, len(as.evaluationAnalytics)) - targetData := map[string]metricsclient.TargetData{} - - for _, analytic := range evaluationAnalytics { - if analytic.target != nil { - if analytic.target.Anonymous == nil || !*analytic.target.Anonymous { - targetAttributes := make([]metricsclient.KeyValue, 0) - if analytic.target.Attributes != nil { - targetAttributes = make([]metricsclient.KeyValue, 0, len(*analytic.target.Attributes)) - for key, value := range *analytic.target.Attributes { - v := convertInterfaceToString(value) - kv := metricsclient.KeyValue{ - Key: key, - Value: v, - } - targetAttributes = append(targetAttributes, kv) - } - - } - - targetName := analytic.target.Identifier - if analytic.target.Name != "" { - targetName = analytic.target.Name - } - - td := metricsclient.TargetData{ - Name: targetName, - Identifier: analytic.target.Identifier, - Attributes: targetAttributes, - } - targetData[analytic.target.Identifier] = td - } - } + metricData := make([]metricsclient.MetricsData, 0, len(evaluationAnalyticsClone)) + targetData := make([]metricsclient.TargetData, 0, len(targetAnalyticsClone)) + // Process evaluation metrics + for _, analytic := range evaluationAnalyticsClone { metricAttributes := []metricsclient.KeyValue{ - { - Key: featureIdentifierAttribute, - Value: analytic.featureConfig.Feature, - }, - { - Key: featureNameAttribute, - Value: analytic.featureConfig.Feature, - }, - { - Key: variationIdentifierAttribute, - Value: analytic.variation.Identifier, - }, - { - Key: variationValueAttribute, - Value: analytic.variation.Value, - }, - { - Key: sdkTypeAttribute, - Value: sdkType, - }, - { - Key: sdkLanguageAttribute, - Value: sdkLanguage, - }, - { - Key: sdkVersionAttribute, - Value: SdkVersion, - }, + {Key: featureIdentifierAttribute, Value: analytic.featureConfig.Feature}, + {Key: featureNameAttribute, Value: analytic.featureConfig.Feature}, + {Key: variationIdentifierAttribute, Value: analytic.variation.Identifier}, + {Key: variationValueAttribute, Value: analytic.variation.Value}, + {Key: sdkTypeAttribute, Value: sdkType}, + {Key: sdkLanguageAttribute, Value: sdkLanguage}, + {Key: sdkVersionAttribute, Value: SdkVersion}, + {Key: targetAttribute, Value: globalTarget}, } - metricAttributes = append(metricAttributes, metricsclient.KeyValue{ - Key: targetAttribute, - Value: globalTarget, - }) - md := metricsclient.MetricsData{ Timestamp: time.Now().UnixNano() / (int64(time.Millisecond) / int64(time.Nanosecond)), Count: analytic.count, @@ -274,20 +220,29 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { metricData = append(metricData, md) } - // if targets data is empty we just send nil - var targetDataPayload *[]metricsclient.TargetData = nil - if len(targetData) > 0 { - targetDataPayload = targetDataMapToArray(targetData) + // Process target metrics + for _, target := range targetAnalyticsClone { + targetAttributes := make([]metricsclient.KeyValue, 0) + for key, value := range *target.Attributes { + targetAttributes = append(targetAttributes, metricsclient.KeyValue{Key: key, Value: convertInterfaceToString(value)}) + } + + td := metricsclient.TargetData{ + Identifier: target.Identifier, + Name: target.Name, + Attributes: targetAttributes, + } + targetData = append(targetData, td) } analyticsPayload := metricsclient.PostMetricsJSONRequestBody{ MetricsData: &metricData, - TargetData: targetDataPayload, + TargetData: &targetData, } if as.metricsClient != nil { - emptyMetricsData := analyticsPayload.MetricsData == nil || len(*analyticsPayload.MetricsData) == 0 - emptyTargetData := analyticsPayload.TargetData == nil || len(*analyticsPayload.TargetData) == 0 + emptyMetricsData := len(metricData) == 0 + emptyTargetData := len(targetData) == 0 // if we have no metrics to send skip the post request if emptyMetricsData && emptyTargetData { From 6e1bbf8bb673ea8d2d55a77e765c593ac68491dc Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:02:14 +0100 Subject: [PATCH 12/44] FFM-11212 Rename key function --- analyticsservice/analytics.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 0f57ff8..21620ce 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -116,7 +116,7 @@ func (as *AnalyticsService) PushToQueue(featureConfig *rest.FeatureConfig, targe func (as *AnalyticsService) listener() { as.logger.Info("Analytics cache successfully initialized") for ad := range as.analyticsChan { - analyticsKey := getEventSummaryKey(ad) + analyticsKey := getEvaluationAnalyticKey(ad) // Update evaluation metrics as.evaluationsAnalyticsMx.Lock() @@ -278,15 +278,7 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { } } -//func getEventKey(event analyticsEvent) string { -// targetIdentifier := "" -// if event.target != nil { -// targetIdentifier = event.target.Identifier -// } -// return fmt.Sprintf("%s-%s-%s-%s", event.featureConfig.Feature, event.variation.Identifier, event.variation.Value, targetIdentifier) -//} - -func getEventSummaryKey(event analyticsEvent) string { +func getEvaluationAnalyticKey(event analyticsEvent) string { return fmt.Sprintf("%s-%s-%s-%s", event.featureConfig.Feature, event.variation.Identifier, event.variation.Value, globalTarget) } From 184ca1404745d2eb4f7e6d74ae58bf1a633794ae Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:02:31 +0100 Subject: [PATCH 13/44] FFM-11212 Delete unused func --- analyticsservice/analytics.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 21620ce..c41a404 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -281,11 +281,3 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { func getEvaluationAnalyticKey(event analyticsEvent) string { return fmt.Sprintf("%s-%s-%s-%s", event.featureConfig.Feature, event.variation.Identifier, event.variation.Value, globalTarget) } - -func targetDataMapToArray(targetMap map[string]metricsclient.TargetData) *[]metricsclient.TargetData { - targetDataArray := make([]metricsclient.TargetData, 0, len(targetMap)) - for _, targetData := range targetMap { - targetDataArray = append(targetDataArray, targetData) - } - return &targetDataArray -} From 682bd92363ae8309bdda73e0ec04e48f754afb59 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:10:46 +0100 Subject: [PATCH 14/44] FFM-11212 Tweak comment --- analyticsservice/analytics.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index c41a404..02b1033 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -181,15 +181,15 @@ func convertInterfaceToString(i interface{}) string { func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - // Clone and reset evaluation analytics cache. As metrics is secondary to the flags, we do it this way - // so it doesn't affect the performance of our users code. Even if it means - // we lose metrics the odd time. + // Clone and reset the evaluation analytics cache. This strategy is employed to minimize the duration + // for which locks are held so that metrics processing does not affect flag evaluations performance + // as metrics are considered secondary, as.evaluationsAnalyticsMx.Lock() evaluationAnalyticsClone := as.evaluationAnalytics as.evaluationAnalytics = map[string]analyticsEvent{} as.evaluationsAnalyticsMx.Unlock() - // Clone and reset target analytics cache + // Clone and reset target analytics cache for same reason. as.targetAnalyticsMx.Lock() targetAnalyticsClone := as.targetAnalytics as.targetAnalytics = make(map[string]evaluation.Target) From 8d27fe5e8ac8a67baf8ccefcf6065b5e18292231 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:12:27 +0100 Subject: [PATCH 15/44] FFM-11212 Tweak comment --- analyticsservice/analytics.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 02b1033..0842ebc 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -181,9 +181,10 @@ func convertInterfaceToString(i interface{}) string { func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { - // Clone and reset the evaluation analytics cache. This strategy is employed to minimize the duration - // for which locks are held so that metrics processing does not affect flag evaluations performance - // as metrics are considered secondary, + // Clone and reset the evaluation analytics cache to minimise the duration + // for which locks are held, so that metrics processing does not affect flag evaluations performance. + // Although this might occasionally result in the loss of some metrics during periods of high load, + // it is an acceptable tradeoff to prevent extended lock periods that could degrade user code. as.evaluationsAnalyticsMx.Lock() evaluationAnalyticsClone := as.evaluationAnalytics as.evaluationAnalytics = map[string]analyticsEvent{} From 089f0c542d40726a1b62a9a0231b2a85ee13f05c Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:18:08 +0100 Subject: [PATCH 16/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 44 +++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 5eb1aa3..6b66c16 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -1,6 +1,13 @@ package analyticsservice -import "testing" +import ( + "testing" + "time" + + "github.com/harness/ff-golang-server-sdk/evaluation" + "github.com/harness/ff-golang-server-sdk/logger" + "github.com/harness/ff-golang-server-sdk/rest" +) func Test_convertInterfaceToString(t *testing.T) { testCases := map[string]struct { @@ -52,3 +59,38 @@ func Test_convertInterfaceToString(t *testing.T) { }) } } + +func TestListenerHandlesEventsCorrectly(t *testing.T) { + noOpLogger := logger.NewNoOpLogger() // assuming you have a constructor for a noOpLogger + service := NewAnalyticsService(1*time.Minute, noOpLogger) + defer close(service.analyticsChan) // Ensure the channel is closed after test + + target := &evaluation.Target{Identifier: "target1", Anonymous: new(bool)} + featureConfig := &rest.FeatureConfig{Feature: "feature1"} + variation := &rest.Variation{Identifier: "var1", Value: "value1"} + + // Send an event to the channel + go func() { + service.analyticsChan <- analyticsEvent{ + target: target, + featureConfig: featureConfig, + variation: variation, + } + }() + + // Allow some time for the event to be processed + time.Sleep(100 * time.Millisecond) + + // Check if the event is processed correctly + service.evaluationsAnalyticsMx.Lock() + if len(service.evaluationAnalytics) != 1 { + t.Errorf("Expected evaluationAnalytics to contain 1 item, got %d", len(service.evaluationAnalytics)) + } + service.evaluationsAnalyticsMx.Unlock() + + service.seenTargetsMx.RLock() + if !service.seenTargets["target1"] { + t.Errorf("Expected target 'target1' to be marked as seen") + } + service.seenTargetsMx.RUnlock() +} From 3a9e2691d9d39b09d25f1bae3d3685f91e2f08f3 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:45:10 +0100 Subject: [PATCH 17/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 92 +++++++++++++++++++++--------- 1 file changed, 66 insertions(+), 26 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 6b66c16..273888a 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -61,36 +61,76 @@ func Test_convertInterfaceToString(t *testing.T) { } func TestListenerHandlesEventsCorrectly(t *testing.T) { - noOpLogger := logger.NewNoOpLogger() // assuming you have a constructor for a noOpLogger - service := NewAnalyticsService(1*time.Minute, noOpLogger) - defer close(service.analyticsChan) // Ensure the channel is closed after test + noOpLogger := logger.NewNoOpLogger() // Assume a constructor exists for the noOpLogger - target := &evaluation.Target{Identifier: "target1", Anonymous: new(bool)} - featureConfig := &rest.FeatureConfig{Feature: "feature1"} - variation := &rest.Variation{Identifier: "var1", Value: "value1"} + testCases := []struct { + name string + events []analyticsEvent + expectedCounts map[string]int // Key by "feature-var-value-target" + expectedSeen map[string]bool + }{ + { + name: "Single evaluation", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + }, + expectedCounts: map[string]int{"feature1-var1-value1-global": 1}, + expectedSeen: map[string]bool{"target1": true}, + }, + { + name: "Two identical evaluations with the same target", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + }, + expectedCounts: map[string]int{"feature1-var1-value1-global": 2}, + expectedSeen: map[string]bool{"target1": true}, + }, + { + name: "Two identical evaluations with different targets", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target2"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + }, + expectedCounts: map[string]int{"feature1-var1-value1-global": 2}, + expectedSeen: map[string]bool{"target1": true, "target2": true}, + }, + } - // Send an event to the channel - go func() { - service.analyticsChan <- analyticsEvent{ - target: target, - featureConfig: featureConfig, - variation: variation, - } - }() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + service := NewAnalyticsService(1*time.Minute, noOpLogger) + defer close(service.analyticsChan) // Ensure the channel is closed after test - // Allow some time for the event to be processed - time.Sleep(100 * time.Millisecond) + // Start the listener in a goroutine + go service.listener() - // Check if the event is processed correctly - service.evaluationsAnalyticsMx.Lock() - if len(service.evaluationAnalytics) != 1 { - t.Errorf("Expected evaluationAnalytics to contain 1 item, got %d", len(service.evaluationAnalytics)) - } - service.evaluationsAnalyticsMx.Unlock() + // Send all events for the test case + for _, event := range tc.events { + service.analyticsChan <- event + } + + // Allow some time for the events to be processed + time.Sleep(100 * time.Millisecond) + + // Check evaluation analytics counts + service.evaluationsAnalyticsMx.Lock() + for key, expectedCount := range tc.expectedCounts { + analytic, exists := service.evaluationAnalytics[key] + if !exists || analytic.count != expectedCount { + t.Errorf("Test %s failed: expected count for key %s is %d, got %d", tc.name, key, expectedCount, analytic.count) + } + } + service.evaluationsAnalyticsMx.Unlock() - service.seenTargetsMx.RLock() - if !service.seenTargets["target1"] { - t.Errorf("Expected target 'target1' to be marked as seen") + // Check seen targets + service.seenTargetsMx.RLock() + for targetID, expectedSeen := range tc.expectedSeen { + if seen := service.seenTargets[targetID]; seen != expectedSeen { + t.Errorf("Test %s failed: expected seen status for target %s is %v, got %v", tc.name, targetID, expectedSeen, seen) + } + } + service.seenTargetsMx.RUnlock() + }) } - service.seenTargetsMx.RUnlock() } From d9f7a8cee7b9b24c2ae02deff8404ec7954e8c23 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:50:29 +0100 Subject: [PATCH 18/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 40 ++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 273888a..81d6d4d 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -64,18 +64,20 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { noOpLogger := logger.NewNoOpLogger() // Assume a constructor exists for the noOpLogger testCases := []struct { - name string - events []analyticsEvent - expectedCounts map[string]int // Key by "feature-var-value-target" - expectedSeen map[string]bool + name string + events []analyticsEvent + expectedEvaluations map[string]int // Key by "feature-var-value-target" + expectedSeen map[string]bool + expectedTargets map[string]evaluation.Target }{ { name: "Single evaluation", events: []analyticsEvent{ {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, }, - expectedCounts: map[string]int{"feature1-var1-value1-global": 1}, - expectedSeen: map[string]bool{"target1": true}, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 1}, + expectedSeen: map[string]bool{"target1": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}}, }, { name: "Two identical evaluations with the same target", @@ -83,8 +85,9 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, }, - expectedCounts: map[string]int{"feature1-var1-value1-global": 2}, - expectedSeen: map[string]bool{"target1": true}, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 2}, + expectedSeen: map[string]bool{"target1": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}}, }, { name: "Two identical evaluations with different targets", @@ -92,8 +95,9 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, {target: &evaluation.Target{Identifier: "target2"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, }, - expectedCounts: map[string]int{"feature1-var1-value1-global": 2}, - expectedSeen: map[string]bool{"target1": true, "target2": true}, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 2}, + expectedSeen: map[string]bool{"target1": true, "target2": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}}, }, } @@ -113,9 +117,9 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { // Allow some time for the events to be processed time.Sleep(100 * time.Millisecond) - // Check evaluation analytics counts + // Check evaluation metrics counts service.evaluationsAnalyticsMx.Lock() - for key, expectedCount := range tc.expectedCounts { + for key, expectedCount := range tc.expectedEvaluations { analytic, exists := service.evaluationAnalytics[key] if !exists || analytic.count != expectedCount { t.Errorf("Test %s failed: expected count for key %s is %d, got %d", tc.name, key, expectedCount, analytic.count) @@ -123,7 +127,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { } service.evaluationsAnalyticsMx.Unlock() - // Check seen targets + // Check target metrics service.seenTargetsMx.RLock() for targetID, expectedSeen := range tc.expectedSeen { if seen := service.seenTargets[targetID]; seen != expectedSeen { @@ -131,6 +135,16 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { } } service.seenTargetsMx.RUnlock() + + // Check target analytics + service.targetAnalyticsMx.Lock() + for targetID, expectedTarget := range tc.expectedTargets { + target, exists := service.targetAnalytics[targetID] + if !exists || target.Identifier != expectedTarget.Identifier || target.Name != expectedTarget.Name { + t.Errorf("Test %s failed: expected target details for %s, got %v", tc.name, targetID, target) + } + } + service.targetAnalyticsMx.Unlock() }) } } From 759d9e213507e43c474f75fee77dfcfaaab8779f Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:51:05 +0100 Subject: [PATCH 19/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 81d6d4d..7dd9e97 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -140,7 +140,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { service.targetAnalyticsMx.Lock() for targetID, expectedTarget := range tc.expectedTargets { target, exists := service.targetAnalytics[targetID] - if !exists || target.Identifier != expectedTarget.Identifier || target.Name != expectedTarget.Name { + if !exists || target.Identifier != expectedTarget.Identifier { t.Errorf("Test %s failed: expected target details for %s, got %v", tc.name, targetID, target) } } From 651c880e25c06875b0f831ad594174691c039986 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:54:16 +0100 Subject: [PATCH 20/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 7dd9e97..873a4e6 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -99,6 +99,16 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { expectedSeen: map[string]bool{"target1": true, "target2": true}, expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}}, }, + { + name: "Two different evaluations with two different targets", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target2"}, featureConfig: &rest.FeatureConfig{Feature: "feature2"}, variation: &rest.Variation{Identifier: "var2", Value: "value2"}}, + }, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 1, "feature2-var2-value2-global": 1}, + expectedSeen: map[string]bool{"target1": true, "target2": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}}, + }, } for _, tc := range testCases { From eb35ce751b2fba9c7c136888c2968c8d5273ebf0 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:54:50 +0100 Subject: [PATCH 21/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 102 ++++++++++++++--------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 873a4e6..77f1d85 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -9,57 +9,6 @@ import ( "github.com/harness/ff-golang-server-sdk/rest" ) -func Test_convertInterfaceToString(t *testing.T) { - testCases := map[string]struct { - input interface{} - expected string - }{ - "Given I input a string": { - input: "123", - expected: "123", - }, - "Given I input a bool with the value false": { - input: false, - expected: "false", - }, - "Given I input a bool with the value true": { - input: true, - expected: "true", - }, - "Given I input an int64": { - input: int64(123), - expected: "123", - }, - "Given I input an int": { - input: 123, - expected: "123", - }, - "Given I input a float64": { - input: float64(2.5), - expected: "2.5", - }, - "Given I input a float32": { - input: float32(2.5), - expected: "2.5", - }, - "Given I input a nil value": { - input: nil, - expected: "nil", - }, - } - - for desc, tc := range testCases { - tc := tc - t.Run(desc, func(t *testing.T) { - - actual := convertInterfaceToString(tc.input) - if actual != tc.expected { - t.Errorf("(%s): expected %s, actual %s", desc, tc.expected, actual) - } - }) - } -} - func TestListenerHandlesEventsCorrectly(t *testing.T) { noOpLogger := logger.NewNoOpLogger() // Assume a constructor exists for the noOpLogger @@ -158,3 +107,54 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { }) } } + +func Test_convertInterfaceToString(t *testing.T) { + testCases := map[string]struct { + input interface{} + expected string + }{ + "Given I input a string": { + input: "123", + expected: "123", + }, + "Given I input a bool with the value false": { + input: false, + expected: "false", + }, + "Given I input a bool with the value true": { + input: true, + expected: "true", + }, + "Given I input an int64": { + input: int64(123), + expected: "123", + }, + "Given I input an int": { + input: 123, + expected: "123", + }, + "Given I input a float64": { + input: float64(2.5), + expected: "2.5", + }, + "Given I input a float32": { + input: float32(2.5), + expected: "2.5", + }, + "Given I input a nil value": { + input: nil, + expected: "nil", + }, + } + + for desc, tc := range testCases { + tc := tc + t.Run(desc, func(t *testing.T) { + + actual := convertInterfaceToString(tc.input) + if actual != tc.expected { + t.Errorf("(%s): expected %s, actual %s", desc, tc.expected, actual) + } + }) + } +} From f0b93724dedf3b3a8b0e880e5ccbd22f568e660b Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Tue, 16 Apr 2024 19:55:19 +0100 Subject: [PATCH 22/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 77f1d85..809a72f 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -10,12 +10,12 @@ import ( ) func TestListenerHandlesEventsCorrectly(t *testing.T) { - noOpLogger := logger.NewNoOpLogger() // Assume a constructor exists for the noOpLogger + noOpLogger := logger.NewNoOpLogger() testCases := []struct { name string events []analyticsEvent - expectedEvaluations map[string]int // Key by "feature-var-value-target" + expectedEvaluations map[string]int expectedSeen map[string]bool expectedTargets map[string]evaluation.Target }{ From 98714d84f3d0812065930c9ff4e8ef571ccc2d4c Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 10:53:12 +0100 Subject: [PATCH 23/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 809a72f..1f1d895 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -58,6 +58,17 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { expectedSeen: map[string]bool{"target1": true, "target2": true}, expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}}, }, + { + name: "Three different evaluations with two identical targets", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target2"}, featureConfig: &rest.FeatureConfig{Feature: "feature2"}, variation: &rest.Variation{Identifier: "var2", Value: "value2"}}, + {target: &evaluation.Target{Identifier: "target3"}, featureConfig: &rest.FeatureConfig{Feature: "feature3"}, variation: &rest.Variation{Identifier: "var3", Value: "value3"}}, + }, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 1, "feature2-var2-value2-global": 1, "feature3-var3-value3-global": 1}, + expectedSeen: map[string]bool{"target1": true, "target2": true, "target3": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}, "target3": {Identifier: "target3"}}, + }, } for _, tc := range testCases { From 0286d677ea8e10d7b7e1c0ca12ae47a3eaff79e3 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 10:55:04 +0100 Subject: [PATCH 24/44] FFM-11212 Remove reduandant paranthesis --- analyticsservice/analytics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 0842ebc..d165c15 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -125,7 +125,7 @@ func (as *AnalyticsService) listener() { ad.count = 1 as.evaluationAnalytics[analyticsKey] = ad } else { - ad.count = (analytic.count + 1) + ad.count = analytic.count + 1 as.evaluationAnalytics[analyticsKey] = ad } as.evaluationsAnalyticsMx.Unlock() From 94b6fbcb15873f7ee4db46dbb4528025569f75df Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 11:42:02 +0100 Subject: [PATCH 25/44] FFM-11212 New test for listener --- analyticsservice/analytics_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 1f1d895..e4f2b58 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -74,7 +74,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { service := NewAnalyticsService(1*time.Minute, noOpLogger) - defer close(service.analyticsChan) // Ensure the channel is closed after test + defer close(service.analyticsChan) // Start the listener in a goroutine go service.listener() @@ -101,7 +101,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { service.seenTargetsMx.RLock() for targetID, expectedSeen := range tc.expectedSeen { if seen := service.seenTargets[targetID]; seen != expectedSeen { - t.Errorf("Test %s failed: expected seen status for target %s is %v, got %v", tc.name, targetID, expectedSeen, seen) + t.Errorf("Test %s failed: expected target to be in seen targets cache %s is %v", tc.name, targetID, expectedSeen) } } service.seenTargetsMx.RUnlock() @@ -111,7 +111,7 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { for targetID, expectedTarget := range tc.expectedTargets { target, exists := service.targetAnalytics[targetID] if !exists || target.Identifier != expectedTarget.Identifier { - t.Errorf("Test %s failed: expected target details for %s, got %v", tc.name, targetID, target) + t.Errorf("Test %s failed: expected target to be in target cache %s", tc.name, targetID) } } service.targetAnalyticsMx.Unlock() From f82e13ecc07b41ec66476120b026148987c6556b Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 12:38:21 +0100 Subject: [PATCH 26/44] FFM-11212 Don't use pointer interface for metrics api --- analyticsservice/analytics.go | 6 ++-- analyticsservice/analytics_test.go | 50 ++++++++++++++++++++++++++++++ client/client.go | 2 +- go.mod | 1 + go.sum | 5 +++ 5 files changed, 60 insertions(+), 4 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index d165c15..a3e37f7 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -54,7 +54,7 @@ type AnalyticsService struct { seenTargets map[string]bool timeout time.Duration logger logger.Logger - metricsClient *metricsclient.ClientWithResponsesInterface + metricsClient metricsclient.ClientWithResponsesInterface environmentID string } @@ -83,7 +83,7 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics } // Start starts the client and timer to send analytics -func (as *AnalyticsService) Start(ctx context.Context, client *metricsclient.ClientWithResponsesInterface, environmentID string) { +func (as *AnalyticsService) Start(ctx context.Context, client metricsclient.ClientWithResponsesInterface, environmentID string) { as.logger.Infof("%s Metrics started", sdk_codes.MetricsStarted) as.metricsClient = client as.environmentID = environmentID @@ -251,7 +251,7 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { return } - mClient := *as.metricsClient + mClient := as.metricsClient jsonData, err := json.Marshal(analyticsPayload) if err != nil { diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index e4f2b58..d60f62d 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -1,14 +1,64 @@ package analyticsservice import ( + "context" + "io" "testing" "time" "github.com/harness/ff-golang-server-sdk/evaluation" "github.com/harness/ff-golang-server-sdk/logger" + "github.com/harness/ff-golang-server-sdk/metricsclient" "github.com/harness/ff-golang-server-sdk/rest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) +// MockMetricsClient is a mock implementation for the ClientWithResponsesInterface +type MockMetricsClient struct { + mock.Mock +} + +func (m *MockMetricsClient) PostMetricsWithResponse(ctx context.Context, environmentUUID metricsclient.EnvironmentPathParam, params *metricsclient.PostMetricsParams, body metricsclient.PostMetricsJSONRequestBody, reqEditors ...metricsclient.RequestEditorFn) (*metricsclient.PostMetricsResponse, error) { + args := m.Called(ctx, environmentUUID, params, body) + return args.Get(0).(*metricsclient.PostMetricsResponse), args.Error(1) +} + +func (m *MockMetricsClient) PostMetricsWithBodyWithResponse(ctx context.Context, environmentUUID metricsclient.EnvironmentPathParam, params *metricsclient.PostMetricsParams, contentType string, body io.Reader, reqEditors ...metricsclient.RequestEditorFn) (*metricsclient.PostMetricsResponse, error) { + args := m.Called(ctx, environmentUUID, params, contentType, body) + return args.Get(0).(*metricsclient.PostMetricsResponse), args.Error(1) +} + +func TestSendDataAndResetCache(t *testing.T) { + noOpLogger := logger.NewNoOpLogger() + mockMetricsClient := MockMetricsClient{} + service := NewAnalyticsService(1*time.Minute, noOpLogger) + service.metricsClient = &mockMetricsClient + + // Setup mock response from metrics server + mockResponse := &metricsclient.PostMetricsResponse{} + mockMetricsClient.On("PostMetricsWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(mockResponse, nil) + + // Simulate analytics data + service.evaluationAnalytics["test-key"] = analyticsEvent{ + target: &evaluation.Target{Identifier: "target1", Name: "Target One"}, + featureConfig: &rest.FeatureConfig{Feature: "feature1"}, + variation: &rest.Variation{Identifier: "var1", Value: "value1"}, + count: 1, + } + service.targetAnalytics["target1"] = evaluation.Target{Identifier: "target1", Name: "Target One", Attributes: &map[string]interface{}{"key": "value"}} + + ctx := context.TODO() + service.sendDataAndResetCache(ctx) + + // Verify that metrics data is sent correctly + mockMetricsClient.AssertCalled(t, "PostMetricsWithResponse", ctx, service.environmentID, nil, mock.AnythingOfType("metricsclient.PostMetricsJSONRequestBody")) + + // Verify that caches are reset + assert.Empty(t, service.evaluationAnalytics, "Evaluation analytics cache should be empty after sending data") + assert.Empty(t, service.targetAnalytics, "Target analytics cache should be empty after sending data") +} + func TestListenerHandlesEventsCorrectly(t *testing.T) { noOpLogger := logger.NewNoOpLogger() diff --git a/client/client.go b/client/client.go index 3062351..4cfabb5 100644 --- a/client/client.go +++ b/client/client.go @@ -586,7 +586,7 @@ func (c *CfClient) setAnalyticsServiceClient(ctx context.Context) { return } c.config.Logger.Info("Posting analytics data enabled") - c.analyticsService.Start(ctx, &c.metricsApi, c.environmentID) + c.analyticsService.Start(ctx, c.metricsApi, c.environmentID) } // BoolVariation returns the value of a boolean feature flag for a given target. diff --git a/go.mod b/go.mod index 5af1be1..26ad767 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,7 @@ require ( github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 // indirect github.com/perimeterx/marshmallow v1.1.5 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/objx v0.5.0 // indirect go.uber.org/atomic v1.7.0 // indirect go.uber.org/multierr v1.6.0 // indirect golang.org/x/net v0.19.0 // indirect diff --git a/go.sum b/go.sum index 89dd9f7..317b121 100644 --- a/go.sum +++ b/go.sum @@ -76,10 +76,15 @@ github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0b github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA= github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad/go.mod h1:qLr4V1qq6nMqFKkMo8ZTx3f+BZEkzsRUY10Xsm2mwU0= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= github.com/ugorji/go/codec v1.2.11 h1:BMaWp1Bb6fHwEtbplGBGJ498wD+LKlNSl25MjdZY4dU= From 7e27d1173f27974facaf5cee592938ffbb7396a2 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 12:49:34 +0100 Subject: [PATCH 27/44] FFM-11212 Don't store anon targets --- analyticsservice/analytics.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index a3e37f7..cf6a7ff 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -130,6 +130,11 @@ func (as *AnalyticsService) listener() { } as.evaluationsAnalyticsMx.Unlock() + // Check if target is nil or anonymous + if ad.target == nil || (ad.target != nil && *ad.target.Anonymous) { + continue + } + // Check if target has been seen as.seenTargetsMx.RLock() _, seen := as.seenTargets[ad.target.Identifier] From 99761db757cdcaba17ef07cfa306a7a8fb893ee2 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 12:59:47 +0100 Subject: [PATCH 28/44] FFM-11212 Test for anonymous target --- analyticsservice/analytics.go | 2 +- analyticsservice/analytics_test.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index cf6a7ff..879743b 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -131,7 +131,7 @@ func (as *AnalyticsService) listener() { as.evaluationsAnalyticsMx.Unlock() // Check if target is nil or anonymous - if ad.target == nil || (ad.target != nil && *ad.target.Anonymous) { + if ad.target == nil || (ad.target.Anonymous != nil && *ad.target.Anonymous) { continue } diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index d60f62d..2133140 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -119,6 +119,17 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { expectedSeen: map[string]bool{"target1": true, "target2": true, "target3": true}, expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target2": {Identifier: "target2"}, "target3": {Identifier: "target3"}}, }, + { + name: "Three different evaluations with two anonymous targets", + events: []analyticsEvent{ + {target: &evaluation.Target{Identifier: "target1"}, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target2", Anonymous: boolPtr(true)}, featureConfig: &rest.FeatureConfig{Feature: "feature2"}, variation: &rest.Variation{Identifier: "var2", Value: "value2"}}, + {target: &evaluation.Target{Identifier: "target3"}, featureConfig: &rest.FeatureConfig{Feature: "feature3"}, variation: &rest.Variation{Identifier: "var3", Value: "value3"}}, + }, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 1, "feature2-var2-value2-global": 1, "feature3-var3-value3-global": 1}, + expectedSeen: map[string]bool{"target1": true, "target3": true}, + expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target3": {Identifier: "target3"}}, + }, } for _, tc := range testCases { @@ -219,3 +230,7 @@ func Test_convertInterfaceToString(t *testing.T) { }) } } + +func boolPtr(b bool) *bool { + return &b +} From 1184137d6df230650f4dece3a19d2f657115af25 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 13:00:50 +0100 Subject: [PATCH 29/44] FFM-11212 Test for nil target --- analyticsservice/analytics_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 2133140..896a44c 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -130,6 +130,17 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { expectedSeen: map[string]bool{"target1": true, "target3": true}, expectedTargets: map[string]evaluation.Target{"target1": {Identifier: "target1"}, "target3": {Identifier: "target3"}}, }, + { + name: "Three different evaluations with one anonymous target and one nil target", + events: []analyticsEvent{ + {target: nil, featureConfig: &rest.FeatureConfig{Feature: "feature1"}, variation: &rest.Variation{Identifier: "var1", Value: "value1"}}, + {target: &evaluation.Target{Identifier: "target2", Anonymous: boolPtr(true)}, featureConfig: &rest.FeatureConfig{Feature: "feature2"}, variation: &rest.Variation{Identifier: "var2", Value: "value2"}}, + {target: &evaluation.Target{Identifier: "target3"}, featureConfig: &rest.FeatureConfig{Feature: "feature3"}, variation: &rest.Variation{Identifier: "var3", Value: "value3"}}, + }, + expectedEvaluations: map[string]int{"feature1-var1-value1-global": 1, "feature2-var2-value2-global": 1, "feature3-var3-value3-global": 1}, + expectedSeen: map[string]bool{"target3": true}, + expectedTargets: map[string]evaluation.Target{"target3": {Identifier: "target3"}}, + }, } for _, tc := range testCases { From 1ce0fc8ee60dc8b10a1aa51f6fd9938edc9b3fbc Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 13:04:35 +0100 Subject: [PATCH 30/44] FFM-11212 Remove test for send data --- analyticsservice/analytics_test.go | 50 ------------------------------ go.mod | 1 - go.sum | 5 --- 3 files changed, 56 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index 896a44c..b8bdd78 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -1,64 +1,14 @@ package analyticsservice import ( - "context" - "io" "testing" "time" "github.com/harness/ff-golang-server-sdk/evaluation" "github.com/harness/ff-golang-server-sdk/logger" - "github.com/harness/ff-golang-server-sdk/metricsclient" "github.com/harness/ff-golang-server-sdk/rest" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" ) -// MockMetricsClient is a mock implementation for the ClientWithResponsesInterface -type MockMetricsClient struct { - mock.Mock -} - -func (m *MockMetricsClient) PostMetricsWithResponse(ctx context.Context, environmentUUID metricsclient.EnvironmentPathParam, params *metricsclient.PostMetricsParams, body metricsclient.PostMetricsJSONRequestBody, reqEditors ...metricsclient.RequestEditorFn) (*metricsclient.PostMetricsResponse, error) { - args := m.Called(ctx, environmentUUID, params, body) - return args.Get(0).(*metricsclient.PostMetricsResponse), args.Error(1) -} - -func (m *MockMetricsClient) PostMetricsWithBodyWithResponse(ctx context.Context, environmentUUID metricsclient.EnvironmentPathParam, params *metricsclient.PostMetricsParams, contentType string, body io.Reader, reqEditors ...metricsclient.RequestEditorFn) (*metricsclient.PostMetricsResponse, error) { - args := m.Called(ctx, environmentUUID, params, contentType, body) - return args.Get(0).(*metricsclient.PostMetricsResponse), args.Error(1) -} - -func TestSendDataAndResetCache(t *testing.T) { - noOpLogger := logger.NewNoOpLogger() - mockMetricsClient := MockMetricsClient{} - service := NewAnalyticsService(1*time.Minute, noOpLogger) - service.metricsClient = &mockMetricsClient - - // Setup mock response from metrics server - mockResponse := &metricsclient.PostMetricsResponse{} - mockMetricsClient.On("PostMetricsWithResponse", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(mockResponse, nil) - - // Simulate analytics data - service.evaluationAnalytics["test-key"] = analyticsEvent{ - target: &evaluation.Target{Identifier: "target1", Name: "Target One"}, - featureConfig: &rest.FeatureConfig{Feature: "feature1"}, - variation: &rest.Variation{Identifier: "var1", Value: "value1"}, - count: 1, - } - service.targetAnalytics["target1"] = evaluation.Target{Identifier: "target1", Name: "Target One", Attributes: &map[string]interface{}{"key": "value"}} - - ctx := context.TODO() - service.sendDataAndResetCache(ctx) - - // Verify that metrics data is sent correctly - mockMetricsClient.AssertCalled(t, "PostMetricsWithResponse", ctx, service.environmentID, nil, mock.AnythingOfType("metricsclient.PostMetricsJSONRequestBody")) - - // Verify that caches are reset - assert.Empty(t, service.evaluationAnalytics, "Evaluation analytics cache should be empty after sending data") - assert.Empty(t, service.targetAnalytics, "Target analytics cache should be empty after sending data") -} - func TestListenerHandlesEventsCorrectly(t *testing.T) { noOpLogger := logger.NewNoOpLogger() diff --git a/go.mod b/go.mod index 26ad767..5af1be1 100644 --- a/go.mod +++ b/go.mod @@ -37,7 +37,6 @@ require ( github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 // indirect github.com/perimeterx/marshmallow v1.1.5 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/stretchr/objx v0.5.0 // indirect go.uber.org/atomic v1.7.0 // indirect go.uber.org/multierr v1.6.0 // indirect golang.org/x/net v0.19.0 // indirect diff --git a/go.sum b/go.sum index 317b121..89dd9f7 100644 --- a/go.sum +++ b/go.sum @@ -76,15 +76,10 @@ github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0b github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA= github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad/go.mod h1:qLr4V1qq6nMqFKkMo8ZTx3f+BZEkzsRUY10Xsm2mwU0= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= -github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c= -github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= github.com/ugorji/go/codec v1.2.11 h1:BMaWp1Bb6fHwEtbplGBGJ498wD+LKlNSl25MjdZY4dU= From dee51f1cde909ebec8aaf577bc1867b5645c5334 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 13:19:02 +0100 Subject: [PATCH 31/44] FFM-11212 Implement cache limits --- analyticsservice/analytics.go | 22 ++++++++++----- sdk_codes/sdk_codes.go | 51 ++++++++++++++++++----------------- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 879743b..9320ed9 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -120,15 +120,23 @@ func (as *AnalyticsService) listener() { // Update evaluation metrics as.evaluationsAnalyticsMx.Lock() - analytic, ok := as.evaluationAnalytics[analyticsKey] - if !ok { - ad.count = 1 - as.evaluationAnalytics[analyticsKey] = ad + if len(as.evaluationAnalytics) >= maxAnalyticsEntries { + as.logger.Warnf("%s Evaluation analytic cache reached max size, remaining evaluation metrics for this analytics interval will not be sent", sdk_codes.EvaluationMetricsMaxSizeReached) } else { - ad.count = analytic.count + 1 - as.evaluationAnalytics[analyticsKey] = ad + analytic, ok := as.evaluationAnalytics[analyticsKey] + if !ok { + ad.count = 1 + as.evaluationAnalytics[analyticsKey] = ad + } else { + ad.count = analytic.count + 1 + as.evaluationAnalytics[analyticsKey] = ad + } + as.evaluationsAnalyticsMx.Unlock() + } + + if len(as.targetAnalytics) >= maxTargetEntries { + as.logger.Warnf("%s Target analytics cache reached max size, remaining target metrics for this analytics interval will not be sent", sdk_codes.TargetMetricsMaxSizeReached) } - as.evaluationsAnalyticsMx.Unlock() // Check if target is nil or anonymous if ad.target == nil || (ad.target.Anonymous != nil && *ad.target.Anonymous) { diff --git a/sdk_codes/sdk_codes.go b/sdk_codes/sdk_codes.go index 744744b..fbbf003 100644 --- a/sdk_codes/sdk_codes.go +++ b/sdk_codes/sdk_codes.go @@ -4,29 +4,30 @@ package sdk_codes type SDKCode string const ( - InitSuccess SDKCode = "SDKCODE:1000" - InitAuthError SDKCode = "SDKCODE:1001" - InitMissingKey SDKCode = "SDKCODE:1002" - InitWaiting SDKCode = "SDKCODE:1003" - AuthSuccess SDKCode = "SDKCODE:2000" - AuthFailed SDKCode = "SDKCODE:2001" - AuthAttempt SDKCode = "SDKCODE:2002" - AuthExceededRetries SDKCode = "SDKCODE:2003" - CloseStarted SDKCode = "SDKCODE:3000" - CloseSuccess SDKCode = "SDKCODE:3001" - PollStart SDKCode = "SDKCODE:4000" - PollStop SDKCode = "SDKCODE:4001" - StreamStarted SDKCode = "SDKCODE:5000" - StreamDisconnected SDKCode = "SDKCODE:5001" - StreamEvent SDKCode = "SDKCODE:5002" - // StreamRetry TODO it's not clear how the SSE retry mechanism is working. Add this once SSE resilency has been established in FFM-9485 - StreamRetry SDKCode = "SDKCODE:5003" - StreamStop SDKCode = "SDKCODE:5004" - EvaluationSuccess SDKCode = "SDKCODE:6000" - EvaluationFailed SDKCode = "SDKCODE:6001" - MissingBucketBy SDKCode = "SDKCODE:6002" - MetricsStarted SDKCode = "SDKCODE:7000" - MetricsStopped SDKCode = "SDKCODE:7001" - MetricsSendFail SDKCode = "SDKCODE:7002" - MetricsSendSuccess SDKCode = "SDKCODE:7003" + InitSuccess SDKCode = "SDKCODE:1000" + InitAuthError SDKCode = "SDKCODE:1001" + InitMissingKey SDKCode = "SDKCODE:1002" + InitWaiting SDKCode = "SDKCODE:1003" + AuthSuccess SDKCode = "SDKCODE:2000" + AuthFailed SDKCode = "SDKCODE:2001" + AuthAttempt SDKCode = "SDKCODE:2002" + AuthExceededRetries SDKCode = "SDKCODE:2003" + CloseStarted SDKCode = "SDKCODE:3000" + CloseSuccess SDKCode = "SDKCODE:3001" + PollStart SDKCode = "SDKCODE:4000" + PollStop SDKCode = "SDKCODE:4001" + StreamStarted SDKCode = "SDKCODE:5000" + StreamDisconnected SDKCode = "SDKCODE:5001" + StreamEvent SDKCode = "SDKCODE:5002" + StreamRetry SDKCode = "SDKCODE:5003" + StreamStop SDKCode = "SDKCODE:5004" + EvaluationSuccess SDKCode = "SDKCODE:6000" + EvaluationFailed SDKCode = "SDKCODE:6001" + MissingBucketBy SDKCode = "SDKCODE:6002" + MetricsStarted SDKCode = "SDKCODE:7000" + MetricsStopped SDKCode = "SDKCODE:7001" + MetricsSendFail SDKCode = "SDKCODE:7002" + MetricsSendSuccess SDKCode = "SDKCODE:7003" + TargetMetricsMaxSizeReached SDKCode = "SDKCODE:7004" + EvaluationMetricsMaxSizeReached SDKCode = "SDKCODE:7007" ) From fbf3caa37071b7df398134337f07ba8a6d519045 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Wed, 17 Apr 2024 13:35:14 +0100 Subject: [PATCH 32/44] FFM-11212 Implement cache limits --- analyticsservice/analytics.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 9320ed9..2e53e2d 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -118,11 +118,10 @@ func (as *AnalyticsService) listener() { for ad := range as.analyticsChan { analyticsKey := getEvaluationAnalyticKey(ad) - // Update evaluation metrics as.evaluationsAnalyticsMx.Lock() - if len(as.evaluationAnalytics) >= maxAnalyticsEntries { - as.logger.Warnf("%s Evaluation analytic cache reached max size, remaining evaluation metrics for this analytics interval will not be sent", sdk_codes.EvaluationMetricsMaxSizeReached) - } else { + // Check if we've hit capacity for evaluations + if len(as.evaluationAnalytics) < maxAnalyticsEntries { + // Update evaluation metrics analytic, ok := as.evaluationAnalytics[analyticsKey] if !ok { ad.count = 1 @@ -131,12 +130,10 @@ func (as *AnalyticsService) listener() { ad.count = analytic.count + 1 as.evaluationAnalytics[analyticsKey] = ad } - as.evaluationsAnalyticsMx.Unlock() - } - - if len(as.targetAnalytics) >= maxTargetEntries { - as.logger.Warnf("%s Target analytics cache reached max size, remaining target metrics for this analytics interval will not be sent", sdk_codes.TargetMetricsMaxSizeReached) + } else { + as.logger.Warnf("%s Evaluation analytic cache reached max size, remaining evaluation metrics for this analytics interval will not be sent", sdk_codes.EvaluationMetricsMaxSizeReached) } + as.evaluationsAnalyticsMx.Unlock() // Check if target is nil or anonymous if ad.target == nil || (ad.target.Anonymous != nil && *ad.target.Anonymous) { @@ -159,7 +156,11 @@ func (as *AnalyticsService) listener() { // Update target metrics as.targetAnalyticsMx.Lock() - as.targetAnalytics[ad.target.Identifier] = *ad.target + if len(as.targetAnalytics) < maxTargetEntries { + as.targetAnalytics[ad.target.Identifier] = *ad.target + } else { + as.logger.Warnf("%s Target analytics cache reached max size, remaining target metrics for this analytics interval will not be sent", sdk_codes.TargetMetricsMaxSizeReached) + } as.targetAnalyticsMx.Unlock() } } From 132b37c493ba778a8ad3651d82cff18e28084b27 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 12:58:14 +0100 Subject: [PATCH 33/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/safe_evaluations_map.go | 33 +++++++ analyticsservice/safe_maps_test.go | 107 ++++++++++++++++++++++ analyticsservice/safe_seen_targets_map.go | 33 +++++++ analyticsservice/safe_target_map.go | 37 ++++++++ 4 files changed, 210 insertions(+) create mode 100644 analyticsservice/safe_evaluations_map.go create mode 100644 analyticsservice/safe_maps_test.go create mode 100644 analyticsservice/safe_seen_targets_map.go create mode 100644 analyticsservice/safe_target_map.go diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go new file mode 100644 index 0000000..1dbe406 --- /dev/null +++ b/analyticsservice/safe_evaluations_map.go @@ -0,0 +1,33 @@ +package analyticsservice + +import "sync" + +type safeEvaluationAnalytics struct { + sync.RWMutex + data map[string]analyticsEvent +} + +func newSafeEvaluationAnalytics() *safeEvaluationAnalytics { + return &safeEvaluationAnalytics{ + data: make(map[string]analyticsEvent), + } +} + +func (s *safeEvaluationAnalytics) set(key string, value analyticsEvent) { + s.Lock() + defer s.Unlock() + s.data[key] = value +} + +func (s *safeEvaluationAnalytics) get(key string) (analyticsEvent, bool) { + s.RLock() + defer s.RUnlock() + val, exists := s.data[key] + return val, exists +} + +func (s *safeEvaluationAnalytics) delete(key string) { + s.Lock() + defer s.Unlock() + delete(s.data, key) +} diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go new file mode 100644 index 0000000..5e7878a --- /dev/null +++ b/analyticsservice/safe_maps_test.go @@ -0,0 +1,107 @@ +package analyticsservice + +import ( + "reflect" + "sync" + "testing" + + "github.com/harness/ff-golang-server-sdk/evaluation" +) + +// SafeMap is a generic thread-safe map +type SafeMap[K comparable, V any] struct { + sync.RWMutex + data map[K]V +} + +// NewSafeMap creates a new SafeMap +func NewSafeMap[K comparable, V any]() *SafeMap[K, V] { + return &SafeMap[K, V]{ + data: make(map[K]V), + } +} + +// Set sets a value in the map +func (s *SafeMap[K, V]) Set(key K, value V) { + s.Lock() + defer s.Unlock() + s.data[key] = value +} + +// Get retrieves a value from the map +func (s *SafeMap[K, V]) Get(key K) (V, bool) { + s.RLock() + defer s.RUnlock() + val, exists := s.data[key] + return val, exists +} + +// Delete removes a value from the map +func (s *SafeMap[K, V]) Delete(key K) { + s.Lock() + defer s.Unlock() + delete(s.data, key) +} + +func testSafeMapOperations[K comparable, V any](t *testing.T, mapInstance *SafeMap[K, V], testData map[K]V) { + // Test set and get + for key, value := range testData { + mapInstance.Set(key, value) + if got, exists := mapInstance.Get(key); !exists || !reflect.DeepEqual(got, value) { + t.Errorf("set or get method failed for key %v, expected %v, got %v", key, value, got) + } + } + + // Test concurrent access + var wg sync.WaitGroup + for key, value := range testData { + wg.Add(1) + go func(k K, v V) { + defer wg.Done() + mapInstance.Set(k, v) + if got, exists := mapInstance.Get(k); !exists || !reflect.DeepEqual(got, v) { + t.Errorf("concurrent set or get failed for key %v, expected %v, got %v", k, v, got) + } + }(key, value) + } + wg.Wait() + + // Test delete + for key := range testData { + mapInstance.Delete(key) + if _, exists := mapInstance.Get(key); exists { + t.Errorf("delete method failed, %v should have been deleted", key) + } + } +} + +func TestSafeEvaluationAnalytics(t *testing.T) { + EvaluationAnalytics := NewSafeMap[string, analyticsEvent]() + testData := map[string]analyticsEvent{ + "test-key": {count: 1}, + "key-1": {count: 10}, + "key-2": {count: 20}, + } + testSafeMapOperations(t, EvaluationAnalytics, testData) +} + +func TestSafeTargetAnalytics(t *testing.T) { + TargetAnalytics := NewSafeMap[string, evaluation.Target]() + testData := map[string]evaluation.Target{ + "test-target": {Identifier: "test-identifier"}, + "target-1": {Identifier: "id-10"}, + "target-2": {Identifier: "id-20"}, + } + testSafeMapOperations(t, TargetAnalytics, testData) +} + +func TestSafeSeenTargets(t *testing.T) { + SeenTargets := NewSafeMap[string, bool]() + + testData := map[string]bool{ + "test-seen": true, + "seen-1": false, + "seen-2": true, + } + testSafeMapOperations(t, SeenTargets, testData) +} diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go new file mode 100644 index 0000000..42882e9 --- /dev/null +++ b/analyticsservice/safe_seen_targets_map.go @@ -0,0 +1,33 @@ +package analyticsservice + +import "sync" + +type safeSeenTargets struct { + sync.RWMutex + data map[string]bool +} + +func newSafeSeenTargets() *safeSeenTargets { + return &safeSeenTargets{ + data: make(map[string]bool), + } +} + +func (s *safeSeenTargets) set(key string, seen bool) { + s.Lock() + defer s.Unlock() + s.data[key] = seen +} + +func (s *safeSeenTargets) get(key string) (bool, bool) { + s.RLock() + defer s.RUnlock() + seen, exists := s.data[key] + return seen, exists +} + +func (s *safeSeenTargets) delete(key string) { + s.Lock() + defer s.Unlock() + delete(s.data, key) +} diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go new file mode 100644 index 0000000..f78f2a4 --- /dev/null +++ b/analyticsservice/safe_target_map.go @@ -0,0 +1,37 @@ +package analyticsservice + +import ( + "sync" + + "github.com/harness/ff-golang-server-sdk/evaluation" +) + +type safeTargetAnalytics struct { + sync.RWMutex + data map[string]evaluation.Target +} + +func newSafeTargetAnalytics() *safeTargetAnalytics { + return &safeTargetAnalytics{ + data: make(map[string]evaluation.Target), + } +} + +func (s *safeTargetAnalytics) set(key string, value evaluation.Target) { + s.Lock() + defer s.Unlock() + s.data[key] = value +} + +func (s *safeTargetAnalytics) get(key string) (evaluation.Target, bool) { + s.RLock() + defer s.RUnlock() + val, exists := s.data[key] + return val, exists +} + +func (s *safeTargetAnalytics) delete(key string) { + s.Lock() + defer s.Unlock() + delete(s.data, key) +} From 4a262bbf7c54a13a2e8443a3dbe4e007c186f68a Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 13:16:54 +0100 Subject: [PATCH 34/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/safe_maps_test.go | 105 +++++++++++------------------ 1 file changed, 39 insertions(+), 66 deletions(-) diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 5e7878a..3e37fd6 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -8,100 +8,73 @@ import ( "github.com/harness/ff-golang-server-sdk/evaluation" ) -// SafeMap is a generic thread-safe map -type SafeMap[K comparable, V any] struct { - sync.RWMutex - data map[K]V -} - -// NewSafeMap creates a new SafeMap -func NewSafeMap[K comparable, V any]() *SafeMap[K, V] { - return &SafeMap[K, V]{ - data: make(map[K]V), - } -} - -// Set sets a value in the map -func (s *SafeMap[K, V]) Set(key K, value V) { - s.Lock() - defer s.Unlock() - s.data[key] = value -} - -// Get retrieves a value from the map -func (s *SafeMap[K, V]) Get(key K) (V, bool) { - s.RLock() - defer s.RUnlock() - val, exists := s.data[key] - return val, exists -} - -// Delete removes a value from the map -func (s *SafeMap[K, V]) Delete(key K) { - s.Lock() - defer s.Unlock() - delete(s.data, key) -} - -func testSafeMapOperations[K comparable, V any](t *testing.T, mapInstance *SafeMap[K, V], testData map[K]V) { - // Test set and get +func testSafeMapOperations[K comparable, V any](t *testing.T, testData map[K]V, setFunc func(K, V), getFunc func(K) (V, bool), deleteFunc func(K)) { for key, value := range testData { - mapInstance.Set(key, value) - if got, exists := mapInstance.Get(key); !exists || !reflect.DeepEqual(got, value) { + setFunc(key, value) + if got, exists := getFunc(key); !exists || !reflect.DeepEqual(got, value) { t.Errorf("set or get method failed for key %v, expected %v, got %v", key, value, got) } } - // Test concurrent access var wg sync.WaitGroup for key, value := range testData { wg.Add(1) go func(k K, v V) { defer wg.Done() - mapInstance.Set(k, v) - if got, exists := mapInstance.Get(k); !exists || !reflect.DeepEqual(got, v) { - t.Errorf("concurrent set or get failed for key %v, expected %v, got %v", k, v, got) + setFunc(k, v) + if got, exists := getFunc(k); !exists || !reflect.DeepEqual(got, v) { + t.Errorf("Concurrent set or get failed for key %v, expected %v, got %v", k, v, got) } }(key, value) } wg.Wait() - // Test delete for key := range testData { - mapInstance.Delete(key) - if _, exists := mapInstance.Get(key); exists { + deleteFunc(key) + if _, exists := getFunc(key); exists { t.Errorf("delete method failed, %v should have been deleted", key) } } } -func TestSafeEvaluationAnalytics(t *testing.T) { - EvaluationAnalytics := NewSafeMap[string, analyticsEvent]() - testData := map[string]analyticsEvent{ - "test-key": {count: 1}, - "key-1": {count: 10}, - "key-2": {count: 20}, +func TestSafeTargetAnalytics(t *testing.T) { + s := newSafeTargetAnalytics() + testData := map[string]evaluation.Target{ + "target1": {Identifier: "id1"}, + "target2": {Identifier: "id2"}, } - testSafeMapOperations(t, EvaluationAnalytics, testData) + + testSafeMapOperations(t, testData, + func(key string, value evaluation.Target) { s.set(key, value) }, + func(key string) (evaluation.Target, bool) { return s.get(key) }, + func(key string) { s.delete(key) }, + ) } -func TestSafeTargetAnalytics(t *testing.T) { - TargetAnalytics := NewSafeMap[string, evaluation.Target]() - testData := map[string]evaluation.Target{ - "test-target": {Identifier: "test-identifier"}, - "target-1": {Identifier: "id-10"}, - "target-2": {Identifier: "id-20"}, +func TestSafeEvaluationAnalytics(t *testing.T) { + s := newSafeEvaluationAnalytics() + testData := map[string]analyticsEvent{ + "event1": {count: 1}, + "event2": {count: 2}, } - testSafeMapOperations(t, TargetAnalytics, testData) + + testSafeMapOperations(t, testData, + func(key string, value analyticsEvent) { s.set(key, value) }, + func(key string) (analyticsEvent, bool) { return s.get(key) }, + func(key string) { s.delete(key) }, + ) } func TestSafeSeenTargets(t *testing.T) { - SeenTargets := NewSafeMap[string, bool]() - + s := newSafeSeenTargets() testData := map[string]bool{ - "test-seen": true, - "seen-1": false, - "seen-2": true, + "seen1": true, + "seen2": false, } - testSafeMapOperations(t, SeenTargets, testData) + + testSafeMapOperations(t, testData, + func(key string, value bool) { s.set(key, value) }, + func(key string) (bool, bool) { return s.get(key) }, + func(key string) { s.delete(key) }, + ) } From 644c9a3c44824978fa06d557833cc23dad630b44 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 15:37:22 +0100 Subject: [PATCH 35/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/analytics.go | 6 + analyticsservice/safe_evaluations_map.go | 2 +- analyticsservice/safe_maps_test.go | 160 ++++++++++++++++------ analyticsservice/safe_seen_targets_map.go | 6 +- analyticsservice/safe_target_map.go | 2 +- 5 files changed, 131 insertions(+), 45 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 2e53e2d..5f8daeb 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -36,6 +36,12 @@ const ( maxTargetEntries int = 100000 ) +type MapOperations[K comparable, V any] interface { + set(key K, value V) + get(key K) (V, bool) + delete(key K) +} + type analyticsEvent struct { target *evaluation.Target featureConfig *rest.FeatureConfig diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index 1dbe406..945438b 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -7,7 +7,7 @@ type safeEvaluationAnalytics struct { data map[string]analyticsEvent } -func newSafeEvaluationAnalytics() *safeEvaluationAnalytics { +func newSafeEvaluationAnalytics() MapOperations[string, analyticsEvent] { return &safeEvaluationAnalytics{ data: make(map[string]analyticsEvent), } diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 3e37fd6..fe504e8 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -8,33 +8,48 @@ import ( "github.com/harness/ff-golang-server-sdk/evaluation" ) -func testSafeMapOperations[K comparable, V any](t *testing.T, testData map[K]V, setFunc func(K, V), getFunc func(K) (V, bool), deleteFunc func(K)) { - for key, value := range testData { - setFunc(key, value) - if got, exists := getFunc(key); !exists || !reflect.DeepEqual(got, value) { - t.Errorf("set or get method failed for key %v, expected %v, got %v", key, value, got) - } - } - +func testMapOperations[K comparable, V any](t *testing.T, mapInstance MapOperations[K, V], testData map[K]V) { var wg sync.WaitGroup + + // Test concurrent sets and gets for key, value := range testData { wg.Add(1) go func(k K, v V) { defer wg.Done() - setFunc(k, v) - if got, exists := getFunc(k); !exists || !reflect.DeepEqual(got, v) { - t.Errorf("Concurrent set or get failed for key %v, expected %v, got %v", k, v, got) + mapInstance.set(k, v) + if got, exists := mapInstance.get(k); !exists || !reflect.DeepEqual(got, v) { + t.Errorf("Concurrent set or get method failed for key %v, expected %v, got %v", k, v, got) } }(key, value) } wg.Wait() + // Test concurrent deletes for key := range testData { - deleteFunc(key) - if _, exists := getFunc(key); exists { - t.Errorf("delete method failed, %v should have been deleted", key) - } + wg.Add(1) + go func(k K) { + defer wg.Done() + mapInstance.delete(k) + if _, exists := mapInstance.get(k); exists { + t.Errorf("Concurrent delete method failed, %v should have been deleted", k) + } + }(key) + } + wg.Wait() // Wait for all delete operations to complete +} + +func TestSafeEvaluationAnalytics(t *testing.T) { + s := newSafeEvaluationAnalytics() + testData := map[string]analyticsEvent{ + "event1": {count: 10}, + "event2": {count: 5}, + "event3": {count: 5}, + "event4": {count: 3}, + "event5": {count: 2}, + "event6": {count: 1}, } + + testMapOperations[string, analyticsEvent](t, s, testData) } func TestSafeTargetAnalytics(t *testing.T) { @@ -42,39 +57,102 @@ func TestSafeTargetAnalytics(t *testing.T) { testData := map[string]evaluation.Target{ "target1": {Identifier: "id1"}, "target2": {Identifier: "id2"}, + "target3": {Identifier: "id3"}, + "target4": {Identifier: "id4"}, + "target5": {Identifier: "id5"}, } - testSafeMapOperations(t, testData, - func(key string, value evaluation.Target) { s.set(key, value) }, - func(key string) (evaluation.Target, bool) { return s.get(key) }, - func(key string) { s.delete(key) }, - ) -} - -func TestSafeEvaluationAnalytics(t *testing.T) { - s := newSafeEvaluationAnalytics() - testData := map[string]analyticsEvent{ - "event1": {count: 1}, - "event2": {count: 2}, - } - - testSafeMapOperations(t, testData, - func(key string, value analyticsEvent) { s.set(key, value) }, - func(key string) (analyticsEvent, bool) { return s.get(key) }, - func(key string) { s.delete(key) }, - ) + testMapOperations[string, evaluation.Target](t, s, testData) } func TestSafeSeenTargets(t *testing.T) { s := newSafeSeenTargets() testData := map[string]bool{ - "seen1": true, - "seen2": false, + "target1": true, + "target21": true, + "target3": true, + "target4": true, } - testSafeMapOperations(t, testData, - func(key string, value bool) { s.set(key, value) }, - func(key string) (bool, bool) { return s.get(key) }, - func(key string) { s.delete(key) }, - ) + testMapOperations[string, bool](t, s, testData) } + +// +//import ( +// "reflect" +// "sync" +// "testing" +// +// "github.com/harness/ff-golang-server-sdk/evaluation" +//) +// +//func testSafeMapOperations[K comparable, V any](t *testing.T, testData map[K]V, setFunc func(K, V), getFunc func(K) (V, bool), deleteFunc func(K)) { +// for key, value := range testData { +// setFunc(key, value) +// if got, exists := getFunc(key); !exists || !reflect.DeepEqual(got, value) { +// t.Errorf("set or get method failed for key %v, expected %v, got %v", key, value, got) +// } +// } +// +// var wg sync.WaitGroup +// for key, value := range testData { +// wg.Add(1) +// go func(k K, v V) { +// defer wg.Done() +// setFunc(k, v) +// if got, exists := getFunc(k); !exists || !reflect.DeepEqual(got, v) { +// t.Errorf("Concurrent set or get failed for key %v, expected %v, got %v", k, v, got) +// } +// }(key, value) +// } +// wg.Wait() +// +// for key := range testData { +// deleteFunc(key) +// if _, exists := getFunc(key); exists { +// t.Errorf("delete method failed, %v should have been deleted", key) +// } +// } +//} +// +//func TestSafeTargetAnalytics(t *testing.T) { +// s := newSafeTargetAnalytics() +// testData := map[string]evaluation.Target{ +// "target1": {Identifier: "id1"}, +// "target2": {Identifier: "id2"}, +// } +// +// testSafeMapOperations(t, testData, +// func(key string, value evaluation.Target) { s.set(key, value) }, +// func(key string) (evaluation.Target, bool) { return s.get(key) }, +// func(key string) { s.delete(key) }, +// ) +//} +// +//func TestSafeEvaluationAnalytics(t *testing.T) { +// s := newSafeEvaluationAnalytics() +// testData := map[string]analyticsEvent{ +// "event1": {count: 1}, +// "event2": {count: 2}, +// } +// +// testSafeMapOperations(t, testData, +// func(key string, value analyticsEvent) { s.set(key, value) }, +// func(key string) (analyticsEvent, bool) { return s.get(key) }, +// func(key string) { s.delete(key) }, +// ) +//} +// +//func TestSafeSeenTargets(t *testing.T) { +// s := newSafeSeenTargets() +// testData := map[string]bool{ +// "seen1": true, +// "seen2": false, +// } +// +// testSafeMapOperations(t, testData, +// func(key string, value bool) { s.set(key, value) }, +// func(key string) (bool, bool) { return s.get(key) }, +// func(key string) { s.delete(key) }, +// ) +//} diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 42882e9..81a11b3 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -1,13 +1,15 @@ package analyticsservice -import "sync" +import ( + "sync" +) type safeSeenTargets struct { sync.RWMutex data map[string]bool } -func newSafeSeenTargets() *safeSeenTargets { +func newSafeSeenTargets() MapOperations[string, bool] { return &safeSeenTargets{ data: make(map[string]bool), } diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go index f78f2a4..cce20ea 100644 --- a/analyticsservice/safe_target_map.go +++ b/analyticsservice/safe_target_map.go @@ -11,7 +11,7 @@ type safeTargetAnalytics struct { data map[string]evaluation.Target } -func newSafeTargetAnalytics() *safeTargetAnalytics { +func newSafeTargetAnalytics() MapOperations[string, evaluation.Target] { return &safeTargetAnalytics{ data: make(map[string]evaluation.Target), } From 28ee9e4f45a95d123aeb22e75d19b017887cba63 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 15:37:39 +0100 Subject: [PATCH 36/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/safe_maps_test.go | 80 ------------------------------ 1 file changed, 80 deletions(-) diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index fe504e8..30e8dbc 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -76,83 +76,3 @@ func TestSafeSeenTargets(t *testing.T) { testMapOperations[string, bool](t, s, testData) } - -// -//import ( -// "reflect" -// "sync" -// "testing" -// -// "github.com/harness/ff-golang-server-sdk/evaluation" -//) -// -//func testSafeMapOperations[K comparable, V any](t *testing.T, testData map[K]V, setFunc func(K, V), getFunc func(K) (V, bool), deleteFunc func(K)) { -// for key, value := range testData { -// setFunc(key, value) -// if got, exists := getFunc(key); !exists || !reflect.DeepEqual(got, value) { -// t.Errorf("set or get method failed for key %v, expected %v, got %v", key, value, got) -// } -// } -// -// var wg sync.WaitGroup -// for key, value := range testData { -// wg.Add(1) -// go func(k K, v V) { -// defer wg.Done() -// setFunc(k, v) -// if got, exists := getFunc(k); !exists || !reflect.DeepEqual(got, v) { -// t.Errorf("Concurrent set or get failed for key %v, expected %v, got %v", k, v, got) -// } -// }(key, value) -// } -// wg.Wait() -// -// for key := range testData { -// deleteFunc(key) -// if _, exists := getFunc(key); exists { -// t.Errorf("delete method failed, %v should have been deleted", key) -// } -// } -//} -// -//func TestSafeTargetAnalytics(t *testing.T) { -// s := newSafeTargetAnalytics() -// testData := map[string]evaluation.Target{ -// "target1": {Identifier: "id1"}, -// "target2": {Identifier: "id2"}, -// } -// -// testSafeMapOperations(t, testData, -// func(key string, value evaluation.Target) { s.set(key, value) }, -// func(key string) (evaluation.Target, bool) { return s.get(key) }, -// func(key string) { s.delete(key) }, -// ) -//} -// -//func TestSafeEvaluationAnalytics(t *testing.T) { -// s := newSafeEvaluationAnalytics() -// testData := map[string]analyticsEvent{ -// "event1": {count: 1}, -// "event2": {count: 2}, -// } -// -// testSafeMapOperations(t, testData, -// func(key string, value analyticsEvent) { s.set(key, value) }, -// func(key string) (analyticsEvent, bool) { return s.get(key) }, -// func(key string) { s.delete(key) }, -// ) -//} -// -//func TestSafeSeenTargets(t *testing.T) { -// s := newSafeSeenTargets() -// testData := map[string]bool{ -// "seen1": true, -// "seen2": false, -// } -// -// testSafeMapOperations(t, testData, -// func(key string, value bool) { s.set(key, value) }, -// func(key string) (bool, bool) { return s.get(key) }, -// func(key string) { s.delete(key) }, -// ) -//} From 51ffe93d6ee20a42bee7f038ff7ae2bd9857607a Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 16:41:44 +0100 Subject: [PATCH 37/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/analytics.go | 70 +++++++++-------------- analyticsservice/safe_evaluations_map.go | 8 ++- analyticsservice/safe_maps_test.go | 2 +- analyticsservice/safe_seen_targets_map.go | 8 ++- analyticsservice/safe_target_map.go | 14 ++++- 5 files changed, 55 insertions(+), 47 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 5f8daeb..05b4ca5 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "strconv" - "sync" "time" "github.com/harness/ff-golang-server-sdk/sdk_codes" @@ -36,10 +35,13 @@ const ( maxTargetEntries int = 100000 ) -type MapOperations[K comparable, V any] interface { +// SafeCache is a type that provides thread safe access to maps +type SafeCache[K comparable, V any] interface { set(key K, value V) get(key K) (V, bool) delete(key K) + size() int + clear() } type analyticsEvent struct { @@ -51,17 +53,14 @@ type analyticsEvent struct { // AnalyticsService provides a way to cache and send analytics to the server type AnalyticsService struct { - analyticsChan chan analyticsEvent - evaluationsAnalyticsMx *sync.Mutex - targetAnalyticsMx *sync.Mutex - seenTargetsMx *sync.RWMutex - evaluationAnalytics map[string]analyticsEvent - targetAnalytics map[string]evaluation.Target - seenTargets map[string]bool - timeout time.Duration - logger logger.Logger - metricsClient metricsclient.ClientWithResponsesInterface - environmentID string + analyticsChan chan analyticsEvent + evaluationAnalytics SafeCache[string, analyticsEvent] + targetAnalytics SafeCache[string, evaluation.Target] + seenTargets SafeCache[string, bool] + timeout time.Duration + logger logger.Logger + metricsClient metricsclient.ClientWithResponsesInterface + environmentID string } // NewAnalyticsService creates and starts a analytics service to send data to the client @@ -73,15 +72,12 @@ func NewAnalyticsService(timeout time.Duration, logger logger.Logger) *Analytics serviceTimeout = 1 * time.Hour } as := AnalyticsService{ - evaluationsAnalyticsMx: &sync.Mutex{}, - targetAnalyticsMx: &sync.Mutex{}, - seenTargetsMx: &sync.RWMutex{}, - analyticsChan: make(chan analyticsEvent), - evaluationAnalytics: map[string]analyticsEvent{}, - targetAnalytics: map[string]evaluation.Target{}, - seenTargets: map[string]bool{}, - timeout: serviceTimeout, - logger: logger, + analyticsChan: make(chan analyticsEvent), + evaluationAnalytics: newSafeEvaluationAnalytics(), + targetAnalytics: newSafeTargetAnalytics(), + seenTargets: newSafeSeenTargets(), + timeout: serviceTimeout, + logger: logger, } go as.listener() @@ -124,22 +120,20 @@ func (as *AnalyticsService) listener() { for ad := range as.analyticsChan { analyticsKey := getEvaluationAnalyticKey(ad) - as.evaluationsAnalyticsMx.Lock() // Check if we've hit capacity for evaluations - if len(as.evaluationAnalytics) < maxAnalyticsEntries { + if as.evaluationAnalytics.size() < maxAnalyticsEntries { // Update evaluation metrics - analytic, ok := as.evaluationAnalytics[analyticsKey] + analytic, ok := as.evaluationAnalytics.get(analyticsKey) if !ok { ad.count = 1 - as.evaluationAnalytics[analyticsKey] = ad + as.evaluationAnalytics.set(analyticsKey, ad) } else { ad.count = analytic.count + 1 - as.evaluationAnalytics[analyticsKey] = ad + as.evaluationAnalytics.set(analyticsKey, ad) } } else { as.logger.Warnf("%s Evaluation analytic cache reached max size, remaining evaluation metrics for this analytics interval will not be sent", sdk_codes.EvaluationMetricsMaxSizeReached) } - as.evaluationsAnalyticsMx.Unlock() // Check if target is nil or anonymous if ad.target == nil || (ad.target.Anonymous != nil && *ad.target.Anonymous) { @@ -147,27 +141,21 @@ func (as *AnalyticsService) listener() { } // Check if target has been seen - as.seenTargetsMx.RLock() - _, seen := as.seenTargets[ad.target.Identifier] - as.seenTargetsMx.RUnlock() + _, seen := as.seenTargets.get(ad.target.Identifier) if seen { continue } // Update seen targets - as.seenTargetsMx.Lock() - as.seenTargets[ad.target.Identifier] = true - as.seenTargetsMx.Unlock() + as.seenTargets.set(ad.target.Identifier, true) // Update target metrics - as.targetAnalyticsMx.Lock() - if len(as.targetAnalytics) < maxTargetEntries { - as.targetAnalytics[ad.target.Identifier] = *ad.target + if as.targetAnalytics.size() < maxTargetEntries { + as.targetAnalytics.set(ad.target.Identifier, *ad.target) } else { as.logger.Warnf("%s Target analytics cache reached max size, remaining target metrics for this analytics interval will not be sent", sdk_codes.TargetMetricsMaxSizeReached) } - as.targetAnalyticsMx.Unlock() } } @@ -205,16 +193,12 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { // for which locks are held, so that metrics processing does not affect flag evaluations performance. // Although this might occasionally result in the loss of some metrics during periods of high load, // it is an acceptable tradeoff to prevent extended lock periods that could degrade user code. - as.evaluationsAnalyticsMx.Lock() evaluationAnalyticsClone := as.evaluationAnalytics - as.evaluationAnalytics = map[string]analyticsEvent{} - as.evaluationsAnalyticsMx.Unlock() + as.evaluationAnalytics.clear() // Clone and reset target analytics cache for same reason. - as.targetAnalyticsMx.Lock() targetAnalyticsClone := as.targetAnalytics as.targetAnalytics = make(map[string]evaluation.Target) - as.targetAnalyticsMx.Unlock() metricData := make([]metricsclient.MetricsData, 0, len(evaluationAnalyticsClone)) targetData := make([]metricsclient.TargetData, 0, len(targetAnalyticsClone)) diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index 945438b..993826c 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -7,7 +7,7 @@ type safeEvaluationAnalytics struct { data map[string]analyticsEvent } -func newSafeEvaluationAnalytics() MapOperations[string, analyticsEvent] { +func newSafeEvaluationAnalytics() SafeCache[string, analyticsEvent] { return &safeEvaluationAnalytics{ data: make(map[string]analyticsEvent), } @@ -26,6 +26,12 @@ func (s *safeEvaluationAnalytics) get(key string) (analyticsEvent, bool) { return val, exists } +func (s *safeEvaluationAnalytics) size() int { + s.RLock() + defer s.RUnlock() + return len(s.data) +} + func (s *safeEvaluationAnalytics) delete(key string) { s.Lock() defer s.Unlock() diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 30e8dbc..8db63dc 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -8,7 +8,7 @@ import ( "github.com/harness/ff-golang-server-sdk/evaluation" ) -func testMapOperations[K comparable, V any](t *testing.T, mapInstance MapOperations[K, V], testData map[K]V) { +func testMapOperations[K comparable, V any](t *testing.T, mapInstance SafeCache[K, V], testData map[K]V) { var wg sync.WaitGroup // Test concurrent sets and gets diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 81a11b3..647bba0 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -9,7 +9,7 @@ type safeSeenTargets struct { data map[string]bool } -func newSafeSeenTargets() MapOperations[string, bool] { +func newSafeSeenTargets() SafeCache[string, bool] { return &safeSeenTargets{ data: make(map[string]bool), } @@ -33,3 +33,9 @@ func (s *safeSeenTargets) delete(key string) { defer s.Unlock() delete(s.data, key) } + +func (s *safeSeenTargets) size() int { + s.RLock() + defer s.RUnlock() + return len(s.data) +} diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go index cce20ea..ac2ed9b 100644 --- a/analyticsservice/safe_target_map.go +++ b/analyticsservice/safe_target_map.go @@ -11,7 +11,7 @@ type safeTargetAnalytics struct { data map[string]evaluation.Target } -func newSafeTargetAnalytics() MapOperations[string, evaluation.Target] { +func newSafeTargetAnalytics() SafeCache[string, evaluation.Target] { return &safeTargetAnalytics{ data: make(map[string]evaluation.Target), } @@ -30,8 +30,20 @@ func (s *safeTargetAnalytics) get(key string) (evaluation.Target, bool) { return val, exists } +func (s *safeTargetAnalytics) size() int { + s.RLock() + defer s.RUnlock() + return len(s.data) +} + func (s *safeTargetAnalytics) delete(key string) { s.Lock() defer s.Unlock() delete(s.data, key) } + +func (s *safeTargetAnalytics) clear() { + s.Lock() + defer s.Unlock() + s.data = make(map[string]evaluation.Target) +} From 220e8599a3eb30a4238ccae3d1a01147c2db58aa Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 16:50:51 +0100 Subject: [PATCH 38/44] FFM-11212 Encapsulate maps and write tests --- analyticsservice/analytics.go | 3 ++- analyticsservice/safe_evaluations_map.go | 10 +++++++++- analyticsservice/safe_seen_targets_map.go | 6 ++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 05b4ca5..81c4571 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -196,9 +196,10 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { evaluationAnalyticsClone := as.evaluationAnalytics as.evaluationAnalytics.clear() + // TODO revist cloning - do we need to? we're making a slice so just clear it after making the slice // Clone and reset target analytics cache for same reason. targetAnalyticsClone := as.targetAnalytics - as.targetAnalytics = make(map[string]evaluation.Target) + as.targetAnalytics.clear() metricData := make([]metricsclient.MetricsData, 0, len(evaluationAnalyticsClone)) targetData := make([]metricsclient.TargetData, 0, len(targetAnalyticsClone)) diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index 993826c..b10cb76 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -1,6 +1,8 @@ package analyticsservice -import "sync" +import ( + "sync" +) type safeEvaluationAnalytics struct { sync.RWMutex @@ -37,3 +39,9 @@ func (s *safeEvaluationAnalytics) delete(key string) { defer s.Unlock() delete(s.data, key) } + +func (s *safeEvaluationAnalytics) clear() { + s.Lock() + defer s.Unlock() + s.data = make(map[string]analyticsEvent) +} diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 647bba0..5ba3733 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -39,3 +39,9 @@ func (s *safeSeenTargets) size() int { defer s.RUnlock() return len(s.data) } + +func (s *safeSeenTargets) clear() { + s.Lock() + defer s.Unlock() + s.data = make(map[string]bool) +} From 196154a435c6e8db17957388f58a3a6bb4240495 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 17:54:52 +0100 Subject: [PATCH 39/44] FFM-11212 Add copy method --- analyticsservice/analytics.go | 1 + analyticsservice/safe_evaluations_map.go | 11 +++++++++++ analyticsservice/safe_seen_targets_map.go | 11 +++++++++++ analyticsservice/safe_target_map.go | 11 +++++++++++ 4 files changed, 34 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 81c4571..aa978cf 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -41,6 +41,7 @@ type SafeCache[K comparable, V any] interface { get(key K) (V, bool) delete(key K) size() int + copy() SafeCache[K, V] clear() } diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index b10cb76..77b8caf 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -1,6 +1,7 @@ package analyticsservice import ( + "maps" "sync" ) @@ -40,6 +41,16 @@ func (s *safeEvaluationAnalytics) delete(key string) { delete(s.data, key) } +func (s *safeEvaluationAnalytics) copy() SafeCache[string, analyticsEvent] { + s.RLock() + defer s.RUnlock() + deepCopy := make(map[string]analyticsEvent) + maps.Copy(s.data, deepCopy) + return &safeEvaluationAnalytics{ + data: deepCopy, + } +} + func (s *safeEvaluationAnalytics) clear() { s.Lock() defer s.Unlock() diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 5ba3733..97c2c33 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -1,6 +1,7 @@ package analyticsservice import ( + "maps" "sync" ) @@ -40,6 +41,16 @@ func (s *safeSeenTargets) size() int { return len(s.data) } +func (s *safeSeenTargets) copy() SafeCache[string, bool] { + s.RLock() + defer s.RUnlock() + deepCopy := make(map[string]bool) + maps.Copy(s.data, deepCopy) + return &safeSeenTargets{ + data: deepCopy, + } +} + func (s *safeSeenTargets) clear() { s.Lock() defer s.Unlock() diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go index ac2ed9b..1859c4e 100644 --- a/analyticsservice/safe_target_map.go +++ b/analyticsservice/safe_target_map.go @@ -1,6 +1,7 @@ package analyticsservice import ( + "maps" "sync" "github.com/harness/ff-golang-server-sdk/evaluation" @@ -42,6 +43,16 @@ func (s *safeTargetAnalytics) delete(key string) { delete(s.data, key) } +func (s *safeTargetAnalytics) copy() SafeCache[string, evaluation.Target] { + s.RLock() + defer s.RUnlock() + deepCopy := make(map[string]evaluation.Target) + maps.Copy(s.data, deepCopy) + return &safeTargetAnalytics{ + data: deepCopy, + } +} + func (s *safeTargetAnalytics) clear() { s.Lock() defer s.Unlock() From 4ea3703b170cf2672ea8d2188d284e79dbdf585e Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 18:44:39 +0100 Subject: [PATCH 40/44] FFM-11212 Add copy method --- analyticsservice/analytics.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index aa978cf..0584a0f 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -195,7 +195,9 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { // Although this might occasionally result in the loss of some metrics during periods of high load, // it is an acceptable tradeoff to prevent extended lock periods that could degrade user code. evaluationAnalyticsClone := as.evaluationAnalytics + as.evaluationAnalytics.clear() + as.evaluationAnalytics = newSafeEvaluationAnalytics() // TODO revist cloning - do we need to? we're making a slice so just clear it after making the slice // Clone and reset target analytics cache for same reason. From bc112ec9981e77cf3d23271095c58134cae4ac0a Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 19:14:10 +0100 Subject: [PATCH 41/44] FFM-11212 Add iterate methpd --- analyticsservice/analytics.go | 18 ++++++++---------- analyticsservice/safe_evaluations_map.go | 19 ++++++++----------- analyticsservice/safe_seen_targets_map.go | 19 ++++++++----------- analyticsservice/safe_target_map.go | 23 +++++++++++++---------- 4 files changed, 37 insertions(+), 42 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index 0584a0f..e024699 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -41,8 +41,8 @@ type SafeCache[K comparable, V any] interface { get(key K) (V, bool) delete(key K) size() int - copy() SafeCache[K, V] clear() + iterate(func(K, V)) } type analyticsEvent struct { @@ -196,19 +196,17 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { // it is an acceptable tradeoff to prevent extended lock periods that could degrade user code. evaluationAnalyticsClone := as.evaluationAnalytics - as.evaluationAnalytics.clear() as.evaluationAnalytics = newSafeEvaluationAnalytics() - // TODO revist cloning - do we need to? we're making a slice so just clear it after making the slice // Clone and reset target analytics cache for same reason. targetAnalyticsClone := as.targetAnalytics - as.targetAnalytics.clear() + as.targetAnalytics = newSafeTargetAnalytics() - metricData := make([]metricsclient.MetricsData, 0, len(evaluationAnalyticsClone)) - targetData := make([]metricsclient.TargetData, 0, len(targetAnalyticsClone)) + metricData := make([]metricsclient.MetricsData, 0, evaluationAnalyticsClone.size()) + targetData := make([]metricsclient.TargetData, 0, targetAnalyticsClone.size()) // Process evaluation metrics - for _, analytic := range evaluationAnalyticsClone { + evaluationAnalyticsClone.iterate(func(key string, analytic analyticsEvent) { metricAttributes := []metricsclient.KeyValue{ {Key: featureIdentifierAttribute, Value: analytic.featureConfig.Feature}, {Key: featureNameAttribute, Value: analytic.featureConfig.Feature}, @@ -227,10 +225,10 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { Attributes: metricAttributes, } metricData = append(metricData, md) - } + }) // Process target metrics - for _, target := range targetAnalyticsClone { + targetAnalyticsClone.iterate(func(key string, target evaluation.Target) { targetAttributes := make([]metricsclient.KeyValue, 0) for key, value := range *target.Attributes { targetAttributes = append(targetAttributes, metricsclient.KeyValue{Key: key, Value: convertInterfaceToString(value)}) @@ -242,7 +240,7 @@ func (as *AnalyticsService) sendDataAndResetCache(ctx context.Context) { Attributes: targetAttributes, } targetData = append(targetData, td) - } + }) analyticsPayload := metricsclient.PostMetricsJSONRequestBody{ MetricsData: &metricData, diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index 77b8caf..1fe57a7 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -1,7 +1,6 @@ package analyticsservice import ( - "maps" "sync" ) @@ -41,18 +40,16 @@ func (s *safeEvaluationAnalytics) delete(key string) { delete(s.data, key) } -func (s *safeEvaluationAnalytics) copy() SafeCache[string, analyticsEvent] { - s.RLock() - defer s.RUnlock() - deepCopy := make(map[string]analyticsEvent) - maps.Copy(s.data, deepCopy) - return &safeEvaluationAnalytics{ - data: deepCopy, - } -} - func (s *safeEvaluationAnalytics) clear() { s.Lock() defer s.Unlock() s.data = make(map[string]analyticsEvent) } + +func (s *safeEvaluationAnalytics) iterate(f func(string, analyticsEvent)) { + s.RLock() + defer s.RUnlock() + for key, value := range s.data { + f(key, value) + } +} diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index 97c2c33..ef3fcca 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -1,7 +1,6 @@ package analyticsservice import ( - "maps" "sync" ) @@ -41,18 +40,16 @@ func (s *safeSeenTargets) size() int { return len(s.data) } -func (s *safeSeenTargets) copy() SafeCache[string, bool] { - s.RLock() - defer s.RUnlock() - deepCopy := make(map[string]bool) - maps.Copy(s.data, deepCopy) - return &safeSeenTargets{ - data: deepCopy, - } -} - func (s *safeSeenTargets) clear() { s.Lock() defer s.Unlock() s.data = make(map[string]bool) } + +func (s *safeSeenTargets) iterate(f func(string, bool)) { + s.RLock() + defer s.RUnlock() + for key, value := range s.data { + f(key, value) + } +} diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go index 1859c4e..f18721e 100644 --- a/analyticsservice/safe_target_map.go +++ b/analyticsservice/safe_target_map.go @@ -1,7 +1,6 @@ package analyticsservice import ( - "maps" "sync" "github.com/harness/ff-golang-server-sdk/evaluation" @@ -43,18 +42,22 @@ func (s *safeTargetAnalytics) delete(key string) { delete(s.data, key) } -func (s *safeTargetAnalytics) copy() SafeCache[string, evaluation.Target] { +func (s *safeTargetAnalytics) clear() { + s.Lock() + defer s.Unlock() + s.data = make(map[string]evaluation.Target) +} + +func (s *safeTargetAnalytics) iterate(f func(string, evaluation.Target)) { s.RLock() defer s.RUnlock() - deepCopy := make(map[string]evaluation.Target) - maps.Copy(s.data, deepCopy) - return &safeTargetAnalytics{ - data: deepCopy, + for key, value := range s.data { + f(key, value) } } -func (s *safeTargetAnalytics) clear() { - s.Lock() - defer s.Unlock() - s.data = make(map[string]evaluation.Target) +func (s *safeTargetAnalytics) iterateUnsafe(f func(string, evaluation.Target)) { + for key, value := range s.data { + f(key, value) + } } From 58257d31f823a38a26879c21579ebf4c1867e4f7 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 19:17:59 +0100 Subject: [PATCH 42/44] FFM-11212 Update tests --- analyticsservice/analytics_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/analyticsservice/analytics_test.go b/analyticsservice/analytics_test.go index b8bdd78..6d48c18 100644 --- a/analyticsservice/analytics_test.go +++ b/analyticsservice/analytics_test.go @@ -110,33 +110,27 @@ func TestListenerHandlesEventsCorrectly(t *testing.T) { time.Sleep(100 * time.Millisecond) // Check evaluation metrics counts - service.evaluationsAnalyticsMx.Lock() for key, expectedCount := range tc.expectedEvaluations { - analytic, exists := service.evaluationAnalytics[key] + analytic, exists := service.evaluationAnalytics.get(key) if !exists || analytic.count != expectedCount { t.Errorf("Test %s failed: expected count for key %s is %d, got %d", tc.name, key, expectedCount, analytic.count) } } - service.evaluationsAnalyticsMx.Unlock() // Check target metrics - service.seenTargetsMx.RLock() for targetID, expectedSeen := range tc.expectedSeen { - if seen := service.seenTargets[targetID]; seen != expectedSeen { + if _, seen := service.seenTargets.get(targetID); seen != expectedSeen { t.Errorf("Test %s failed: expected target to be in seen targets cache %s is %v", tc.name, targetID, expectedSeen) } } - service.seenTargetsMx.RUnlock() // Check target analytics - service.targetAnalyticsMx.Lock() for targetID, expectedTarget := range tc.expectedTargets { - target, exists := service.targetAnalytics[targetID] + target, exists := service.targetAnalytics.get(targetID) if !exists || target.Identifier != expectedTarget.Identifier { t.Errorf("Test %s failed: expected target to be in target cache %s", tc.name, targetID) } } - service.targetAnalyticsMx.Unlock() }) } } From 90d26aa7d8a6f0f04c7eddb466c9e0612e16becc Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 19:43:12 +0100 Subject: [PATCH 43/44] FFM-11212 Update tests --- analyticsservice/analytics.go | 10 ++++---- analyticsservice/safe_evaluations_map.go | 2 +- analyticsservice/safe_maps_test.go | 2 +- analyticsservice/safe_seen_targets_map.go | 2 +- analyticsservice/safe_target_map.go | 2 +- client/client_test.go | 29 +++++------------------ 6 files changed, 15 insertions(+), 32 deletions(-) diff --git a/analyticsservice/analytics.go b/analyticsservice/analytics.go index e024699..e2cca31 100644 --- a/analyticsservice/analytics.go +++ b/analyticsservice/analytics.go @@ -35,8 +35,8 @@ const ( maxTargetEntries int = 100000 ) -// SafeCache is a type that provides thread safe access to maps -type SafeCache[K comparable, V any] interface { +// SafeAnalyticsCache is a type that provides thread safe access to maps used by analytics +type SafeAnalyticsCache[K comparable, V any] interface { set(key K, value V) get(key K) (V, bool) delete(key K) @@ -55,9 +55,9 @@ type analyticsEvent struct { // AnalyticsService provides a way to cache and send analytics to the server type AnalyticsService struct { analyticsChan chan analyticsEvent - evaluationAnalytics SafeCache[string, analyticsEvent] - targetAnalytics SafeCache[string, evaluation.Target] - seenTargets SafeCache[string, bool] + evaluationAnalytics SafeAnalyticsCache[string, analyticsEvent] + targetAnalytics SafeAnalyticsCache[string, evaluation.Target] + seenTargets SafeAnalyticsCache[string, bool] timeout time.Duration logger logger.Logger metricsClient metricsclient.ClientWithResponsesInterface diff --git a/analyticsservice/safe_evaluations_map.go b/analyticsservice/safe_evaluations_map.go index 1fe57a7..8669955 100644 --- a/analyticsservice/safe_evaluations_map.go +++ b/analyticsservice/safe_evaluations_map.go @@ -9,7 +9,7 @@ type safeEvaluationAnalytics struct { data map[string]analyticsEvent } -func newSafeEvaluationAnalytics() SafeCache[string, analyticsEvent] { +func newSafeEvaluationAnalytics() SafeAnalyticsCache[string, analyticsEvent] { return &safeEvaluationAnalytics{ data: make(map[string]analyticsEvent), } diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 8db63dc..3929749 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -8,7 +8,7 @@ import ( "github.com/harness/ff-golang-server-sdk/evaluation" ) -func testMapOperations[K comparable, V any](t *testing.T, mapInstance SafeCache[K, V], testData map[K]V) { +func testMapOperations[K comparable, V any](t *testing.T, mapInstance SafeAnalyticsCache[K, V], testData map[K]V) { var wg sync.WaitGroup // Test concurrent sets and gets diff --git a/analyticsservice/safe_seen_targets_map.go b/analyticsservice/safe_seen_targets_map.go index ef3fcca..94e4a8d 100644 --- a/analyticsservice/safe_seen_targets_map.go +++ b/analyticsservice/safe_seen_targets_map.go @@ -9,7 +9,7 @@ type safeSeenTargets struct { data map[string]bool } -func newSafeSeenTargets() SafeCache[string, bool] { +func newSafeSeenTargets() SafeAnalyticsCache[string, bool] { return &safeSeenTargets{ data: make(map[string]bool), } diff --git a/analyticsservice/safe_target_map.go b/analyticsservice/safe_target_map.go index f18721e..f8a634b 100644 --- a/analyticsservice/safe_target_map.go +++ b/analyticsservice/safe_target_map.go @@ -11,7 +11,7 @@ type safeTargetAnalytics struct { data map[string]evaluation.Target } -func newSafeTargetAnalytics() SafeCache[string, evaluation.Target] { +func newSafeTargetAnalytics() SafeAnalyticsCache[string, evaluation.Target] { return &safeTargetAnalytics{ data: make(map[string]evaluation.Target), } diff --git a/client/client_test.go b/client/client_test.go index beca237..1e97e72 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -4,6 +4,12 @@ import ( "bytes" "encoding/json" "errors" + "io" + "net/http" + "os" + "testing" + "time" + "github.com/cenkalti/backoff/v4" "github.com/harness/ff-golang-server-sdk/dto" "github.com/harness/ff-golang-server-sdk/evaluation" @@ -13,11 +19,6 @@ import ( "github.com/harness/ff-golang-server-sdk/types" "github.com/jarcoal/httpmock" "github.com/stretchr/testify/assert" - "io" - "net/http" - "os" - "testing" - "time" ) const ( @@ -591,24 +592,6 @@ func TestCfClient_DefaultVariationReturned(t *testing.T) { expectedJSON: types.JSON{"a default flagIdentifier": "a default value"}, expectedError: DefaultVariationReturnedError, }, - { - name: "Evaluations with Sync client with valid sdk key and flag not found", - clientFunc: func() (*CfClient, error) { - return newClient(http.DefaultClient, ValidSDKKey, WithWaitForInitialized(true)) - }, - mockResponder: func() { - authSuccessResponse := AuthResponse(200, ValidAuthToken) - registerResponders(authSuccessResponse, TargetSegmentsResponse, FeatureConfigsResponse) - - }, - flagIdentifier: "made up", - expectedBool: false, - expectedString: "a default value", - expectedInt: 45555, - expectedNumber: 45.222, - expectedJSON: types.JSON{"a default flagIdentifier": "a default value"}, - expectedError: DefaultVariationReturnedError, - }, } target := target() From 7892eb45399a053b0c2931d3d82215b823b09673 Mon Sep 17 00:00:00 2001 From: Erdi Rowlands Date: Thu, 18 Apr 2024 20:18:57 +0100 Subject: [PATCH 44/44] FFM-11212 Update tests --- analyticsservice/safe_maps_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/analyticsservice/safe_maps_test.go b/analyticsservice/safe_maps_test.go index 3929749..fb25e1d 100644 --- a/analyticsservice/safe_maps_test.go +++ b/analyticsservice/safe_maps_test.go @@ -24,6 +24,21 @@ func testMapOperations[K comparable, V any](t *testing.T, mapInstance SafeAnalyt } wg.Wait() + // Test concurrent iteration and size + for key := range testData { + wg.Add(1) + go func(k K) { + defer wg.Done() + mapInstance.size() + mapInstance.iterate(func(k K, v V) { + if expected, exists := testData[k]; !exists || !reflect.DeepEqual(v, expected) { + t.Errorf("Iterate failed for key %v, expected %v, got %v", k, expected, v) + } + }) + }(key) + } + wg.Wait() + // Test concurrent deletes for key := range testData { wg.Add(1) @@ -35,7 +50,7 @@ func testMapOperations[K comparable, V any](t *testing.T, mapInstance SafeAnalyt } }(key) } - wg.Wait() // Wait for all delete operations to complete + wg.Wait() } func TestSafeEvaluationAnalytics(t *testing.T) {