Skip to content

Report updated experiments if either experiment id or variant id changes. #4555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 14, 2023
Merged

Report updated experiments if either experiment id or variant id changes. #4555

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab049cf
Consider experiments updated if either experiment id or variant id ch…
danasilver Jan 12, 2023
16f2c94
Add changelog entry.
danasilver Jan 12, 2023
3e7b35c
Add comments to indicate which experiments are from Analytics.
danasilver Jan 12, 2023
9385b2a
Merge branch 'master' into ds-abt-variant-id
danasilver Jan 12, 2023
c644b7d
Format java files.
danasilver Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions firebase-abt/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Unreleased

* [changed] Internal changes to improve experiment reporting.

# 21.1.0
* [changed] Internal changes to ensure functionality alignment with other
SDK releases.
Expand Down
47 changes: 24 additions & 23 deletions firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,8 @@ public void reportActiveExperiment(AbtExperimentInfo activeExperiment) throws Ab
public void validateRunningExperiments(List<AbtExperimentInfo> runningExperiments)
throws AbtException {
throwAbtExceptionIfAnalyticsIsNull();
Set<String> runningExperimentIds = new HashSet<>();
for (AbtExperimentInfo runningExperiment : runningExperiments) {
runningExperimentIds.add(runningExperiment.getExperimentId());
}
List<ConditionalUserProperty> experimentsToRemove =
getExperimentsToRemove(getAllExperimentsInAnalytics(), runningExperimentIds);
getExperimentsToRemove(getAllExperiments(), runningExperiments);
removeExperiments(experimentsToRemove);
}

Expand Down Expand Up @@ -245,34 +241,25 @@ private void replaceAllExperimentsWith(List<AbtExperimentInfo> replacementExperi
return;
}

Set<String> replacementExperimentIds = new HashSet<>();
for (AbtExperimentInfo replacementExperiment : replacementExperiments) {
replacementExperimentIds.add(replacementExperiment.getExperimentId());
}

List<ConditionalUserProperty> experimentsInAnalytics = getAllExperimentsInAnalytics();
Set<String> idsOfExperimentsInAnalytics = new HashSet<>();
for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) {
idsOfExperimentsInAnalytics.add(experimentInAnalytics.name);
}
List<AbtExperimentInfo> experimentsInAnalytics = getAllExperiments();

List<ConditionalUserProperty> experimentsToRemove =
getExperimentsToRemove(experimentsInAnalytics, replacementExperimentIds);
getExperimentsToRemove(experimentsInAnalytics, replacementExperiments);
removeExperiments(experimentsToRemove);

List<AbtExperimentInfo> experimentsToAdd =
getExperimentsToAdd(replacementExperiments, idsOfExperimentsInAnalytics);
getExperimentsToAdd(replacementExperiments, experimentsInAnalytics);
addExperiments(experimentsToAdd);
}

/** Returns this origin's experiments in Analytics that are no longer assigned to this App. */
private ArrayList<ConditionalUserProperty> getExperimentsToRemove(
List<ConditionalUserProperty> experimentsInAnalytics, Set<String> replacementExperimentIds) {
List<AbtExperimentInfo> experimentsInAnalytics, List<AbtExperimentInfo> replacementExperiments) {

ArrayList<ConditionalUserProperty> experimentsToRemove = new ArrayList<>();
for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) {
if (!replacementExperimentIds.contains(experimentInAnalytics.name)) {
experimentsToRemove.add(experimentInAnalytics);
for (AbtExperimentInfo experimentInAnalytics : experimentsInAnalytics) {
if (!experimentsListContainsExperiment(replacementExperiments, experimentInAnalytics)) {
experimentsToRemove.add(experimentInAnalytics.toConditionalUserProperty(originService));
}
}
return experimentsToRemove;
Expand All @@ -283,17 +270,31 @@ private ArrayList<ConditionalUserProperty> getExperimentsToRemove(
* to this origin's list of experiments in Analytics.
*/
private ArrayList<AbtExperimentInfo> getExperimentsToAdd(
List<AbtExperimentInfo> replacementExperiments, Set<String> idsOfExperimentsInAnalytics) {
List<AbtExperimentInfo> replacementExperiments, List<AbtExperimentInfo> experimentInfoFromAnalytics) {

ArrayList<AbtExperimentInfo> experimentsToAdd = new ArrayList<>();
for (AbtExperimentInfo replacementExperiment : replacementExperiments) {
if (!idsOfExperimentsInAnalytics.contains(replacementExperiment.getExperimentId())) {
if (!experimentsListContainsExperiment(experimentInfoFromAnalytics, replacementExperiment)) {
experimentsToAdd.add(replacementExperiment);
}
}
return experimentsToAdd;
}

private boolean experimentsListContainsExperiment(
List<AbtExperimentInfo> experiments, AbtExperimentInfo experiment) {
String experimentId = experiment.getExperimentId();
String variantId = experiment.getVariantId();

for (AbtExperimentInfo experimentInfo : experiments) {
if (experimentInfo.getExperimentId().equals(experimentId) && experimentInfo.getVariantId().equals(variantId)) {
return true;
}
}

return false;
}

/** Adds the given experiments to the origin's list in Analytics. */
private void addExperiments(List<AbtExperimentInfo> experimentsToAdd) {

Expand Down
85 changes: 66 additions & 19 deletions firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,21 @@ public class FirebaseABTestingTest {
private static final String TEST_EXPERIMENT_1_ID = "1";
private static final String TEST_EXPERIMENT_2_ID = "2";

private static final String TEST_VARIANT_ID_A = "VARIANT_A";
private static final String TEST_VARIANT_ID_B = "VARIANT_B";

private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_1 =
createExperimentInfo(
TEST_EXPERIMENT_1_ID,
TEST_VARIANT_ID_A,
/*triggerEventName=*/ "",
/*experimentStartTimeInEpochMillis=*/ 1000L);
private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2 =
private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2_VARIANT_A =
createExperimentInfo(
TEST_EXPERIMENT_2_ID, "trigger_event_2", /*experimentStartTimeInEpochMillis=*/ 2000L);
TEST_EXPERIMENT_2_ID, TEST_VARIANT_ID_A, "trigger_event_2", /*experimentStartTimeInEpochMillis=*/ 2000L);
private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2_VARIANT_B =
createExperimentInfo(
TEST_EXPERIMENT_2_ID, TEST_VARIANT_ID_B, "trigger_event_2", /*experimentStartTimeInEpochMillis=*/ 2000L);

private static final int MAX_ALLOWED_EXPERIMENTS_IN_ANALYTICS = 100;

Expand Down Expand Up @@ -91,7 +98,7 @@ public void replaceAllExperiments_noExperimentsInAnalytics_experimentsCorrectlyS

firebaseAbt.replaceAllExperiments(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toStringMap(), TEST_ABT_EXPERIMENT_2.toStringMap()));
TEST_ABT_EXPERIMENT_1.toStringMap(), TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap()));

ArgumentCaptor<ConditionalUserProperty> analyticsExperimentArgumentCaptor =
ArgumentCaptor.forClass(ConditionalUserProperty.class);
Expand All @@ -107,7 +114,7 @@ public void replaceAllExperiments_noExperimentsInAnalytics_experimentsCorrectlyS

// Validates that TEST_ABT_EXPERIMENT_1 and TEST_ABT_EXPERIMENT_2 have been set in Analytics.
assertThat(analyticsExperiment1.toStringMap()).isEqualTo(TEST_ABT_EXPERIMENT_1.toStringMap());
assertThat(analyticsExperiment2.toStringMap()).isEqualTo(TEST_ABT_EXPERIMENT_2.toStringMap());
assertThat(analyticsExperiment2.toStringMap()).isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap());
}

@Test
Expand All @@ -117,10 +124,10 @@ public void replaceAllExperiments_existExperimentsInAnalytics_experimentsCorrect
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "", 1000L);
AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "trigger_event_4", 1000L);
AbtExperimentInfo newExperiment3 = createExperimentInfo("3", TEST_VARIANT_ID_A, "", 1000L);
AbtExperimentInfo newExperiment4 = createExperimentInfo("4", TEST_VARIANT_ID_A, "trigger_event_4", 1000L);

// Simulates the case where experiment 1 is assigned (as before), experiment 2 is no longer
// assigned; experiment 3 and experiment 4 are newly assigned.
Expand All @@ -146,6 +153,46 @@ public void replaceAllExperiments_existExperimentsInAnalytics_experimentsCorrect
.isEqualTo(newExperiment4.toStringMap());
}

@Test
public void replaceAllExperiments_existExperimentsInAnalyticsWithDifferentVariants_experimentsCorrectlySetInAnalytics()
throws Exception {
when(mockAnalyticsConnector.getConditionalUserProperties(ORIGIN_SERVICE, ""))
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "b", "", 1000L);
AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "a", "trigger_event_4", 1000L);

// Simulates the case where experiments 1 and 2 are removed,
// experiment 2 is re-set with a new variant, and experiments 3 and 4 are newly added.
firebaseAbt.replaceAllExperiments(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_2_VARIANT_B.toStringMap(),
newExperiment3.toStringMap(),
newExperiment4.toStringMap()));

