Implement remote config fetcher to fetch and cache configs. #4967

visumickey · 2023-05-02T01:16:50Z

No description provided.

google-oss-bot · 2023-05-02T01:28:03Z

Coverage Report ¹

Affected Products

`firebase-sessions`

Overall coverage changed from ? (d4a30e1) to 68.79% (485f4b5) by ?.

17 individual files with coverage change

Filename	Base (`d4a30e1`)	Merge (`485f4b5`)	Diff
ApplicationInfo.kt	?	100.00%	?
EventGDTLogger.kt	?	75.00%	?
FirebaseSessions.kt	?	0.00%	?
FirebaseSessionsEarly.kt	?	0.00%	?
FirebaseSessionsRegistrar.kt	?	0.00%	?
LocalOverrideSettings.kt	?	88.46%	?
RemoteSettings.kt	?	85.53%	?
RemoteSettingsFetcher.kt	?	3.23%	?
SessionCoordinator.kt	?	76.47%	?
SessionEvent.kt	?	100.00%	?
SessionEvents.kt	?	100.00%	?
SessionGenerator.kt	?	20.00%	?
SessionInitiator.kt	?	66.67%	?
SessionsSettings.kt	?	90.00%	?
SettingsCache.kt	?	94.12%	?
SettingsProvider.kt	?	0.00%	?
Time.kt	?	0.00%	?

Test Logs

Merge (485f4b5): https://github.com/firebase/firebase-android-sdk/actions/runs/4888731711

https://storage.googleapis.com/firebase-sdk-metric-reports/3AWi645jZY.html

google-oss-bot · 2023-05-02T01:40:55Z

Size Report ¹

Affected Products

base
Type Base (d4a30e1) Merge (485f4b5) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-datatransport
Type Base (d4a30e1) Merge (485f4b5) Diff
aar ? 4.94 kB ? (?)
apk (aggressive) ? 161 kB ? (?)
apk (release) ? 1.35 MB ? (?)
firebase-sessions
Type Base (d4a30e1) Merge (485f4b5) Diff
aar ? 93.9 kB ? (?)

Test Logs

Merge (485f4b5): https://github.com/firebase/firebase-android-sdk/actions/runs/4888731711

https://storage.googleapis.com/firebase-sdk-metric-reports/jJcvLwCr9L.html

github-actions · 2023-05-02T01:45:01Z

Unit Test Results

22 files 22 suites 31s ⏱️
35 tests 35 ✔️ 0 💤 0 ❌
70 runs 70 ✔️ 0 💤 0 ❌

Results for commit e21558d.

♻️ This comment has been updated with latest results.

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt

...ase-sessions/src/test/kotlin/com/google/firebase/sessions/testing/FakeRemoteConfigFetcher.kt

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/RemoteSettingsTest.kt

google-oss-bot · 2023-05-03T07:24:20Z

Startup Time Report ¹

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Startup time comparison between the CI merge commit (485f4b5) and the base commit (d4a30e1) are not available.

No macrobenchmark data found for the base commit (d4a30e1). Analysis for the CI merge commit (485f4b5) can be found at:

https://storage.googleapis.com/firebase-sdk-metric-reports/NICHfrhJHO/index.html

https://storage.googleapis.com/firebase-sdk-metric-reports/iEC3KqbrFM/index.html

samedson · 2023-05-03T16:39:07Z

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt

+            val aqsSettings = it.get("app_quality") as JSONObject
+            try {
+              if (aqsSettings.has("sessions_enabled")) {
+                sessionsEnabled = aqsSettings.get("sessions_enabled") as Boolean?
+              }
+
+              if (aqsSettings.has("sampling_rate")) {
+                sessionSamplingRate = aqsSettings.get("sampling_rate") as Double?
+              }

-      // Check if cache is expired. If not, return
-      // Initiate a fetch. On successful response cache the fetched values
+              if (aqsSettings.has("session_timeout_seconds")) {
+                sessionTimeoutSeconds = aqsSettings.get("session_timeout_seconds") as Int?
+              }

+              if (aqsSettings.has("cache_duration")) {
+                cacheDuration = aqsSettings.get("cache_duration") as Int?
+              }
+            } catch (exception: JSONException) {
+              Log.e(TAG, "Error parsing the configs remotely fetched: ", exception)
+            }
+          }
+
+          sessionsEnabled?.let { settingsCache.updateSettingsEnabled(sessionsEnabled) }
+
+          sessionTimeoutSeconds?.let {
+            settingsCache.updateSessionRestartTimeout(sessionTimeoutSeconds)
+          }
+
+          sessionSamplingRate?.let { settingsCache.updateSamplingRate(sessionSamplingRate) }
+
+          cacheDuration?.let { settingsCache.updateSessionCacheDuration(cacheDuration) }
+            ?: let { settingsCache.updateSessionCacheDuration(86400) }
+
+          settingsCache.updateSessionCacheUpdatedTime(System.currentTimeMillis())
+          fetchInProgress = false


