[release-6.6] test(metrics): add functional tests verifying component_sent_bytes_total carries component_id labels - #3382
Merged
openshift-merge-bot[bot] merged 1 commit intoJul 30, 2026
Conversation
…tal carries component_id labels
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR Summary by QodoAdd functional metrics tests for component_sent_bytes_total label coverage
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Contributor
Contributor
|
/assign @jcantrill |
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1. Overbroad metric matching
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
Comment on lines
+68
to
+75
| for _, line := range strings.Split(raw, "\n") { | ||
| if strings.HasPrefix(line, "#") { | ||
| continue | ||
| } | ||
| if strings.Contains(line, metricName) { | ||
| matched = append(matched, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Overbroad metric matching 🐞 Bug ☼ Reliability
CollectMetricLines selects lines using strings.Contains(line, metricName), which can accidentally include non-target series whose names merely contain the substring (e.g., similarly named metrics), making the new tests brittle and potentially failing for unrelated metric lines. The repo’s canonical metric name is vector_component_sent_bytes_total, but the new tests pass the shorter component_sent_bytes_total substring, relying on this imprecise matching.
Agent Prompt
### Issue description
`CollectMetricLines()` currently uses substring matching (`strings.Contains`) to collect metric lines. This can unintentionally match other series that merely contain the substring, making tests flaky/brittle and diagnostics confusing.
### Issue Context
The repo’s canonical metric name is `vector_component_sent_bytes_total`, but the new tests call `CollectMetricLines("component_sent_bytes_total", ...)`, depending on substring matching.
### Fix Focus Areas
- test/framework/functional/metrics.go[54-82]
- test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go[245-286]
### Implementation guidance
- Parse the metric token (the substring before the first `{` or whitespace) and compare it to an **exact** metric name.
- Update call sites to pass the full metric name (e.g. `vector_component_sent_bytes_total`) and match with `HasPrefix(line, metricName+"{")` / token parse.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+24
to
+49
| metricsReaderRole = runtime.NewClusterRole( | ||
| roleName, | ||
| runtime.NewNonResourceURLPolicyRule([]string{"/metrics"}, []string{"get"}), | ||
| ) | ||
| if err = f.Test.Create(metricsReaderRole); err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| metricsReaderBinding = runtime.NewClusterRoleBinding( | ||
| roleName, | ||
| runtime.NewClusterRoleRef(roleName), | ||
| runtime.NewServiceAccountSubject("default", f.Namespace), | ||
| ) | ||
| if err = f.Test.Create(metricsReaderBinding); err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| tokenReviewName := fmt.Sprintf("%s-%s-token-reviewer", f.Test.NS.Name, f.Name) | ||
| tokenReviewBinding = runtime.NewClusterRoleBinding( | ||
| tokenReviewName, | ||
| runtime.NewClusterRoleRef("system:auth-delegator"), | ||
| runtime.NewServiceAccountSubject("default", f.Namespace), | ||
| ) | ||
| if err = f.Test.Create(tokenReviewBinding); err != nil { | ||
| return nil, nil, nil, err | ||
| } |
There was a problem hiding this comment.
2. Rbac setup leaks on failure 🐞 Bug ☼ Reliability
SetupMetricsRBAC returns early on create errors without deleting any previously created cluster-scoped RBAC objects, so a mid-way failure leaves ClusterRoles/ClusterRoleBindings behind. Since caller cleanup is only registered after SetupMetricsRBAC returns successfully, partial failures are not cleaned up.
Agent Prompt
### Issue description
`SetupMetricsRBAC()` creates multiple cluster-scoped RBAC resources sequentially. If creation fails mid-way, earlier-created resources are leaked because the function returns immediately and callers only register cleanup after a successful return.
### Issue Context
This affects functional test runs by leaving cluster-wide RBAC artifacts behind on transient failures (API errors, timeouts, AlreadyExists, etc.).
### Fix Focus Areas
- test/framework/functional/metrics.go[24-49]
### Implementation guidance
- Track successfully-created objects and, on any subsequent error, delete the already-created ones before returning.
- Alternatively, register internal deferred rollback logic within `SetupMetricsRBAC()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+16
to
+22
| // SetupMetricsRBAC creates the cluster-scoped RBAC resources needed to scrape | ||
| // the collector's /metrics endpoint from within a test pod. Names are prefixed | ||
| // with the framework's namespace so parallel test packages cannot collide. | ||
| // The returned function deletes all created resources and should be registered | ||
| // with DeferCleanup or called in AfterEach. | ||
| func (f *CollectorFunctionalFramework) SetupMetricsRBAC() (metricsReaderRole *rbacv1.ClusterRole, metricsReaderBinding *rbacv1.ClusterRoleBinding, tokenReviewBinding *rbacv1.ClusterRoleBinding, err error) { | ||
| roleName := fmt.Sprintf("%s-%s-metrics-reader", f.Test.NS.Name, f.Name) |
There was a problem hiding this comment.
3. Misleading cleanup comment 🐞 Bug ⚙ Maintainability
SetupMetricsRBAC’s doc comment claims it “returns a function” that deletes created resources, but the function actually returns RBAC objects and an error. This mismatch can mislead future callers into missing required cleanup.
Agent Prompt
### Issue description
The doc comment for `SetupMetricsRBAC` states it returns a cleanup function, but the function signature returns RBAC objects. This is misleading and can cause future misuse/leaks.
### Issue Context
Call sites currently perform cleanup themselves.
### Fix Focus Areas
- test/framework/functional/metrics.go[16-22]
### Implementation guidance
- Update the comment to describe that the function returns the created objects and the caller must delete them, OR change the API to actually return a cleanup function.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
@openshift-cherrypick-robot: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
Contributor
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill, openshift-cherrypick-robot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit Jul 30, 2026
2093483
into
openshift:release-6.6
9 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #3379
/assign Clee2691