diff --git a/src/main/java/io/split/android/client/EvaluationResult.java b/src/main/java/io/split/android/client/EvaluationResult.java index f5ecebc80..d7f16ec4c 100644 --- a/src/main/java/io/split/android/client/EvaluationResult.java +++ b/src/main/java/io/split/android/client/EvaluationResult.java @@ -1,24 +1,33 @@ package io.split.android.client; +import androidx.annotation.VisibleForTesting; + public final class EvaluationResult { private final String mTreatment; private final String mLabel; private final Long mChangeNumber; private final String mConfigurations; + private final boolean mTrackImpression; + @VisibleForTesting public EvaluationResult(String treatment, String label) { - this(treatment, label, null); + this(treatment, label, null, null, true); + } + + public EvaluationResult(String treatment, String label, boolean trackImpression) { + this(treatment, label, null, null, trackImpression); } - EvaluationResult(String treatment, String label, Long changeNumber) { - this(treatment, label, changeNumber, null); + EvaluationResult(String treatment, String label, Long changeNumber, boolean trackImpression) { + this(treatment, label, changeNumber, null, trackImpression); } - public EvaluationResult(String treatment, String label, Long changeNumber, String configurations) { + public EvaluationResult(String treatment, String label, Long changeNumber, String configurations, boolean trackImpression) { mTreatment = treatment; mLabel = label; mChangeNumber = changeNumber; mConfigurations = configurations; + mTrackImpression = trackImpression; } public String getTreatment() { @@ -36,4 +45,8 @@ public Long getChangeNumber() { public String getConfigurations() { return mConfigurations; } + + public boolean getTrackImpression() { + return mTrackImpression; + } } diff --git a/src/main/java/io/split/android/client/EvaluatorImpl.java b/src/main/java/io/split/android/client/EvaluatorImpl.java index ea15ab796..b845bf6d0 100644 --- a/src/main/java/io/split/android/client/EvaluatorImpl.java +++ b/src/main/java/io/split/android/client/EvaluatorImpl.java @@ -28,16 +28,16 @@ public EvaluationResult getTreatment(String matchingKey, String bucketingKey, St try { ParsedSplit parsedSplit = mSplitParser.parse(mSplitsStorage.get(splitName), matchingKey); if (parsedSplit == null) { - return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.DEFINITION_NOT_FOUND); + return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.DEFINITION_NOT_FOUND, true); } return getTreatment(matchingKey, bucketingKey, parsedSplit, attributes); } catch (ChangeNumberExceptionWrapper ex) { Logger.e(ex, "Catch Change Number Exception"); - return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.EXCEPTION, ex.changeNumber()); + return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.EXCEPTION, ex.changeNumber(), true); } catch (Exception e) { Logger.e(e, "Catch All Exception"); - return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.EXCEPTION); + return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.EXCEPTION, true); } } @@ -52,7 +52,7 @@ public EvaluationResult getTreatment(String matchingKey, String bucketingKey, St private EvaluationResult getTreatment(String matchingKey, String bucketingKey, ParsedSplit parsedSplit, Map attributes) throws ChangeNumberExceptionWrapper { try { if (parsedSplit.killed()) { - return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.KILLED, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment())); + return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.KILLED, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment()), parsedSplit.trackImpression()); } /* @@ -75,7 +75,7 @@ private EvaluationResult getTreatment(String matchingKey, String bucketingKey, P if (bucket > parsedSplit.trafficAllocation()) { // out of split - return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.NOT_IN_SPLIT, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment())); + return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.NOT_IN_SPLIT, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment()), parsedSplit.trackImpression()); } } @@ -84,11 +84,11 @@ private EvaluationResult getTreatment(String matchingKey, String bucketingKey, P if (parsedCondition.matcher().match(matchingKey, bucketingKey, attributes, this)) { String treatment = Splitter.getTreatment(bk, parsedSplit.seed(), parsedCondition.partitions(), parsedSplit.algo()); - return new EvaluationResult(treatment, parsedCondition.label(), parsedSplit.changeNumber(), configForTreatment(parsedSplit, treatment)); + return new EvaluationResult(treatment, parsedCondition.label(), parsedSplit.changeNumber(), configForTreatment(parsedSplit, treatment), parsedSplit.trackImpression()); } } - return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.DEFAULT_RULE, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment())); + return new EvaluationResult(parsedSplit.defaultTreatment(), TreatmentLabels.DEFAULT_RULE, parsedSplit.changeNumber(), configForTreatment(parsedSplit, parsedSplit.defaultTreatment()), parsedSplit.trackImpression()); } catch (Exception e) { throw new ChangeNumberExceptionWrapper(e, parsedSplit.changeNumber()); } diff --git a/src/main/java/io/split/android/client/impressions/DecoratedImpression.java b/src/main/java/io/split/android/client/impressions/DecoratedImpression.java new file mode 100644 index 000000000..d1114a28a --- /dev/null +++ b/src/main/java/io/split/android/client/impressions/DecoratedImpression.java @@ -0,0 +1,17 @@ +package io.split.android.client.impressions; + +import java.util.Map; + +public class DecoratedImpression extends Impression { + + private final boolean mTrackImpressions; + + public DecoratedImpression(String key, String bucketingKey, String split, String treatment, long time, String appliedRule, Long changeNumber, Map atributes, boolean trackImpressions) { + super(key, bucketingKey, split, treatment, time, appliedRule, changeNumber, atributes); + mTrackImpressions = trackImpressions; + } + + public boolean getTrackImpressions() { + return mTrackImpressions; + } +} diff --git a/src/main/java/io/split/android/client/service/impressions/StrategyImpressionManager.java b/src/main/java/io/split/android/client/service/impressions/StrategyImpressionManager.java index 5eb2a104e..35cdc8e57 100644 --- a/src/main/java/io/split/android/client/service/impressions/StrategyImpressionManager.java +++ b/src/main/java/io/split/android/client/service/impressions/StrategyImpressionManager.java @@ -8,6 +8,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import io.split.android.client.impressions.DecoratedImpression; import io.split.android.client.impressions.Impression; import io.split.android.client.service.impressions.strategy.PeriodicTracker; import io.split.android.client.service.impressions.strategy.ProcessStrategy; @@ -39,7 +40,7 @@ public void pushImpression(Impression impression) { return; } - if (track(impression)) { + if (shouldTrack(impression)) { mProcessStrategy.apply(impression); } else { mNoneStrategy.apply(impression); @@ -72,7 +73,12 @@ public void stopPeriodicRecording() { } } - private static boolean track(Impression impression) { - return true; // TODO: Placeholder method + private static boolean shouldTrack(Impression impression) { + if (impression instanceof DecoratedImpression) { + return ((DecoratedImpression) impression).getTrackImpressions(); + } else { + // default behaviour; will never get here. + return true; + } } } diff --git a/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java b/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java index a88e81747..16c4f2708 100644 --- a/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java +++ b/src/main/java/io/split/android/client/validators/TreatmentManagerImpl.java @@ -21,7 +21,7 @@ import io.split.android.client.attributes.AttributesMerger; import io.split.android.client.events.ListenableEventsManager; import io.split.android.client.events.SplitEvent; -import io.split.android.client.impressions.Impression; +import io.split.android.client.impressions.DecoratedImpression; import io.split.android.client.impressions.ImpressionListener; import io.split.android.client.storage.splits.SplitsStorage; import io.split.android.client.telemetry.model.Method; @@ -294,7 +294,8 @@ private TreatmentResult getTreatmentWithConfigWithoutMetrics(String split, Map attributes) { + private void logImpression(String matchingKey, String bucketingKey, String splitName, String result, String label, Long changeNumber, Map attributes, boolean trackImpression) { try { - mImpressionListener.log(new Impression(matchingKey, bucketingKey, splitName, result, System.currentTimeMillis(), label, changeNumber, attributes)); + mImpressionListener.log(new DecoratedImpression(matchingKey, bucketingKey, splitName, result, System.currentTimeMillis(), label, changeNumber, attributes, trackImpression)); } catch (Throwable t) { Logger.e("An error occurred logging impression: " + t.getLocalizedMessage()); } @@ -339,7 +341,7 @@ private EvaluationResult evaluateIfReady(String featureFlagName, mValidationLogger.w("the SDK is not ready, results may be incorrect for feature flag " + featureFlagName + ". Make sure to wait for SDK readiness before using this method", validationTag); mTelemetryStorageProducer.recordNonReadyUsage(); - return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.NOT_READY, null, null); + return new EvaluationResult(Treatments.CONTROL, TreatmentLabels.NOT_READY, null, null, true); } return mEvaluator.getTreatment(mMatchingKey, mBucketingKey, featureFlagName, attributes); } diff --git a/src/test/java/io/split/android/client/TreatmentManagerTest.java b/src/test/java/io/split/android/client/TreatmentManagerTest.java index a13f3e287..c1280161c 100644 --- a/src/test/java/io/split/android/client/TreatmentManagerTest.java +++ b/src/test/java/io/split/android/client/TreatmentManagerTest.java @@ -2,6 +2,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -27,6 +28,7 @@ import io.split.android.client.dtos.Split; import io.split.android.client.events.ListenableEventsManager; import io.split.android.client.events.SplitEvent; +import io.split.android.client.impressions.DecoratedImpression; import io.split.android.client.impressions.ImpressionListener; import io.split.android.client.storage.mysegments.MySegmentsStorage; import io.split.android.client.storage.mysegments.MySegmentsStorageContainer; @@ -316,6 +318,20 @@ public void evaluationWhenNotReadyLogsCorrectMessage() { verify(validationMessageLogger).w(eq("the SDK is not ready, results may be incorrect for feature flag test_split. Make sure to wait for SDK readiness before using this method"), any()); } + @Test + public void trackValueFromEvaluationResultGetsPassedInToImpression() { + Evaluator evaluatorMock = mock(Evaluator.class); + when(evaluatorMock.getTreatment(eq("matching_key"), eq("bucketing_key"), eq("test_split"), anyMap())) + .thenReturn(new EvaluationResult("test", "test")); + TreatmentManagerImpl tManager = initializeTreatmentManager(evaluatorMock); + + tManager.getTreatment("test_split", null, false); + + verify(impressionListener).log(argThat(argument -> { + return ((DecoratedImpression) argument).getTrackImpressions(); + })); + } + private void assertControl(List splitList, String treatment, Map treatmentList, SplitResult splitResult, Map splitResultList) { Assert.assertNotNull(treatment); Assert.assertEquals(Treatments.CONTROL, treatment); diff --git a/src/test/java/io/split/android/client/service/impressions/StrategyImpressionManagerTest.kt b/src/test/java/io/split/android/client/service/impressions/StrategyImpressionManagerTest.kt index 563802099..d69c4e78c 100644 --- a/src/test/java/io/split/android/client/service/impressions/StrategyImpressionManagerTest.kt +++ b/src/test/java/io/split/android/client/service/impressions/StrategyImpressionManagerTest.kt @@ -1,6 +1,6 @@ package io.split.android.client.service.impressions -import io.split.android.client.impressions.Impression +import io.split.android.client.impressions.DecoratedImpression import io.split.android.client.service.impressions.strategy.PeriodicTracker import io.split.android.client.service.impressions.strategy.ProcessStrategy import org.junit.Before @@ -8,6 +8,8 @@ import org.junit.Test import org.mockito.Mock import org.mockito.Mockito.mock import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyNoInteractions +import org.mockito.Mockito.`when` import org.mockito.MockitoAnnotations class StrategyImpressionManagerTest { @@ -37,6 +39,7 @@ class StrategyImpressionManagerTest { impressionManager.flush() verify(tracker).flush() + verify(noneTracker).flush() } @Test @@ -44,6 +47,7 @@ class StrategyImpressionManagerTest { impressionManager.startPeriodicRecording() verify(tracker).startPeriodicRecording() + verify(noneTracker).startPeriodicRecording() } @Test @@ -51,13 +55,61 @@ class StrategyImpressionManagerTest { impressionManager.stopPeriodicRecording() verify(tracker).stopPeriodicRecording() + verify(noneTracker).stopPeriodicRecording() } @Test fun `pushImpression calls apply on strategy`() { - val impression = mock(Impression::class.java) + val impression = mock(DecoratedImpression::class.java) + `when`(impression.trackImpressions).thenReturn(true) impressionManager.pushImpression(impression) verify(strategy).apply(impression) + verifyNoInteractions(noneStrategy) + } + + @Test + fun `pushImpression calls apply on noneStrategy when trackImpressions is false`() { + val impression = mock(DecoratedImpression::class.java) + `when`(impression.trackImpressions).thenReturn(false) + impressionManager.pushImpression(impression) + + verify(noneStrategy).apply(impression) + verifyNoInteractions(strategy) + } + + @Test + fun `pushImpression when it is decorated uses value from trackImpression to track`() { + val impression = mock(DecoratedImpression::class.java) + val impression2 = mock(DecoratedImpression::class.java) + `when`(impression.trackImpressions).thenReturn(false) + `when`(impression2.trackImpressions).thenReturn(true) + impressionManager.pushImpression(impression) + impressionManager.pushImpression(impression2) + + verify(strategy).apply(impression2) + verify(noneStrategy).apply(impression) + } + + @Test + fun `enableTracking set to true causes impressions to be sent to strategy`() { + impressionManager.enableTracking(true) + val impression = mock(DecoratedImpression::class.java) + `when`(impression.trackImpressions).thenReturn(true) + impressionManager.pushImpression(impression) + + verify(strategy).apply(impression) + verifyNoInteractions(noneStrategy) + } + + @Test + fun `enableTracking set to false causes impressions to not be tracked`() { + impressionManager.enableTracking(false) + val impression = mock(DecoratedImpression::class.java) + `when`(impression.trackImpressions).thenReturn(true) + impressionManager.pushImpression(impression) + + verifyNoInteractions(noneStrategy) + verifyNoInteractions(strategy) } }