Skip to content

Standardize support for Firebase products that integrate with Remote Config. #2222

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 7 commits into from
Jan 20, 2021
Merged

Standardize support for Firebase products that integrate with Remote Config. #2222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f037c14
More logging for personalization.
vic-flair Dec 2, 2020
ffb9786
Merge branch 'master' into vlum.p13n-2
vic-flair Jan 12, 2021
3fdff9f
Remove Converter.
vic-flair Jan 12, 2021
10daf06
Check cache for choice ID and add more asserts in tests.
vic-flair Jan 15, 2021
46fb82f
Rename constants.
vic-flair Jan 16, 2021
8268fe6
Use value that looks more like actual Personalization metadata.
vic-flair Jan 16, 2021
64f38af
Editing some comments.
vic-flair Jan 20, 2021
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
60 changes: 49 additions & 11 deletions firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,36 @@
import android.os.Bundle;
import androidx.annotation.NonNull;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import org.json.JSONObject;

public class Personalization {
public static final String ANALYTICS_ORIGIN_PERSONALIZATION = "fp";
public static final String ANALYTICS_PULL_EVENT = "_fpc";
public static final String ARM_KEY = "_fpid";
public static final String ARM_VALUE = "_fpct";
static final String PERSONALIZATION_ID = "personalizationId";

// The PARAM suffix identifies log keys sent to Google Analytics.
public static final String EXTERNAL_EVENT = "personalization_assignment";
public static final String EXTERNAL_RC_PARAMETER_PARAM = "arm_key";
public static final String EXTERNAL_ARM_VALUE_PARAM = "arm_value";
public static final String PERSONALIZATION_ID = "personalizationId";
public static final String EXTERNAL_PERSONALIZATION_ID_PARAM = "personalization_id";
public static final String ARM_INDEX = "armIndex";
public static final String EXTERNAL_ARM_INDEX_PARAM = "arm_index";
public static final String GROUP = "group";
public static final String EXTERNAL_GROUP_PARAM = "group";

public static final String INTERNAL_EVENT = "_fpc";
public static final String CHOICE_ID = "choiceId";
public static final String INTERNAL_CHOICE_ID_PARAM = "_fpid";

/** The app's Firebase Analytics client. */
private final AnalyticsConnector analyticsConnector;

/** Remote Config parameter key and choice ID pairs that have already been logged to Analytics. */
private final Map<String, String> loggedChoiceIds =
Collections.synchronizedMap(new HashMap<String, String>());

/** Creates an instance of {@code Personalization}. */
public Personalization(@NonNull AnalyticsConnector analyticsConnector) {
this.analyticsConnector = analyticsConnector;
Expand All @@ -38,11 +56,11 @@ public Personalization(@NonNull AnalyticsConnector analyticsConnector) {
* Called when a Personalization parameter value (an arm) is retrieved, and uses Google Analytics
* for Firebase to log metadata if it's a Personalization parameter.
*
* @param key Remote Config parameter
* @param rcParameter Remote Config parameter
* @param configContainer {@link ConfigContainer} containing Personalization metadata for {@code
* key}
*/
public void logArmActive(@NonNull String key, @NonNull ConfigContainer configContainer) {
public void logArmActive(@NonNull String rcParameter, @NonNull ConfigContainer configContainer) {
JSONObject ids = configContainer.getPersonalizationMetadata();
if (ids.length() < 1) {
return;
Expand All @@ -53,14 +71,34 @@ public void logArmActive(@NonNull String key, @NonNull ConfigContainer configCon
return;
}

JSONObject metadata = ids.optJSONObject(key);
JSONObject metadata = ids.optJSONObject(rcParameter);
if (metadata == null) {
return;
}

Bundle params = new Bundle();
params.putString(ARM_KEY, metadata.optString(PERSONALIZATION_ID));
params.putString(ARM_VALUE, values.optString(key));
analyticsConnector.logEvent(ANALYTICS_ORIGIN_PERSONALIZATION, ANALYTICS_PULL_EVENT, params);
String choiceId = metadata.optString(CHOICE_ID);
if (choiceId.isEmpty()) {
return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking: this won't break EAP usage because they aren't using the SDK

}

synchronized (loggedChoiceIds) {
if (choiceId.equals(loggedChoiceIds.get(rcParameter))) {
return;
}
loggedChoiceIds.put(rcParameter, choiceId);
}

Bundle logParams = new Bundle();
logParams.putString(EXTERNAL_RC_PARAMETER_PARAM, rcParameter);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Much easier to read 👍

logParams.putString(EXTERNAL_ARM_VALUE_PARAM, values.optString(rcParameter));
logParams.putString(EXTERNAL_PERSONALIZATION_ID_PARAM, metadata.optString(PERSONALIZATION_ID));
logParams.putInt(EXTERNAL_ARM_INDEX_PARAM, metadata.optInt(ARM_INDEX, -1));
logParams.putString(EXTERNAL_GROUP_PARAM, metadata.optString(GROUP));
analyticsConnector.logEvent(ANALYTICS_ORIGIN_PERSONALIZATION, EXTERNAL_EVENT, logParams);

Bundle internalLogParams = new Bundle();
internalLogParams.putString(INTERNAL_CHOICE_ID_PARAM, choiceId);
analyticsConnector.logEvent(
ANALYTICS_ORIGIN_PERSONALIZATION, INTERNAL_EVENT, internalLogParams);
}
}
12 changes: 6 additions & 6 deletions firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ public void personalization_hasMetadata_successful() throws Exception {
.when(mockAnalyticsConnector)
.logEvent(
eq(Personalization.ANALYTICS_ORIGIN_PERSONALIZATION),
eq(Personalization.ANALYTICS_PULL_EVENT),
eq(Personalization.EXTERNAL_EVENT),
any(Bundle.class));

ConfigContainer configContainer =
Expand Down Expand Up @@ -986,18 +986,18 @@ public void personalization_hasMetadata_successful() throws Exception {
verify(mockAnalyticsConnector, times(2))
.logEvent(
eq(Personalization.ANALYTICS_ORIGIN_PERSONALIZATION),
eq(Personalization.ANALYTICS_PULL_EVENT),
eq(Personalization.EXTERNAL_EVENT),
any(Bundle.class));
assertThat(fakeLogs).hasSize(2);

Bundle params1 = new Bundle();
params1.putString(Personalization.ARM_KEY, "id1");
params1.putString(Personalization.ARM_VALUE, "value1");
params1.putString(Personalization.EXTERNAL_RC_PARAMETER_PARAM, "id1");
params1.putString(Personalization.EXTERNAL_ARM_VALUE_PARAM, "value1");
assertThat(fakeLogs.get(0).toString()).isEqualTo(params1.toString());

Bundle params2 = new Bundle();
params2.putString(Personalization.ARM_KEY, "id2");
params2.putString(Personalization.ARM_VALUE, "value2");
params2.putString(Personalization.EXTERNAL_RC_PARAMETER_PARAM, "id2");
params2.putString(Personalization.EXTERNAL_ARM_VALUE_PARAM, "value2");
assertThat(fakeLogs.get(1).toString()).isEqualTo(params2.toString());
});
}
Expand Down
4 changes: 3 additions & 1 deletion ...nfig/src/test/java/com/google/firebase/remoteconfig/internal/ConfigStorageClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ public void read_validContainerWithPersonalization_returnsContainer() throws Exc
ConfigContainer configWithPersonalization =
ConfigContainer.newBuilder(configContainer)
.withPersonalizationMetadata(
new JSONObject(ImmutableMap.of(Personalization.ARM_KEY, "arm_value")))
new JSONObject(
"{long_param: {personalizationId: 'id1'}, "
+ "string_param: {personalizationId: 'id2'}}"))
.build();
storageClient.write(configWithPersonalization);
Preconditions.checkArgument(getFileAsString().equals(configWithPersonalization.toString()));
Expand Down
94 changes: 65 additions & 29 deletions ...e-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,25 @@

import static com.google.common.truth.Truth.assertThat;
import static com.google.firebase.remoteconfig.internal.Personalization.ANALYTICS_ORIGIN_PERSONALIZATION;
import static com.google.firebase.remoteconfig.internal.Personalization.ANALYTICS_PULL_EVENT;
import static com.google.firebase.remoteconfig.internal.Personalization.ARM_KEY;
import static com.google.firebase.remoteconfig.internal.Personalization.ARM_VALUE;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_ARM_INDEX_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_ARM_VALUE_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_EVENT;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_GROUP_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_PERSONALIZATION_ID_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.EXTERNAL_RC_PARAMETER_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.INTERNAL_CHOICE_ID_PARAM;
import static com.google.firebase.remoteconfig.internal.Personalization.INTERNAL_EVENT;
import static org.mockito.AdditionalMatchers.or;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.MockitoAnnotations.initMocks;

import android.os.Bundle;
import com.google.common.truth.Correspondence;
import com.google.firebase.analytics.connector.AnalyticsConnector;
import java.util.ArrayList;
import java.util.Date;
Expand All @@ -45,6 +53,10 @@
@Config(manifest = Config.NONE)
public class PersonalizationTest {
private static final ConfigContainer CONFIG_CONTAINER;
private static final Bundle LOG_PARAMS_1 = new Bundle();
private static final Bundle LOG_PARAMS_2 = new Bundle();
private static final Bundle INTERNAL_LOG_PARAMS_1 = new Bundle();
private static final Bundle INTERNAL_LOG_PARAMS_2 = new Bundle();

static {
try {
Expand All @@ -55,15 +67,37 @@ public class PersonalizationTest {
.withFetchTime(new Date(1))
.withPersonalizationMetadata(
new JSONObject(
"{key1: {personalizationId: 'id1'}, key2: {personalizationId: 'id2'}}"))
"{key1: {personalizationId: 'p13n1', armIndex: 0,"
+ " choiceId: 'id1', group: 'BASELINE'},"
+ " key2: {personalizationId: 'p13n2', armIndex: 1,"
+ " choiceId: 'id2', group: 'P13N'}}"))
.build();
} catch (JSONException e) {
throw new RuntimeException(e);
}

LOG_PARAMS_1.putString(EXTERNAL_RC_PARAMETER_PARAM, "key1");
LOG_PARAMS_1.putString(EXTERNAL_ARM_VALUE_PARAM, "value1");
LOG_PARAMS_1.putString(EXTERNAL_PERSONALIZATION_ID_PARAM, "p13n1");
LOG_PARAMS_1.putInt(EXTERNAL_ARM_INDEX_PARAM, 0);
LOG_PARAMS_1.putString(EXTERNAL_GROUP_PARAM, "BASELINE");

LOG_PARAMS_2.putString(EXTERNAL_RC_PARAMETER_PARAM, "key2");
LOG_PARAMS_2.putString(EXTERNAL_ARM_VALUE_PARAM, "value2");
LOG_PARAMS_2.putString(EXTERNAL_PERSONALIZATION_ID_PARAM, "p13n2");
LOG_PARAMS_2.putInt(EXTERNAL_ARM_INDEX_PARAM, 1);
LOG_PARAMS_2.putString(EXTERNAL_GROUP_PARAM, "P13N");

INTERNAL_LOG_PARAMS_1.putString(INTERNAL_CHOICE_ID_PARAM, "id1");

INTERNAL_LOG_PARAMS_2.putString(INTERNAL_CHOICE_ID_PARAM, "id2");
}

private static final List<Bundle> FAKE_LOGS = new ArrayList<>();

private static final Correspondence<Bundle, String> TO_STRING =
Correspondence.transforming(Bundle::toString, "as String is");

private Personalization personalization;

@Mock private AnalyticsConnector mockAnalyticsConnector;
Expand All @@ -74,8 +108,7 @@ public void setUp() {

doAnswer(invocation -> FAKE_LOGS.add(invocation.getArgument(2)))
.when(mockAnalyticsConnector)
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
.logEvent(eq(ANALYTICS_ORIGIN_PERSONALIZATION), anyString(), any(Bundle.class));

personalization = new Personalization(mockAnalyticsConnector);

Expand All @@ -88,43 +121,46 @@ public void logArmActive_nonPersonalizationKey_notLogged() {

verify(mockAnalyticsConnector, times(0))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
eq(ANALYTICS_ORIGIN_PERSONALIZATION),
or(eq(EXTERNAL_EVENT), eq(INTERNAL_EVENT)),
any(Bundle.class));
assertThat(FAKE_LOGS).isEmpty();
}

@Test
public void logArmActive_singlePersonalizationKey_loggedOnce() {
public void logArmActive_singlePersonalizationKey_loggedInternallyAndExternally() {
personalization.logArmActive("key1", CONFIG_CONTAINER);

verify(mockAnalyticsConnector, times(1))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(1);
.logEvent(eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(EXTERNAL_EVENT), any(Bundle.class));
verify(mockAnalyticsConnector, times(1))
.logEvent(eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(INTERNAL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(2);

Bundle params = new Bundle();
params.putString(ARM_KEY, "id1");
params.putString(ARM_VALUE, "value1");
assertThat(FAKE_LOGS.get(0).toString()).isEqualTo(params.toString());
assertThat(FAKE_LOGS)
.comparingElementsUsing(TO_STRING)
.containsExactly(LOG_PARAMS_1.toString(), INTERNAL_LOG_PARAMS_1.toString());
}

@Test
public void logArmActive_multiplePersonalizationKeys_loggedMultiple() {
public void logArmActive_multiplePersonalizationKeys_loggedInternallyAndExternally() {
personalization.logArmActive("key1", CONFIG_CONTAINER);
personalization.logArmActive("key2", CONFIG_CONTAINER);
personalization.logArmActive("key1", CONFIG_CONTAINER);

verify(mockAnalyticsConnector, times(2))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(2);

Bundle params1 = new Bundle();
params1.putString(ARM_KEY, "id1");
params1.putString(ARM_VALUE, "value1");
assertThat(FAKE_LOGS.get(0).toString()).isEqualTo(params1.toString());

Bundle params2 = new Bundle();
params2.putString(ARM_KEY, "id2");
params2.putString(ARM_VALUE, "value2");
assertThat(FAKE_LOGS.get(1).toString()).isEqualTo(params2.toString());
.logEvent(eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(EXTERNAL_EVENT), any(Bundle.class));
verify(mockAnalyticsConnector, times(2))
.logEvent(eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(INTERNAL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(4);

assertThat(FAKE_LOGS)
.comparingElementsUsing(TO_STRING)
.containsAtLeast(LOG_PARAMS_1.toString(), LOG_PARAMS_2.toString())
.inOrder();
assertThat(FAKE_LOGS)
.comparingElementsUsing(TO_STRING)
.containsAtLeast(INTERNAL_LOG_PARAMS_1.toString(), INTERNAL_LOG_PARAMS_2.toString())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could be combined with the above into one statement with all four logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't combine it if we want to check the ordering too, since it's not guaranteed that the external ones come before the internal ones.

.inOrder();
}
}