Fix fireperf RateLimiter replenishment behavior and unit alignment. #2662
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // Copyright 2021 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package com.google.firebase.perf.util; | ||
|
|
||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
|
|
||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** A Rate object representing the number of tokens per total specified time unit. */ | ||
| public class Rate { | ||
|
|
||
| private long numTokensPerTotalTimeUnit; | ||
| private long numTimeUnits; | ||
| private TimeUnit timeUnit; | ||
|
|
||
| /** | ||
| * Constructs a Rate object. | ||
| * | ||
| * <p>For example, a rate of 3 tokens per minute can be represented by new Rate(3, 1, MINUTES) or | ||
| * new Rate(3, 60, SECONDS). | ||
| * | ||
| * @param numTokensPerTotalTimeUnit The number of tokens to be issued within the specified time | ||
| * @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. |
||
| assert numTimeUnits > 0; | ||
| this.numTokensPerTotalTimeUnit = numTokensPerTotalTimeUnit; | ||
| this.numTimeUnits = numTimeUnits; | ||
| this.timeUnit = timeUnit; | ||
| } | ||
|
|
||
| /** | ||
| * Converts the rate to tokens per second. | ||
| * | ||
| * @return rate in tokens per second | ||
| */ | ||
| public double getTokensPerSeconds() { | ||
| switch (timeUnit) { | ||
| case NANOSECONDS: | ||
| return ((double) numTokensPerTotalTimeUnit / numTimeUnits) * SECONDS.toNanos(1); | ||
| case MICROSECONDS: | ||
| return ((double) numTokensPerTotalTimeUnit / numTimeUnits) * SECONDS.toMicros(1); | ||
| case MILLISECONDS: | ||
| return ((double) numTokensPerTotalTimeUnit / numTimeUnits) * SECONDS.toMillis(1); | ||
| default: | ||
| return (double) numTokensPerTotalTimeUnit / timeUnit.toSeconds(numTimeUnits); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is a possibility. But in those situations, it is fine to default it to the foreground rate.