-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize allocation behavior of the autoscaler #10375
Optimize allocation behavior of the autoscaler #10375
Conversation
This optimizes the autoscaler's allocation behavior by 1. Collapsing metric record calls, since that path is kind of expensive 2. Avoiding to build a new logger and instead only adding the metric key where relevant 3. Avoiding to build new loggers for the stable and panic debug logs 4. Avoiding to call formatting logs by checking the log levels first
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes 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 |
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.
just nits
logger.With(zap.String("mode", "panic")).Debugf("Observed average scaling metric value: %0.3f, targeting %0.3f.", | ||
observedPanicValue, spec.TargetValue) | ||
if desugared.Core().Enabled(zapcore.DebugLevel) { | ||
desugared.Debug(fmt.Sprintf("Observed average scaling metric value: %0.3f, targeting %0.3f.", |
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.
Is it worth doing debug+sprintf on desugared, vs with+debugf?
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, the With
allocates heavily as it creates a new intermittent logger.
Codecov Report
@@ Coverage Diff @@
## master #10375 +/- ##
==========================================
- Coverage 87.99% 87.93% -0.06%
==========================================
Files 186 186
Lines 8720 8738 +18
==========================================
+ Hits 7673 7684 +11
- Misses 810 815 +5
- Partials 237 239 +2
Continue to review full report at Codecov.
|
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.
/lgtm
/test pull-knative-serving-istio-stable-no-mesh |
Proposed Changes
This optimizes the autoscaler's allocation behavior by
With the benchmark shipped in #10374, this results in the following gains:
Pulling in knative/pkg#1964 will further reduce this to:
/assign @vagababov