Fix fireperf RateLimiter replenishment behavior and unit alignment. #2662
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (1b057bff) is created by Prow via merging commits: 4bc47df a2525a1. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (1b057bff) is created by Prow via merging commits: 4bc47df a2525a1. |
firebase-perf/src/main/java/com/google/firebase/perf/util/Rate.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Rate.java
Outdated
Show resolved
Hide resolved
|
|
||
| private long foregroundCapacity; | ||
| private long backgroundCapacity; | ||
|
|
||
| RateLimiterImpl( | ||
| final double rate, | ||
| final Rate rate, |
There was a problem hiding this comment.
I'm noticing further down that there is a separate foreground and background rate, but they seem to be ignoring this initial one and just blindly overwriting it. Do you understand how it works?
There was a problem hiding this comment.
I think the foreground and background rate is set during initilization and read from remote config, but the passed in rate is still in use, until changeRate() is called. changeRate() is called by the TransportManager when an app updates its state (i.e., changes from foreground to background). At least that's my understanding.
There was a problem hiding this comment.
But shouldn't the passed-in rate be either foreground or background? Why would there be a 3rd rate that is only used to initialize and then overwritten?
There was a problem hiding this comment.
Perhaps because we don't know whether the app will be in foreground or background when it finishes initializing? I am not sure if there were other reasons that I am not aware of. We could remove the passed in rate and set it to either foreground or background on initialization if that makes more sense (but which one?)
There was a problem hiding this comment.
@visumickey do you know why this is the way it is and whether it makes sense to keep it or change it?
There was a problem hiding this comment.
This seems weird. I don't see a need for a third rate. My recommendation would be not to pass the rate as a parameter since we anyways extract the rate from the configResolver.
As a part of the initialization, we can check on the state of the application to get the rate and use that for the token generation. Eg: on iOS, currently we do not pass the rate as a parameter, but use the state of the application to get the rate from config resolver as specified above.
There was a problem hiding this comment.
That sounds good - I'd recommend leaving it out of this PR though to not continue changing more things in a single PR.
There was a problem hiding this comment.
Sounds good, I will leave that out of this PR. Although, is there a possibility that the app has not finished initializing yet so it's not in either foreground or background?
There was a problem hiding this comment.
Yes, that is a possibility. But in those situations, it is fine to default it to the foreground rate.
firebase-perf/src/test/java/com/google/firebase/perf/util/RateTest.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/test/java/com/google/firebase/perf/transport/RateLimiterTest.java
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Rate.java
Outdated
Show resolved
Hide resolved
|
|
||
| private long foregroundCapacity; | ||
| private long backgroundCapacity; | ||
|
|
||
| RateLimiterImpl( | ||
| final double rate, | ||
| final Rate rate, |
There was a problem hiding this comment.
But shouldn't the passed-in rate be either foreground or background? Why would there be a 3rd rate that is only used to initialize and then overwritten?
firebase-perf/src/test/java/com/google/firebase/perf/transport/RateLimiterTest.java
Show resolved
Hide resolved
| * @param numTimeUnits The number of specified time unit | ||
| * @param timeUnit The specified time unit | ||
| */ | ||
| public Rate(long numTokensPerTotalTimeUnit, long numTimeUnits, TimeUnit timeUnit) { |
There was a problem hiding this comment.
Curious if we can unify the second and third parameter together and be prescriptive about the timeUnit?
Eg. new Rate(3, 60) - Would mean, 3 tokens in 60 seconds.
Any reason to keep the timeUnit as a separate parameter?
There was a problem hiding this comment.
This is to prevent bugs like the existing one; if you only pass an int or long, it's very easy to get the units wrong (before we were mixing up minutes and seconds). By passing this, it's impossible to get it wrong.
There was a problem hiding this comment.
I think the third parameter is there mainly for readability reasons. Before in this class, there are two conflicting comments about what the rate is: one says it's token per minute and the other one says it's token per second. By explicitly passing in the TimeUnit, we make it very clear that new Rate(3, 60, SECONDS) means 3 tokens per 60 seconds, and not 3 tokens per 60 minutes or vice versa.
Besides, by keeping the third parameter, we can also make testing more readable, by specifying rates of units other than SECONDS. For example, we can specify new Rate(2, 1, MINUTE) for 2 tokens per minute.
There was a problem hiding this comment.
I definitely see the need for the readability part. My suggestion was to create the readability by calling the time unit to be always in seconds. Eg. (numberOfTokensForTimeLimit, timeLimitInSeconds)
The current is definitely super verbose and helpful. Was just feeling that verbosity can come by qualifying the parameter with better names.
Anyways, not too tied to my thoughts on this. Fine with this change as well.
|
|
||
| private long foregroundCapacity; | ||
| private long backgroundCapacity; | ||
|
|
||
| RateLimiterImpl( | ||
| final double rate, | ||
| final Rate rate, |
There was a problem hiding this comment.
This seems weird. I don't see a need for a third rate. My recommendation would be not to pass the rate as a parameter since we anyways extract the rate from the configResolver.
As a part of the initialization, we can check on the state of the application to get the rate and use that for the token generation. Eg: on iOS, currently we do not pass the rate as a parameter, but use the state of the application to get the rate from config resolver as specified above.
firebase-perf/src/test/java/com/google/firebase/perf/transport/RateLimiterTest.java
Show resolved
Hide resolved
Fixed two bugs in the RateLimiter: