feat: Add new hpa metrics to prevent prometheus timeseries duplication #2614
Conversation
…MetricSource and add tests
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CountryTk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: no
|
Welcome @CountryTk! |
k8s-ci-robot
added
size/L
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
Hey, could this please be reviewed @CatherineF-dev @rexagod |
|
Ok! In reviewing |
| targetMetricLabels = []string{"metric_name", "metric_target_type"} | ||
| targetMetricLabels = []string{"metric_name", "metric_target_type"} | ||
| containerMetricLabels = []string{"metric_name", "metric_target_type", "container"} | ||
| objectMetricLabels = []string{"metric_name", "metric_target_type", "full_target_name"} |
There was a problem hiding this comment.
nit: full_target_name -> target_name, container -> container_name
| ) | ||
| } | ||
|
|
||
| func createHPASpecTargetMetric() generator.FamilyGenerator { |
There was a problem hiding this comment.
nit: Unify createHPASpecTargetMetric and createHPASpecTargetObjectMetric. These two functions have some similar codes.
For example
- createHPASpecTargetMetric(CollectContainerResourceMetricSourceType=false)
- createHPASpecTargetMetric(CollectContainerResourceMetricSourceType=true)
|
Overall LGTM. Two small comments. |
Thanks for the review, I've implemented your suggestions @CatherineF-dev |
| createHPASpecTargetContainerMetric(), | ||
| createHPAStatusTargetContainerMetric(), | ||
| createHPAStatusTargetObjectMetric(), | ||
| createHPASpecTargetMetric(true), |
There was a problem hiding this comment.
Nit:
createHPASpecTargetObjectMetric()
createHPASpecTargetMetric()
func createHPASpecTargetObjectMetric() {
createHPASpecTarget([]{autoscaling.ObjectMetricSourceType})
}
func createHPASpecTargetMetric() {
createHPASpecTarget([]{autoscaling.PodsMetricSourceType, autoscaling.ResourceMetricSourceType, ...})
}
func createHPASpecTarget(autoscalingTypes) {
if m.Type not in autoscalingTypes {
return
}
}
There was a problem hiding this comment.
Made some adjustments according to how I understood this comment, not 100% sure if I did it as you wish though.
What this PR does / why we need it:
Added 4 new hpa metrics to prevent duplicated timeseries events like described in this issue: #2403
Added new metrics are:
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Cardinality is increased because of new metrics
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # #2403
FYI: I've also tested this change in our prelive cluster and for us it fixed the issue