Add the annotation for averaging algorithm selection. #10840
Conversation
I've tried to document the differences. Obviously this will have to go to the public documentation as well. I'll provide the rest of the management in a separate PR, once we've done bikeshedding the name and the comment. Change-Id: Iaa66d4968cc3a38ceb16e63bb2d238c80ad9831e
Codecov Report
@@ Coverage Diff @@
## master #10840 +/- ##
==========================================
- Coverage 88.06% 88.04% -0.03%
==========================================
Files 188 188
Lines 9036 9036
==========================================
- Hits 7958 7956 -2
- Misses 827 828 +1
- Partials 251 252 +1
Continue to review full report at Codecov.
|
| // KPA will compute the decay multiplier automatically and it is capped | ||
| // at 0.2 from below. This algorithm might not utilize all the values | ||
| // in the window, due to their coefficients being infinitesimal. | ||
| MetricAggregationAlgorithm = GroupName + "/metricAggregationAlgorithm" |
There was a problem hiding this comment.
Should we mark this experimental somehow? Either just in the comment or even in the annotation name/value? I think we want the freedom to muck around with the UX a bit before it's "set" and we wouldn't want anyone insisting on backwards compat before then. If we feel it's actually good-to-go before next release we can always remove the warning. (I guess a counter-argument is this is an annotation anyway but.. I'm not convinced people treat annotations as hints that might go away in this project)
There was a problem hiding this comment.
I can leave this as a comment.
pkg/apis/autoscaling/register.go
Outdated
| // metric window (default); | ||
| // - weightedExponential — weighted average with exponential decay. | ||
| // KPA will compute the decay multiplier automatically and it is capped | ||
| // at 0.2 from below. This algorithm might not utilize all the values |
There was a problem hiding this comment.
The word "below" confused me for a bit here. I was initially reading it as referring to an algorithm lower down in the file, but I think we mean 0.2 is the lower bound of the multiplier? Maybe we could use some other form of words if so, like "with a lower bound of 0.2" or just "at least 0.2"
pkg/apis/autoscaling/register.go
Outdated
| // - empty/missing or "linear" — linear average over the whole | ||
| // metric window (default); | ||
| // - weightedExponential — weighted average with exponential decay. | ||
| // KPA will compute the decay multiplier automatically and it is capped |
There was a problem hiding this comment.
I'd like to expand "automatically" somehow, is there a good way to explain the criteria it uses to calculate the multiplier in half a sentence? :). "Automatically based on the size of the window" or something?
| @@ -83,6 +83,19 @@ const ( | |||
| // scale-to-zero-pod-retention-period global setting. | |||
| ScaleToZeroPodRetentionPeriodKey = GroupName + "/scaleToZeroPodRetentionPeriod" | |||
|
|
|||
| // MetricAggregationAlgorithm is the annotation that can be used for selection | |||
| // of the algorithm to use for averaging metric data in the Autoscaler. | |||
There was a problem hiding this comment.
Are we going to apply the same algorithm to both windows or just one? If both, will it always be the same algorithm for both or would there be a use case for a diff algorithm per window? (Obviously asking because we may want to name this StableWindowAggregationAlgorithm, for example, if so).
There was a problem hiding this comment.
you mean panic and stable? I thought to both for uniformity.
There was a problem hiding this comment.
Interestingly, it is probably less important for the panic window, since they are shorter and difference between regular and weighted avg is small.
|
Pushed a new version. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've tried to document the differences. Obviously this will have to go
to the public documentation as well.
I'll provide the rest of the management in a separate PR, once we've
done bikeshedding the name and the comment.
/assign @julz @markusthoemmes
p.s. Markus is out, so it's more like for post-review review 😁