remove prometheus dependencies from k/k #89285
Conversation
| } | ||
|
|
||
| // ExpectFindMetric find a metric with the given name nad labels or reports a fatal test error. | ||
| func ExpectFindMetric(t *testing.T, name string, expectedLabels map[string]string) *dto.Metric { |
There was a problem hiding this comment.
I'm afraid we can't just simply move LabelsMatch and ExpectFindMetric to components.
There are a few methods you can select in testutil package, such as CollectAndCompare and GatherAndCompare, etc.
That means you need to refactor the original test case with the methods in testutil.
There was a problem hiding this comment.
sure , thanks your comment , i will udpate it
|
/cc @logicalhan |
Sure , Thanks for your review. |
|
/retest |
|
/test pull-kubernetes-dependencies |
|
/cc @RainbowMango |
|
/test pull-kubernetes-e2e-kind-ipv6 |
1 similar comment
|
/test pull-kubernetes-e2e-kind-ipv6 |
| // LabelsMatch returns true if metric has all expected labels otherwise false | ||
| func LabelsMatch(metric *dto.Metric, labelFilter map[string]string) bool { | ||
| for _, lp := range metric.GetLabel() { | ||
| if value, ok := labelFilter[lp.GetName()]; ok && lp.GetValue() != value { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The docstring looks like it's not true. Let's say we have a labelFilter that this:
map[string]string{
"a": "expectedVal1",
"b": "expectedVal2",
}
If the metric only has a single label, let's say 'a', then this function returns true, which seems not in line with the documentation.
There was a problem hiding this comment.
oo , sure .
Thanks . I will update it .
There was a problem hiding this comment.
/cc @logicalhan
/cc @RainbowMango
could you review it again?Thanks
| gotLabelCount := len(metric.GetLabel()) | ||
| wantLabelCount := len(expectedLabels) | ||
| if wantLabelCount != gotLabelCount { | ||
| t.Errorf("Got metric with %d labels, but wanted %d labels. Wanted %#+v for %s", | ||
| gotLabelCount, wantLabelCount, expectedLabels, metric.String()) | ||
| } | ||
| return metric |
There was a problem hiding this comment.
This line is important. You need to return here otherwise this function will never successfully find a match.
This is why the blaze unit test is failing. You can run these tests locally (bazel test //staging/src/k8s.io/apiserver/pkg/admission/metrics:go_default_test) to debug what is happening.
|
/retest |
1 similar comment
|
/retest |
| "a": "1", | ||
| "b": "2", | ||
| "c": "3", |
There was a problem hiding this comment.
Do you mind adding two edge cases?
(1) empty LabelPair and a non-empty map
(2) non-empty LabelPair but an empty map
There was a problem hiding this comment.
/cc @logicalhan
is it ok now?
| labelFilter map[string]string | ||
| expectedMatch bool | ||
| }{ | ||
| {"metric's labels and labelFilter have the same labels and values", &dto.Metric{ |
There was a problem hiding this comment.
Sorry for reply late.
Would you mind replace metric's with metric? That'll be ugly if subtest's name contains a specific symbol.
Same as below.
And, why not set fields name here? like,
{
name: "...",
metric: ...
....
}
There was a problem hiding this comment.
ok . I have replace metric's with metric.
And for fields, I have updated it though i think both are ok.
Thanks for your reply.
|
kindly |
| for labelName, labelValue := range labelFilter { | ||
| labelFound := false | ||
| for _, labelPair := range metric.Label { | ||
| if labelName == *labelPair.Name && labelValue == *labelPair.Value { |
There was a problem hiding this comment.
labelPair.GetName() returns the literal string and likewise for labelPair.GetValue()
| for _, labelPair := range metric.Label { | ||
| if labelName == *labelPair.Name && labelValue == *labelPair.Value { | ||
| labelFound = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: we can bring the nested for-loop to the outer scope to avoid the repeated work (practically the number of labels are going to be bounded in tests and hopefully in our metrics).
metricLabels := map[string]string{}
for _, labelPair := range metric.Label {
metricLabels[labelPair.GetName()] = labelPair.GetValue()
}
// length comparison then match key to values in the maps
There was a problem hiding this comment.
/cc @logicalhan
updated
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, logicalhan, tanjunchen 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 |
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Ref kubernetes/enhancements#1238
ref #89267
Special notes for your reviewer:
/cc @RainbowMango
/cc @serathius
Could you help me review it if you are free. Thanks
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: