diff --git a/firebase-abt/CHANGELOG.md b/firebase-abt/CHANGELOG.md index 931ae65ff4d..d54afdaa8e6 100644 --- a/firebase-abt/CHANGELOG.md +++ b/firebase-abt/CHANGELOG.md @@ -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. diff --git a/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java b/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java index 15ce789b78d..d3f89e99bbf 100644 --- a/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java +++ b/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java @@ -30,10 +30,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Deque; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; /** * Manages Firebase A/B Testing Experiments. @@ -144,11 +142,11 @@ public void removeAllExperiments() throws AbtException { } /** - * Gets the origin service's list of experiments in the app. + * Gets the origin service's list of experiments in the app via the Analytics SDK. * *

Note: This is a blocking call and therefore should be called from a worker thread. * - * @return the origin service's list of experiments in the app. + * @return the origin service's list of experiments in the app as {@link AbtExperimentInfo}s. * @throws AbtException If there is no Analytics SDK. */ @WorkerThread @@ -204,12 +202,10 @@ public void reportActiveExperiment(AbtExperimentInfo activeExperiment) throws Ab public void validateRunningExperiments(List runningExperiments) throws AbtException { throwAbtExceptionIfAnalyticsIsNull(); - Set runningExperimentIds = new HashSet<>(); - for (AbtExperimentInfo runningExperiment : runningExperiments) { - runningExperimentIds.add(runningExperiment.getExperimentId()); - } + + // Get all experiments in Analytics and remove the ones that aren't running. List experimentsToRemove = - getExperimentsToRemove(getAllExperimentsInAnalytics(), runningExperimentIds); + getExperimentsToRemove(getAllExperiments(), runningExperiments); removeExperiments(experimentsToRemove); } @@ -245,34 +241,29 @@ private void replaceAllExperimentsWith(List replacementExperi return; } - Set replacementExperimentIds = new HashSet<>(); - for (AbtExperimentInfo replacementExperiment : replacementExperiments) { - replacementExperimentIds.add(replacementExperiment.getExperimentId()); - } - - List experimentsInAnalytics = getAllExperimentsInAnalytics(); - Set idsOfExperimentsInAnalytics = new HashSet<>(); - for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) { - idsOfExperimentsInAnalytics.add(experimentInAnalytics.name); - } + // Get all experiments in Analytics. + List experimentsInAnalytics = getAllExperiments(); + // Remove experiments no longer assigned. List experimentsToRemove = - getExperimentsToRemove(experimentsInAnalytics, replacementExperimentIds); + getExperimentsToRemove(experimentsInAnalytics, replacementExperiments); removeExperiments(experimentsToRemove); + // Add newly assigned or updated (changed variant id). List 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 getExperimentsToRemove( - List experimentsInAnalytics, Set replacementExperimentIds) { + List experimentsInAnalytics, + List replacementExperiments) { ArrayList 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; @@ -283,17 +274,33 @@ private ArrayList getExperimentsToRemove( * to this origin's list of experiments in Analytics. */ private ArrayList getExperimentsToAdd( - List replacementExperiments, Set idsOfExperimentsInAnalytics) { + List replacementExperiments, + List experimentInfoFromAnalytics) { ArrayList 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 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 experimentsToAdd) { @@ -369,7 +376,7 @@ private int getMaxUserPropertiesInAnalytics() { * Returns a list of all this origin's experiments in this App's Analytics SDK. * *

The list is sorted chronologically by the experiment start time, with the oldest experiment - * at index 0. + * at index 0. Experiments are stored as {@link ConditionalUserProperty}s in Analytics. */ @WorkerThread private List getAllExperimentsInAnalytics() { diff --git a/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java b/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java index a98c05f317f..c8e2c6d8fdd 100644 --- a/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java +++ b/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java @@ -54,14 +54,27 @@ 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, + TEST_VARIANT_ID_A, + "trigger_event_2", + /*experimentStartTimeInEpochMillis=*/ 2000L); + private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2_VARIANT_B = createExperimentInfo( - TEST_EXPERIMENT_2_ID, "trigger_event_2", /*experimentStartTimeInEpochMillis=*/ 2000L); + TEST_EXPERIMENT_2_ID, + TEST_VARIANT_ID_B, + "trigger_event_2", + /*experimentStartTimeInEpochMillis=*/ 2000L); private static final int MAX_ALLOWED_EXPERIMENTS_IN_ANALYTICS = 100; @@ -91,7 +104,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 analyticsExperimentArgumentCaptor = ArgumentCaptor.forClass(ConditionalUserProperty.class); @@ -107,7 +120,8 @@ 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 @@ -117,10 +131,11 @@ 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. @@ -146,6 +161,47 @@ 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 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 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 { @@ -155,17 +211,18 @@ 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())); @@ -231,7 +288,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(); @@ -266,7 +323,7 @@ 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 abtExperimentInfoList = firebaseAbt.getAllExperiments(); @@ -274,7 +331,7 @@ public void getAllExperiments_existExperimentsInAnalytics_returnAllExperiments() 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 @@ -298,7 +355,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)); @@ -315,11 +372,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()); @@ -346,11 +403,14 @@ 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, diff --git a/firebase-firestore/src/proto/google/firestore/v1/bloom_filter.proto b/firebase-firestore/src/proto/google/firestore/v1/bloom_filter.proto new file mode 100644 index 00000000000..9487e3036fe --- /dev/null +++ b/firebase-firestore/src/proto/google/firestore/v1/bloom_filter.proto @@ -0,0 +1,73 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +syntax = "proto3"; + +package google.firestore.v1; + +option csharp_namespace = "Google.Cloud.Firestore.V1"; +option go_package = "google.golang.org/genproto/googleapis/firestore/v1;firestore"; +option java_multiple_files = true; +option java_outer_classname = "BloomFilterProto"; +option java_package = "com.google.firestore.v1"; +option objc_class_prefix = "GCFS"; +option php_namespace = "Google\\Cloud\\Firestore\\V1"; +option ruby_package = "Google::Cloud::Firestore::V1"; + +// A sequence of bits, encoded in a byte array. +// +// Each byte in the `bitmap` byte array stores 8 bits of the sequence. The only +// exception is the last byte, which may store 8 _or fewer_ bits. The `padding` +// defines the number of bits of the last byte to be ignored as "padding". The +// values of these "padding" bits are unspecified and must be ignored. +// +// To retrieve the first bit, bit 0, calculate: (bitmap[0] & 0x01) != 0. +// To retrieve the second bit, bit 1, calculate: (bitmap[0] & 0x02) != 0. +// To retrieve the third bit, bit 2, calculate: (bitmap[0] & 0x04) != 0. +// To retrieve the fourth bit, bit 3, calculate: (bitmap[0] & 0x08) != 0. +// To retrieve bit n, calculate: (bitmap[n / 8] & (0x01 << (n % 8))) != 0. +// +// The "size" of a `BitSequence` (the number of bits it contains) is calculated +// by this formula: (bitmap.length * 8) - padding. +message BitSequence { + // The bytes that encode the bit sequence. + // May have a length of zero. + bytes bitmap = 1; + + // The number of bits of the last byte in `bitmap` to ignore as "padding". + // If the length of `bitmap` is zero, then this value must be 0. + // Otherwise, this value must be between 0 and 7, inclusive. + int32 padding = 2; +} + +// A bloom filter (https://en.wikipedia.org/wiki/Bloom_filter). +// +// The bloom filter hashes the entries with MD5 and treats the resulting 128-bit +// hash as 2 distinct 64-bit hash values, interpreted as unsigned integers +// using 2's complement encoding. +// +// These two hash values, named h1 and h2, are then used to compute the +// `hash_count` hash values using the formula, starting at i=0: +// +// h(i) = h1 + (i * h2) +// +// These resulting values are then taken modulo the number of bits in the bloom +// filter to get the bits of the bloom filter to test for the given entry. +message BloomFilter { + // The bloom filter data. + BitSequence bits = 1; + + // The number of hashes used by the algorithm. + int32 hash_count = 2; +} \ No newline at end of file diff --git a/firebase-firestore/src/proto/google/firestore/v1/firestore.proto b/firebase-firestore/src/proto/google/firestore/v1/firestore.proto index dda4721596c..1bf75ea3c15 100644 --- a/firebase-firestore/src/proto/google/firestore/v1/firestore.proto +++ b/firebase-firestore/src/proto/google/firestore/v1/firestore.proto @@ -25,6 +25,7 @@ import "google/firestore/v1/query.proto"; import "google/firestore/v1/write.proto"; import "google/protobuf/empty.proto"; import "google/protobuf/timestamp.proto"; +import "google/protobuf/wrappers.proto"; import "google/rpc/status.proto"; option csharp_namespace = "Google.Cloud.Firestore.V1"; @@ -746,6 +747,15 @@ message Target { // If the target should be removed once it is current and consistent. bool once = 6; + + // The number of documents that last matched the query at the resume token or + // read time. + // + // This value is only relevant when a `resume_type` is provided. This value + // being present and greater than zero signals that the client wants + // `ExistenceFilter.unchanged_names` to be included in the response. + // + google.protobuf.Int32Value expected_count = 12; } // Targets being watched have changed. diff --git a/firebase-firestore/src/proto/google/firestore/v1/write.proto b/firebase-firestore/src/proto/google/firestore/v1/write.proto index 59b2dafe4fd..49acae6ebf6 100644 --- a/firebase-firestore/src/proto/google/firestore/v1/write.proto +++ b/firebase-firestore/src/proto/google/firestore/v1/write.proto @@ -18,6 +18,7 @@ syntax = "proto3"; package google.firestore.v1; import "google/api/annotations.proto"; +import "google/firestore/v1/bloom_filter.proto"; import "google/firestore/v1/common.proto"; import "google/firestore/v1/document.proto"; import "google/protobuf/timestamp.proto"; @@ -261,4 +262,18 @@ message ExistenceFilter { // If different from the count of documents in the client that match, the // client must manually determine which documents no longer match the target. int32 count = 2; + + // A bloom filter that contains the UTF-8 byte encodings of the resource names + // of the documents that match [target_id][google.firestore.v1.ExistenceFilter.target_id], in the + // form `projects/{project_id}/databases/{database_id}/documents/{document_path}` + // that have NOT changed since the query results indicated by the resume token + // or timestamp given in `Target.resume_type`. + // + // This bloom filter may be omitted at the server's discretion, such as if it + // is deemed that the client will not make use of it or if it is too + // computationally expensive to calculate or transmit. Clients must gracefully + // handle this field being absent by falling back to the logic used before + // this field existed; that is, re-add the target without a resume token to + // figure out which documents in the client's cache are out of sync. + BloomFilter unchanged_names = 3; }