Combine ready, not ready, terminating, and pending pod counter reporters #6617
Conversation
knative-prow-robot
added
the
size/M
There was a problem hiding this comment.
@taragu: 0 warnings.
In response to this:
/lint
Addresses #6476 (comment)
Proposed Changes
Combine ready, not ready, terminating, and pending pod counter reporters
Release Note
NONE
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
pkg/autoscaler/autoscaler_test.go
Outdated
|
|
||
| // ReportTerminatingPodCount of a mockReporter does nothing and return nil for error. | ||
| func (r *mockReporter) ReportTerminatingPodCount(v int64) error { | ||
| func (r *mockReporter) ReportActualPodCount(ready, notReady, terminating, pending int64) error { |
There was a problem hiding this comment.
the name should probably be ReportPodCounts since ActualPodCount refers to the ready pods in the metrics.
I wonder if we should change actual_pods to ready_pods here for the metrics to match better with the rest of the metrics we report. wdyt @vagababov @markusthoemmes ?
pkg/autoscaler/stats_reporter.go
Outdated
| return r.report(terminatingPodCountM.M(v)) | ||
| // ReportActualPodCount captures values for ready, not ready, terminating, and pending pod count measure. | ||
| func (r *Reporter) ReportActualPodCount(ready, notReady, terminating, pending int64) error { | ||
| var err error |
There was a problem hiding this comment.
I don't think this error variable is needed.
pkg/autoscaler/stats_reporter.go
Outdated
| // ReportActualPodCount captures values for ready, not ready, terminating, and pending pod count measure. | ||
| func (r *Reporter) ReportActualPodCount(ready, notReady, terminating, pending int64) error { | ||
| var err error | ||
| if err = r.report(actualPodCountM.M(ready)); err != nil { |
There was a problem hiding this comment.
| if err = r.report(actualPodCountM.M(ready)); err != nil { | |
| if err := r.report(actualPodCountM.M(ready)); err != nil { |
Apply to all others as well.
|
/assign |
pkg/autoscaler/stats_reporter.go
Outdated
| var err error | ||
| if err = r.report(actualPodCountM.M(ready)); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The only error in those is
if !r.initialized {
return errors.New("StatsReporter is not initialized yet")
}
which means we can just check it at the beginning and then just call report.
There was a problem hiding this comment.
Or ditch this check altogether 😂
There was a problem hiding this comment.
I am all for ditching checks 😅
There was a problem hiding this comment.
This seems like an orthogonal cleanup and probably was added for a reason (not sure which, but 🤷♂ )
There was a problem hiding this comment.
you mean the PR or the check?
I asked @taragu to do the PR as a followup to this conversation: #6476 (comment)
There was a problem hiding this comment.
Should we ditch r.initialized altogether? It seems it's not being used elsewhere except for the check
if !r.initialized {
return errors.New("StatsReporter is not initialized yet")
}
There was a problem hiding this comment.
Yeah afaik it doesn't do something useful.
knative-prow-robot
added
size/L
taragu
force-pushed
the
cleanup-report-podcounts
branch
from
1ef9553 to
6518de6
Compare
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: taragu, vagababov 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 |
knative-prow-robot
added
the
approved
|
The following jobs failed:
Automatically retrying due to test flakiness... |
/lint
Addresses #6476 (comment)
Proposed Changes
Combine ready, not ready, terminating, and pending pod counter reporters
Release Note