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 1 commit
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
1 change: 1 addition & 0 deletions firebase-config/firebase-config.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ dependencies {

implementation 'com.google.firebase:firebase-measurement-connector:18.0.0'
implementation 'com.google.android.gms:play-services-tasks:17.0.2'
implementation 'com.google.guava:guava:28.1-android'

compileOnly 'com.google.code.findbugs:jsr305:3.0.2'

Expand Down
49 changes: 44 additions & 5 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 @@ -16,22 +16,41 @@

import android.os.Bundle;
import androidx.annotation.NonNull;
import com.google.common.base.CaseFormat;
import com.google.common.base.Converter;
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";

public static final String ANALYTICS_PULL_EVENT = "personalization_choice";
public static final String ARM_KEY = "arm_key";
public static final String ARM_VALUE = "arm_value";
public static final String PERSONALIZATION_ID = "personalization_id";
public static final String ARM_INDEX = "arm_index";
public static final String GROUP = "group";

public static final String ANALYTICS_PULL_EVENT_INTERNAL = "_fpc";
Copy link

@erikeldridge erikeldridge Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking (no action req'd): "_fpc" for the internal event name, and "_fpid" for the parameter name (in which to log the choice ID), aligns w the per-choice metadata design.

public static final String CHOICE_ID = "choiceId";
public static final String CHOICE_ID_KEY = "_fpid";

private static final Converter<String, String> CONVERTER =
CaseFormat.LOWER_UNDERSCORE.converterTo(CaseFormat.LOWER_CAMEL);

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

/** A map of Remote Config parameter key to Personalization ID. */
private final Map<String, String> armsCache;

/** Creates an instance of {@code Personalization}. */
public Personalization(@NonNull AnalyticsConnector analyticsConnector) {
this.analyticsConnector = analyticsConnector;
armsCache = Collections.synchronizedMap(new HashMap<String, String>());
}

/**
Expand All @@ -58,9 +77,29 @@ public void logArmActive(@NonNull String key, @NonNull ConfigContainer configCon
return;
}

String personalizationId = metadata.optString(CONVERTER.convert(PERSONALIZATION_ID));
if (personalizationId.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 (armsCache) {
Copy link

Choose a reason for hiding this comment

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

Mind adding a comment explaining why we need this?

Thinking: this ensures we only log one event per param-choice pair. This map is bounded by the number of params. Choices are made on each fetch, so this isn't a long-lived cache.

if (armsCache.get(key) == personalizationId) {
return;
}
armsCache.put(key, personalizationId);
}

Bundle params = new Bundle();
params.putString(ARM_KEY, metadata.optString(PERSONALIZATION_ID));
params.putString(ARM_KEY, key);
params.putString(ARM_VALUE, values.optString(key));
params.putString(PERSONALIZATION_ID, personalizationId);
params.putInt(ARM_INDEX, metadata.optInt(CONVERTER.convert(ARM_INDEX), -1));
params.putString(GROUP, metadata.optString(CONVERTER.convert(GROUP)));
analyticsConnector.logEvent(ANALYTICS_ORIGIN_PERSONALIZATION, ANALYTICS_PULL_EVENT, params);

Bundle paramsInternal = new Bundle();
paramsInternal.putString(CHOICE_ID_KEY, metadata.optString(CHOICE_ID));
analyticsConnector.logEvent(
ANALYTICS_ORIGIN_PERSONALIZATION, ANALYTICS_PULL_EVENT_INTERNAL, paramsInternal);
}
}
52 changes: 33 additions & 19 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 @@ -17,16 +17,18 @@
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.ANALYTICS_PULL_EVENT_INTERNAL;
import static com.google.firebase.remoteconfig.internal.Personalization.CHOICE_ID_KEY;
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 Down Expand Up @@ -55,7 +57,10 @@ 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);
Expand All @@ -64,6 +69,9 @@ public class PersonalizationTest {

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 +82,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 @@ -87,8 +94,7 @@ public void logArmActive_nonPersonalizationKey_notLogged() {
personalization.logArmActive("key3", CONFIG_CONTAINER);

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

Expand All @@ -99,12 +105,16 @@ public void logArmActive_singlePersonalizationKey_loggedOnce() {
verify(mockAnalyticsConnector, times(1))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(1);
verify(mockAnalyticsConnector, times(1))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION),
eq(ANALYTICS_PULL_EVENT_INTERNAL),
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());
params.putString(CHOICE_ID_KEY, "id1");
assertThat(FAKE_LOGS).comparingElementsUsing(TO_STRING).contains(params.toString());
}

@Test
Expand All @@ -115,16 +125,20 @@ public void logArmActive_multiplePersonalizationKeys_loggedMultiple() {
verify(mockAnalyticsConnector, times(2))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION), eq(ANALYTICS_PULL_EVENT), any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(2);
verify(mockAnalyticsConnector, times(2))
.logEvent(
eq(ANALYTICS_ORIGIN_PERSONALIZATION),
eq(ANALYTICS_PULL_EVENT_INTERNAL),
any(Bundle.class));
assertThat(FAKE_LOGS).hasSize(4);

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

params1.putString(CHOICE_ID_KEY, "id1");
Bundle params2 = new Bundle();
params2.putString(ARM_KEY, "id2");
params2.putString(ARM_VALUE, "value2");
assertThat(FAKE_LOGS.get(1).toString()).isEqualTo(params2.toString());
params2.putString(CHOICE_ID_KEY, "id2");
assertThat(FAKE_LOGS)
.comparingElementsUsing(TO_STRING)
.containsAtLeast(params1.toString(), params2.toString())
.inOrder();
}
}