From da59df3a9e40317ea65cf41ba08c529f65b8e751 Mon Sep 17 00:00:00 2001 From: Andrew Bell <115623869+andybharness@users.noreply.github.com> Date: Fri, 2 Dec 2022 11:44:57 +0000 Subject: [PATCH] [FFM-5471] - Target identifier set in the target builder function in the Java SDK is not used (#125) What Targets reported by Java SDK are incorrectly showing in the UI Target Management -> Targets as the global target Why We're sending the global target identifier in the targetData when we shouldn't be. Also global target is not correctly applied to metricsData when isGlobalTargetEnabled is set Testing Tested manually with sample app to confirm changes are reflected in the UI. Also added a new unit test to capture and assert on the posted MetricsData object. --- .../cf/client/api/MetricsProcessor.java | 14 +-- .../cf/client/api/MetricsProcessorTest.java | 85 ++++++++++++++++--- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/src/main/java/io/harness/cf/client/api/MetricsProcessor.java b/src/main/java/io/harness/cf/client/api/MetricsProcessor.java index 00546cba..90185b34 100644 --- a/src/main/java/io/harness/cf/client/api/MetricsProcessor.java +++ b/src/main/java/io/harness/cf/client/api/MetricsProcessor.java @@ -31,6 +31,9 @@ class MetricsProcessor extends AbstractScheduledService { private static final String SDK_LANGUAGE = "SDK_LANGUAGE"; private static final String SDK_VERSION = "SDK_VERSION"; + private static final Target globalTarget = + Target.builder().name(GLOBAL_TARGET_NAME).identifier(GLOBAL_TARGET).build(); + private final Connector connector; private final BaseConfig config; private final AtomicLongMap frequencyMap; @@ -70,11 +73,12 @@ public synchronized void pushToQueue(Target target, String featureName, Variatio uniqueTargetSet.add(target); - if (config.isGlobalTargetEnabled()) { - target.setIdentifier(GLOBAL_TARGET); + Target metricTarget = globalTarget; + if (!config.isGlobalTargetEnabled()) { + metricTarget = target; } - frequencyMap.incrementAndGet(new MetricEvent(featureName, target, variation)); + frequencyMap.incrementAndGet(new MetricEvent(featureName, metricTarget, variation)); } /** This method sends the metrics data to the analytics server and resets the cache */ @@ -117,10 +121,8 @@ protected Metrics prepareSummaryMetricsBody(Map data, Set(), new ArrayList<>()); final Map summaryMetricsData = new HashMap<>(); - addTargetData( - metrics, Target.builder().name(GLOBAL_TARGET_NAME).identifier(GLOBAL_TARGET).build()); - targets.forEach(target -> addTargetData(metrics, target)); + data.forEach( (target, count) -> summaryMetricsData.put( diff --git a/src/test/java/io/harness/cf/client/api/MetricsProcessorTest.java b/src/test/java/io/harness/cf/client/api/MetricsProcessorTest.java index b30e69e4..e8644cdb 100644 --- a/src/test/java/io/harness/cf/client/api/MetricsProcessorTest.java +++ b/src/test/java/io/harness/cf/client/api/MetricsProcessorTest.java @@ -1,22 +1,22 @@ package io.harness.cf.client.api; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.when; import com.google.common.collect.Maps; import io.harness.cf.client.connector.Connector; +import io.harness.cf.client.connector.ConnectorException; import io.harness.cf.client.dto.Target; -import io.harness.cf.model.FeatureConfig; -import io.harness.cf.model.Metrics; -import io.harness.cf.model.Variation; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; +import io.harness.cf.model.*; +import java.util.*; import java.util.concurrent.*; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.*; @Slf4j @@ -141,7 +141,7 @@ public void onMetricsFailure() { } @Test - public void testPrepareSummaryMetricsBody() { + void testPrepareSummaryMetricsBody() { Target target = Target.builder().identifier("harness").build(); FeatureConfig feature = FeatureConfig.builder().feature("bool-flag").build(); Variation variation = Variation.builder().identifier("true").value("true").build(); @@ -150,12 +150,69 @@ public void testPrepareSummaryMetricsBody() { Set uniqueTargets = new HashSet<>(); uniqueTargets.add(target); - Map map = Maps.newHashMap(); - map.put(event, 6L); + Map freqMap = Maps.newHashMap(); + freqMap.put(event, 6L); - Metrics metrics = metricsProcessor.prepareSummaryMetricsBody(map, uniqueTargets); + Metrics metrics = metricsProcessor.prepareSummaryMetricsBody(freqMap, uniqueTargets); - assert metrics.getTargetData() != null; - assert metrics.getTargetData().get(1).getIdentifier().equals(target.getIdentifier()); + assertNotNull(metrics); + assertNotNull(metrics.getTargetData()); + assertNotNull(metrics.getMetricsData()); + + assertEquals(target.getIdentifier(), metrics.getTargetData().get(0).getIdentifier()); + assertEquals(6, metrics.getMetricsData().get(0).getCount()); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void shouldPostCorrectMetrics_WhenGlobalTargetEnabledOrDisabled(boolean globalTargetEnabled) + throws ConnectorException { + final Connector mockConnector = Mockito.mock(Connector.class); + final BaseConfig mockConfig = Mockito.mock(BaseConfig.class); + final ArgumentCaptor metricsArgumentCaptor = ArgumentCaptor.forClass(Metrics.class); + + when(mockConfig.isGlobalTargetEnabled()).thenReturn(globalTargetEnabled); + when(mockConfig.getBufferSize()).thenReturn(10); + doNothing().when(mockConnector).postMetrics(metricsArgumentCaptor.capture()); + + final MetricsProcessor processor = + new MetricsProcessor(mockConnector, mockConfig, Mockito.mock(MetricsCallback.class)); + + final Target target = Target.builder().identifier("target123").build(); + final Variation variation = Variation.builder().identifier("true").value("true").build(); + processor.pushToQueue(target, "feature1", variation); + processor.pushToQueue(target, "feature1", variation); + processor.pushToQueue(target, "feature2", variation); + processor.pushToQueue(target, "feature2", variation); + processor.pushToQueue(target, "feature3", variation); + processor.pushToQueue(target, "feature3", variation); + processor.runOneIteration(); // Mimic scheduled job + + final Metrics sentMetrics = metricsArgumentCaptor.getValue(); + + assertNotNull(sentMetrics.getMetricsData()); + assertNotNull(sentMetrics.getTargetData()); + + assertEquals(1, sentMetrics.getTargetData().size()); + assertEquals("target123", sentMetrics.getTargetData().get(0).getIdentifier()); + + assertEquals(3, sentMetrics.getMetricsData().size()); + for (MetricsData md : sentMetrics.getMetricsData()) { + final Map map = keyValueArrayToMap(md.getAttributes()); + assertEquals(2, md.getCount()); + if (globalTargetEnabled) { + assertEquals("__global__cf_target", map.get("target")); + } else { + assertEquals("target123", map.get("target")); + } + } + } + + private Map keyValueArrayToMap(List keyValueList) { + final Map map = new HashMap<>(); + for (KeyValue kv : keyValueList) { + map.put(kv.getKey(), kv.getValue()); + } + return map; } }