// Validates that experiment 1 is cleared, experiment 2 is updated,
// and experiment 3 and experiment 4 are set in Analytics.
ArgumentCaptor<ConditionalUserProperty> analyticsExperimentArgumentCaptor =
ArgumentCaptor.forClass(ConditionalUserProperty.class);
verify(mockAnalyticsConnector, times(1))
.clearConditionalUserProperty(TEST_EXPERIMENT_1_ID, null, null);
verify(mockAnalyticsConnector, times(1))
.clearConditionalUserProperty(TEST_EXPERIMENT_2_ID, null, null);
verify(mockAnalyticsConnector, times(3))
.setConditionalUserProperty(analyticsExperimentArgumentCaptor.capture());

List<ConditionalUserProperty> actualValues = analyticsExperimentArgumentCaptor.getAllValues();
assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(0)).toStringMap())
.isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_B.toStringMap());
assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(1)).toStringMap())
.isEqualTo(newExperiment3.toStringMap());
assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(2)).toStringMap())
.isEqualTo(newExperiment4.toStringMap());
}

@Test
public void replaceAllExperiments_totalExperimentsExceedsAnalyticsLimit_oldExperimentsDiscarded()
throws Exception {
Expand All @@ -155,17 +202,17 @@ public void replaceAllExperiments_totalExperimentsExceedsAnalyticsLimit_oldExper
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "", 1000L);
AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "trigger_event_4", 1000L);
AbtExperimentInfo newExperiment3 = createExperimentInfo("3", TEST_VARIANT_ID_A, "", 1000L);
AbtExperimentInfo newExperiment4 = createExperimentInfo("4", TEST_VARIANT_ID_A, "trigger_event_4", 1000L);

// Simulates the case where experiment 1 and 2 are assigned (as before), experiment 3 and
// experiment 4 are newly assigned.
firebaseAbt.replaceAllExperiments(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toStringMap(),
TEST_ABT_EXPERIMENT_2.toStringMap(),
TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap(),
newExperiment3.toStringMap(),
newExperiment4.toStringMap()));

Expand Down Expand Up @@ -231,7 +278,7 @@ public void removeAllExperiments_existExperimentsInAnalytics_experimentsClearedF
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

firebaseAbt.removeAllExperiments();

Expand Down Expand Up @@ -266,15 +313,15 @@ public void getAllExperiments_existExperimentsInAnalytics_returnAllExperiments()
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

List<AbtExperimentInfo> abtExperimentInfoList = firebaseAbt.getAllExperiments();

assertThat(abtExperimentInfoList).hasSize(2);
assertThat(abtExperimentInfoList.get(0).toStringMap())
.isEqualTo(TEST_ABT_EXPERIMENT_1.toStringMap());
assertThat(abtExperimentInfoList.get(1).toStringMap())
.isEqualTo(TEST_ABT_EXPERIMENT_2.toStringMap());
.isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap());
}

@Test
Expand All @@ -298,7 +345,7 @@ public void getAllExperiments_analyticsSdkUnavailable_throwsAbtException() {
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

// Update to just one experiment running
firebaseAbt.validateRunningExperiments(Lists.newArrayList(TEST_ABT_EXPERIMENT_1));
Expand All @@ -315,11 +362,11 @@ public void validateRunningExperiments_noinactiveExperimentsInAnalytics_cleansUp
.thenReturn(
Lists.newArrayList(
TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));

// Update still says the same two experiments are running
firebaseAbt.validateRunningExperiments(
Lists.newArrayList(TEST_ABT_EXPERIMENT_1, TEST_ABT_EXPERIMENT_2));
Lists.newArrayList(TEST_ABT_EXPERIMENT_1, TEST_ABT_EXPERIMENT_2_VARIANT_A));

// Verify nothing cleared
verify(mockAnalyticsConnector, never()).clearConditionalUserProperty(any(), any(), any());
Expand All @@ -346,11 +393,11 @@ public void reportActiveExperiment_setsNullTriggerCondition() throws Exception {
}

private static AbtExperimentInfo createExperimentInfo(
String experimentId, String triggerEventName, long experimentStartTimeInEpochMillis) {
String experimentId, String variantId, String triggerEventName, long experimentStartTimeInEpochMillis) {

return new AbtExperimentInfo(
experimentId,
VARIANT_ID_VALUE,
variantId,
triggerEventName,
new Date(experimentStartTimeInEpochMillis),
TRIGGER_TIMEOUT_IN_MILLIS_VALUE,
Expand Down