[Fireperf] Fix RateLimiter by avoiding manually setting time

[leotianlizhan]

Blocking #4024.

Context and why this is bad

b/243686627
TL;DR: don't use @VisibleForTesting method for real code, they break encapsulation. In this case, Timer(long time) is breaking logic too because it's mixing 2 different timebases together (time-since-epoch and time-since-VM-start). It's a bad method that should not exist even for tests.

Leftover problem

However if we remove the above, there's a leftover problem from #2662:
Before #2662, say there are 0 tokens at first. We can call rateLimiter.check() after any amount of time, so something like 1.33 tokens can be generated, but we only use long for our tokenCount, so it will automatically round to 1, and record current time as lastTimeTokenReplenished, then use that token so we are back down to 0. However if next time we call check(), 0.67 tokens are generated, then we round to 0, logic before #2662 would say we have no tokens to use, which is wrong because within the time described in above scenario, 2 tokens should have been generated, and we only used 2 tokens, so there should be enough.

#2662 tried to fix this by manually setting lastTimeTokenReplenished to the exact time at which a whole-number of tokens are added. In the same example, if 1.33 tokens are generated, then we still round-down and only add 1 to tokenCount, but we advance lastTimeTokenReplenished by the time it took to generate 1 token. This is like time only increment in ticks, with each tick guaranteed to be a multiple of the time it takes to generate a single token. This works, but manually setting time is error-prone (this PR/bug is literal proof).

Solution

Simply change tokens to double. Guava also uses double for their rate limiter's token counter https://github.com/google/guava/blob/9c1e5dea4b980202ba003b90fcb64183d42031b3/android/guava/src/com/google/common/util/concurrent/SmoothRateLimiter.java#L313

[google-oss-bot]

Coverage Report ¹

Affected Products

• firebase-perf

  Overall coverage changed from 71.38% (13083a2) to 71.37% (8ac300f) by -0.01%.

  Filename	Base (13083a2)	Merge (8ac300f)	Diff
  RateLimiter.java	90.98%	90.77%	-0.21%

Test Logs

• Base (13083a2): https://github.com/firebase/firebase-android-sdk/actions/runs/2921192983
• Merge (8ac300f): https://github.com/firebase/firebase-android-sdk/actions/runs/2923231735

Notes

• Commit (8ac300f) is created by Prow via merging PR base commit (13083a2) and head commit (497b9bc).
• Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

1. https://storage.googleapis.com/firebase-sdk-metric-reports/2mloYNBSZX.html

[google-oss-bot]

Size Report ¹

Affected Products

• firebase-perf

  Type	Base (13083a2)	Merge (8ac300f)	Diff
  aar	311 kB	311 kB	-56 B (-0.0%)
  apk (aggressive)	1.03 MB	1.03 MB	-64 B (-0.0%)
  apk (release)	2.48 MB	2.48 MB	-132 B (-0.0%)

Test Logs

• Base (13083a2): https://github.com/firebase/firebase-android-sdk/actions/runs/2921192983
• Merge (8ac300f): https://github.com/firebase/firebase-android-sdk/actions/runs/2923231735

Notes

• Commit (8ac300f) is created by Prow via merging PR base commit (13083a2) and head commit (497b9bc).

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

[github-actions]

Unit Test Results

  50 files   -    342    50 suites   - 342   1m 41s ⏱️ - 14m 50s
969 tests  - 3 739  969 ✔️  - 3 716  0 💤  - 22  0 ❌  - 1 
969 runs   - 3 755  969 ✔️  - 3 732  0 💤  - 22  0 ❌  - 1 

Results for commit 497b9bc. ± Comparison against base commit 13083a2.

[visumickey]

A similar issue would exist in iOS as well. We might need to followup on that to fix it.

[leotianlizhan]

Does iOS also have our own rate limiter implementation?

[jfcong]

Thanks for fixing this!

[leotianlizhan]

Only failing GHA is Health Metrics / Coverage (push). I asked Acore, this check is to compute metrics differences like size and coverage, and it's not required because it runs for every product (not just fireperf), so other products might fail / be flaky. I checked the logs and it indeed failed due to other products, unrelated to us, so it is okay to merge.

[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: FAILURE: Build completed with 3 failures.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 1: Task failed with an exception.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: -----------
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: * What went wrong:
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: Execution failed for task ':firebase-messaging:testDebugUnitTest'.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: ==============================================================================
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 2: Task failed with an exception.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: -----------
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: * What went wrong:
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: Execution failed for task ':firebase-storage:testDebugUnitTest'.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: ==============================================================================
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: 3: Task failed with an exception.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: -----------
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: * What went wrong:
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: Execution failed for task ':firebase-messaging:testReleaseUnitTest'.
[I] 2022-08-25 02:32:14 +0000 UTC fireci.gradle: ==============================================================================