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

vic-flair · 2020-12-02T19:25:37Z

No description provided.

google-oss-bot · 2020-12-02T19:32:20Z

Coverage Report

Affected SDKs

firebase-config

SDK overall coverage changed from 88.50% (2c088d7) to 88.58% (e28bccdd) by +0.08%.

Filename Base (2c088d7) Head (e28bccdd) Diff

Personalization.java 88.24% 90.91% +2.67%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (e28bccdd) is created by Prow via merging commits: 2c088d7 64f38af.

google-oss-bot · 2020-12-02T19:37:56Z

Binary Size Report

Affected SDKs

firebase-config

Type Base (2c088d7) Head (e28bccdd) Diff

aar 70.8 kB 71.5 kB +654 B (+0.9%)

apk (aggressive) 91.1 kB 91.1 kB +16 B (+0.0%)

apk (release) 718 kB 719 kB +292 B (+0.0%)

Test Logs

Notes

Head commit (e28bccdd) is created by Prow via merging commits: 2c088d7 64f38af.

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java

danasilver

Some questions and comments about usage. We should make sure we test the upgrade path between SDK versions, too.

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java

danasilver

A couple naming and formatting followups.

erikeldridge

Seems technically correct to me 👍 I dropped a couple comments re event field naming to improve readability, but since they concern naming, which can be contentious, I flagged them as minor. To avoid miscommunication, I'll defer formal approval to @danasilver, since he's the Android lead.

erikeldridge · 2021-01-15T19:48:45Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+  public static final String ARM_INDEX_LOG_KEY = "arm_index";
+  public static final String GROUP = "group";
+
+  public static final String ANALYTICS_PULL_EVENT_INTERNAL = "_fpc";


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.

erikeldridge · 2021-01-15T20:01:03Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

-    analyticsConnector.logEvent(ANALYTICS_ORIGIN_PERSONALIZATION, ANALYTICS_PULL_EVENT, params);
+    String choiceId = metadata.optString(CHOICE_ID);
+    if (choiceId.isEmpty()) {
+      return;


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

erikeldridge · 2021-01-15T20:08:42Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+      return;
+    }
+
+    synchronized (armsCache) {


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.

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

erikeldridge · 2021-01-15T20:28:45Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+    }
+
+    Bundle logParams = new Bundle();
+    logParams.putString(ARM_KEY, key);


Thinking: the design lists five things we want to log:

Developer friendly identifier of the personalization (either the rc param, or an id customers can map back to rc param), satisfied by the arm_key field

The choice that was made (a string value), satisfied by the arm_value field

The group the user was in (“p13n” or “baseline”), satisfied by the group field

Personalization ID, satisfied by the personalization_id field

Arm index, satisfied by the arm_index field

Thinking: the external event name is arbitrary, we just need to document what it is. personalization_assignment seems self-descriptive to me 👍

Minor: renaming arm_key to something like rc_parameter would be more self-descriptive given it communicates an RC parameter.

erikeldridge · 2021-01-15T20:31:51Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+
+  // Constants with LOG_KEY suffix are how the corresponding ones without it are identified on
+  // Google Analytics.
+  public static final String ANALYTICS_PULL_EVENT = "personalization_assignment";


Minor: as I commented on the iOS change, it's unclear to me what "pull" means here and below.

Also, given this whole file concerns logging to Analytics, including "analytics" in some field names, but not others is confusing. I'd vote to just omit it.