WDYT about putting the onSuccess block in a separate method?

Not sure what gains that provides. I would rather leave it here as the only thing that is done on success is to cache the values. So, leaving it as is.

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt

samedson · 2023-05-03T16:41:31Z

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/RemoteSettingsTest.kt

+    fetchedResponse.remove("app_quality")
+
+    // Sleep for a second before updating configs
+    Thread.sleep(2000)


Do the sleeps slow down tests?

It does - for 2 seconds. Basically it is a situations where we are setting the cache duration for configs to be set as 1 second and 2 seconds later, we are providing updated configs.

Ah I see - we should be able to avoid sleeps. Sleeps are bad cause they slow down the tests and are flakey. We get the time here it seems:

firebase-android-sdk/firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/SettingsCache.kt

Line 73 in 8c12472

val currentTimestamp = System.currentTimeMillis()

We can pass this in as an function parameter to RemoteSettings. That way we can fast forward time in the test. We do that in our Session Initiator Tests:

firebase-android-sdk/firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionInitiatorTest.kt

Line 136 in 8c12472

fakeTimeProvider.addInterval(LARGE_INTERVAL)

Tried making the refactor to accomodate the TimeProvider, but that seem to take a lot more refactor that what it is today. Example: Time class internally does not provide time comparison with another time value. Until that is done, it is hard to refactor to use that class. So, will leave it as is for now since the time internal to sleep here is just 2 seconds.

...ase-sessions/src/test/kotlin/com/google/firebase/sessions/testing/FakeRemoteConfigFetcher.kt

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt

google-oss-bot added the size/XL label May 2, 2023

Implement remote config fetcher to cache values.

cf4b036

visumickey force-pushed the firebase-sessions-config-fetcher branch from a220260 to cf4b036 Compare May 2, 2023 01:23

visumickey added the no-changelog label May 2, 2023

visumickey requested review from samedson and mrober May 2, 2023 01:23

mrober reviewed May 2, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

mrober reviewed May 2, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

mrober reviewed May 2, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

mrober reviewed May 2, 2023

View reviewed changes

...ase-sessions/src/test/kotlin/com/google/firebase/sessions/testing/FakeRemoteConfigFetcher.kt Outdated Show resolved Hide resolved

mrober reviewed May 2, 2023

View reviewed changes

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/RemoteSettingsTest.kt Outdated Show resolved Hide resolved

visumickey added 4 commits May 2, 2023 23:29

Reduce main thread usage with dispatcher threads.

08b27df

Organize code better.

d5ecb05

Address comments.

165fd4f

Address more comments.

c87f38b

visumickey changed the title ~~Implement remote config fetcher to cache values.~~ Implement remote config fetcher to fetch and cache configs. May 3, 2023

samedson reviewed May 3, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

samedson reviewed May 3, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

samedson reviewed May 3, 2023

View reviewed changes

...ase-sessions/src/test/kotlin/com/google/firebase/sessions/testing/FakeRemoteConfigFetcher.kt Show resolved Hide resolved

Address comments.

8c12472

mrober approved these changes May 4, 2023

View reviewed changes

mrober reviewed May 4, 2023

View reviewed changes

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/settings/RemoteSettings.kt Outdated Show resolved Hide resolved

visumickey added 2 commits May 4, 2023 17:21

Introduce background and blocking dispatchers for Firebase alignment.

443b8bf

Fix styling.

e21558d

samedson approved these changes May 5, 2023

View reviewed changes

visumickey merged commit c3b0dcc into firebase-sessions May 5, 2023

visumickey deleted the firebase-sessions-config-fetcher branch May 5, 2023 16:18

firebase locked and limited conversation to collaborators Jun 5, 2023

Implement remote config fetcher to fetch and cache configs. #4967

Implement remote config fetcher to fetch and cache configs. #4967

Uh oh!

Conversation

visumickey commented May 2, 2023

Uh oh!

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

Uh oh!

Coverage Report 1

Affected Products

firebase-sessions

Test Logs

Uh oh!

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

Uh oh!

Size Report 1

Affected Products

base

firebase-datatransport

firebase-sessions

Test Logs

Uh oh!

github-actions bot commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

google-oss-bot commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Startup Time Report 1

Uh oh!

samedson May 3, 2023

Choose a reason for hiding this comment

Uh oh!

visumickey May 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

samedson May 3, 2023

Choose a reason for hiding this comment

Uh oh!

visumickey May 3, 2023

Choose a reason for hiding this comment

Uh oh!

samedson May 4, 2023

Choose a reason for hiding this comment

Uh oh!

visumickey May 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

google-oss-bot commented May 2, 2023 •

edited

Loading

Coverage Report ¹

`firebase-sessions`

google-oss-bot commented May 2, 2023 •

edited

Loading

Size Report ¹

`base`

`firebase-datatransport`

`firebase-sessions`

github-actions bot commented May 2, 2023 •

edited

Loading

google-oss-bot commented May 3, 2023 •

edited

Loading

Startup Time Report ¹