The main distinctions from my perspective are internal vs external, and "event" vs "param" (to use Analytics' "feature type" nomenclature from the event proposal).

To summarize, I'd vote to use "internal" and "external" prefixes to differentiate internal from external events, and "event" and "param" suffixes to differentiate event names from param names, eg INTERNAL_EVENT, INTERNAL_CHOICE_ID_PARAM, etc

Thinking (no action req'd): all these field names are consistent across Android and iOS 👍

erikeldridge · 2021-01-15T20:36:38Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+    analyticsConnector.logEvent(ANALYTICS_ORIGIN_PERSONALIZATION, ANALYTICS_PULL_EVENT, logParams);
+
+    Bundle internalLogParams = new Bundle();
+    internalLogParams.putString(CHOICE_ID_LOG_KEY, choiceId);


Thinking: the design lists one thing we want to log:

Choice ID, satisfied by the _fpid param

Thinking: the _fpc event name is correct per the events proposal.

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java

andyliangdong

I'm not quite familiar with SDK dev process, thus I'm approving from language point of view.

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

andyliangdong · 2021-01-16T00:09:32Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+    logParams.putString(ARM_VALUE, values.optString(key));
+    logParams.putString(PERSONALIZATION_ID_LOG_KEY, metadata.optString(PERSONALIZATION_ID));
+    logParams.putInt(ARM_INDEX_LOG_KEY, metadata.optInt(ARM_INDEX, -1));
+    logParams.putString(GROUP, metadata.optString(GROUP));


I'm not sure where the metadata come from? is this logging server side's decision?

Yeah, from the server.

erikeldridge · 2021-01-19T22:17:27Z

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

+    }
+
+    Bundle logParams = new Bundle();
+    logParams.putString(EXTERNAL_RC_PARAMETER_PARAM, rcParameter);


This looks great. Much easier to read 👍

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

danasilver · 2021-01-19T23:08:10Z

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java

+        .inOrder();
+    assertThat(FAKE_LOGS)
+        .comparingElementsUsing(TO_STRING)
+        .containsAtLeast(INTERNAL_LOG_PARAMS_1.toString(), INTERNAL_LOG_PARAMS_2.toString())


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

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.

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java

danasilver · 2021-01-19T23:49:40Z

@vic-flair Would also be good to add a test that logging a duplicate choice ID does not log to GA.

…Config. (#2222)

More logging for personalization.

f037c14

googlebot added the cla: yes Override cla label Dec 2, 2020

google-oss-bot added the size/L label Dec 2, 2020

vic-flair requested a review from danasilver December 2, 2020 19:26

danasilver reviewed Jan 12, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

vic-flair changed the title ~~Add support for other Firebase products to integrate with Remote Config.~~ Improve support for Firebase products that integrate with Remote Config. Jan 12, 2021

vic-flair added 2 commits January 12, 2021 11:25

Merge branch 'master' into vlum.p13n-2

ffb9786

Remove Converter.

3fdff9f

google-oss-bot added size/M and removed size/L labels Jan 12, 2021

vic-flair changed the title ~~Improve support for Firebase products that integrate with Remote Config.~~ Standardize support for Firebase products that integrate with Remote Config. Jan 12, 2021

danasilver reviewed Jan 14, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 14, 2021

View reviewed changes

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java Show resolved Hide resolved

danasilver suggested changes Jan 14, 2021

View reviewed changes

google-oss-bot added size/L and removed size/M labels Jan 15, 2021

Check cache for choice ID and add more asserts in tests.

10daf06

danasilver reviewed Jan 15, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 15, 2021

View reviewed changes

firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/Personalization.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 15, 2021

View reviewed changes

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java Outdated Show resolved Hide resolved

danasilver reviewed Jan 15, 2021

View reviewed changes

...base-config/src/test/java/com/google/firebase/remoteconfig/internal/PersonalizationTest.java Outdated Show resolved Hide resolved

danasilver suggested changes Jan 15, 2021

View reviewed changes

erikeldridge reviewed Jan 15, 2021

View reviewed changes

andyliangdong approved these changes Jan 16, 2021

View reviewed changes

vic-flair added 2 commits January 15, 2021 18:06

Rename constants.

46fb82f

Use value that looks more like actual Personalization metadata.

8268fe6

vic-flair force-pushed the vlum.p13n-2 branch from 50cfb86 to 8268fe6 Compare January 16, 2021 02:40

erikeldridge reviewed Jan 19, 2021

View reviewed changes

danasilver approved these changes Jan 19, 2021

View reviewed changes

Editing some comments.

64f38af

vic-flair merged commit a2c275c into master Jan 20, 2021

andreaowu pushed a commit that referenced this pull request Jan 29, 2021

Standardize support for Firebase products that integrate with Remote …

ef051d6

…Config. (#2222)

firebase locked and limited conversation to collaborators Feb 19, 2021

kaibolay deleted the vlum.p13n-2 branch September 15, 2022 13:32

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

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

Uh oh!

Conversation

vic-flair commented Dec 2, 2020

Uh oh!

google-oss-bot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-config

Test Logs

Notes

Uh oh!

google-oss-bot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-config

Test Logs

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danasilver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danasilver left a comment

Choose a reason for hiding this comment

Uh oh!

erikeldridge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erikeldridge Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyliangdong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyliangdong Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

vic-flair Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

erikeldridge Jan 19, 2021

google-oss-bot commented Dec 2, 2020 •

edited

Loading

`firebase-config`

google-oss-bot commented Dec 2, 2020 •

edited

Loading

`firebase-config`

erikeldridge left a comment •

edited

Loading

erikeldridge Jan 15, 2021 •

edited

Loading

erikeldridge Jan 15, 2021 •

edited

Loading

erikeldridge Jan 15, 2021 •

edited

Loading

erikeldridge Jan 15, 2021 •

edited

Loading