test(metrics): add functional tests verifying component_sent_bytes_total carries component_id labels - #3379
Conversation
|
/cherry-pick release-6.6 |
|
@Clee2691: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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-sigs/prow repository. |
PR Summary by QodoAdd functional metrics tests for component_sent_bytes_total label integrity
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
📝 WalkthroughWalkthroughChangesAdds shared RBAC and Prometheus metric scraping helpers, then uses them in CloudWatch, S3, and HTTP functional tests to validate output metric labels. The HTTP test also refactors compression setup and destination wiring. Collector metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/functional/outputs/http/forward_to_http_test.go (1)
245-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert on the collected lines instead of only logging them.
The only real gate here is the
waitFortimeout; unlike the CloudWatch/S3 tests, unlabeled duplicates ofcomponent_sent_bytes_totalwould pass silently. Add the same per-linecomponent_idassertion so this positive control is meaningful.♻️ Proposed change
lines, err := framework.CollectMetricLines("component_sent_bytes_total", `component_id="output_http"`, 30*time.Second) Expect(err).To(BeNil(), "Timed out waiting for component_sent_bytes_total with component_id label") - log.V(2).Info("matched metric lines", "lines", lines) + for _, line := range lines { + log.V(2).Info("component_sent_bytes_total line", "line", line) + Expect(line).To(ContainSubstring(`component_id=`), + "component_sent_bytes_total without component_id label (transport-layer duplicate): %s", line) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/outputs/http/forward_to_http_test.go` around lines 245 - 259, Update the test around CollectMetricLines in the component_sent_bytes_total case to assert each collected line contains the expected component_id="output_http" label, rather than only logging lines. Match the per-line label assertion pattern used by the CloudWatch/S3 tests while preserving the existing timeout and metric collection behavior.test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go (1)
253-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated metrics-reader RBAC lifecycle into the functional framework. The same ~40-line ClusterRole/ClusterRoleBinding setup and teardown is copy-pasted in three tests; a single helper (e.g.
framework.SetupMetricsRBAC()returning a cleanup func, next toCollectMetricLinesintest/framework/functional/metrics.go) removes the drift risk.
test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go#L253-L293: replace the local vars plusBeforeEach/AfterEachwith the shared helper andDeferCleanup.test/functional/outputs/aws/s3/forward_to_s3_test.go#L200-L240: same replacement.test/functional/outputs/http/forward_to_http_test.go#L203-L243: same replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go` around lines 253 - 293, Extract the duplicated metrics-reader RBAC setup and teardown into a shared framework helper, such as SetupMetricsRBAC next to CollectMetricLines, returning a cleanup function. In test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go:253-293, test/functional/outputs/aws/s3/forward_to_s3_test.go:200-240, and test/functional/outputs/http/forward_to_http_test.go:203-243, remove the local RBAC variables and BeforeEach/AfterEach lifecycle, invoke the helper, and register its cleanup with DeferCleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/framework/functional/metrics.go`:
- Around line 19-23: Update the metrics polling flow around f.RunCommand to
propagate ctx and add curl’s --max-time bounded by the poll timeout, while
retaining the most recent execution or scrape error in lastErr. On return, wrap
the final error with the last scrape error when both are present, preserving the
matched result and existing success behavior.
---
Nitpick comments:
In `@test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go`:
- Around line 253-293: Extract the duplicated metrics-reader RBAC setup and
teardown into a shared framework helper, such as SetupMetricsRBAC next to
CollectMetricLines, returning a cleanup function. In
test/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.go:253-293,
test/functional/outputs/aws/s3/forward_to_s3_test.go:200-240, and
test/functional/outputs/http/forward_to_http_test.go:203-243, remove the local
RBAC variables and BeforeEach/AfterEach lifecycle, invoke the helper, and
register its cleanup with DeferCleanup.
In `@test/functional/outputs/http/forward_to_http_test.go`:
- Around line 245-259: Update the test around CollectMetricLines in the
component_sent_bytes_total case to assert each collected line contains the
expected component_id="output_http" label, rather than only logging lines. Match
the per-line label assertion pattern used by the CloudWatch/S3 tests while
preserving the existing timeout and metric collection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa9f1766-6902-403d-af8c-756dd3c5e451
📒 Files selected for processing (4)
test/framework/functional/metrics.gotest/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.gotest/functional/outputs/aws/s3/forward_to_s3_test.gotest/functional/outputs/http/forward_to_http_test.go
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
…tal carries component_id labels
384e5d6 to
6ae03b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/framework/functional/metrics.go`:
- Around line 37-48: Update the setup flow around metricsReaderBinding and
tokenReviewBinding so that if a later f.Test.Create call fails, all previously
created RBAC resources are deleted before returning the error. Add cleanup for
metricsReaderBinding when tokenReviewBinding creation fails, and preserve the
existing error return behavior while ensuring each successfully created resource
is removed on partial setup failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 56a1b946-4525-450f-871a-cdc569ca6b22
📒 Files selected for processing (4)
test/framework/functional/metrics.gotest/functional/outputs/aws/cloudwatch/forward_to_cloudwatch_test.gotest/functional/outputs/aws/s3/forward_to_s3_test.gotest/functional/outputs/http/forward_to_http_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/outputs/http/forward_to_http_test.go
|
/retest |
|
@Clee2691: The following test failed, say
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. |
| runtime.NewClusterRoleRef(roleName), | ||
| runtime.NewServiceAccountSubject("default", f.Namespace), | ||
| ) | ||
| if err = f.Test.Create(metricsReaderBinding); err != nil { |
There was a problem hiding this comment.
I don't recall if we have a cleanup function for functional tests but do we need to be concerned about cleaning up these clusterrolbinding in a test cluster because of potentially "false positives" due to essentially "name" collision?
There was a problem hiding this comment.
This is not a concern because the name of the binding and other rbac resources is derived like this:
roleName := fmt.Sprintf("%s-%s-metrics-reader", f.Test.NS.Name, f.Name)
The test's namespace name is unique for every tests so there shouldn't be any collision especially since the NS name is generated from UniqueNameForTest()
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Clee2691, jcantrill 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
|
/lgtm |
4820fd0
into
openshift:master
|
@Clee2691: new pull request created: #3382 DetailsIn response to this:
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-sigs/prow repository. |
3 participants
Description
This PR is a manual cherry-pick of #3348.
This PR also adds a test for AWS S3 along with setup for metrics auth.
/cc @vparfonov
/assign @jcantrill
Links
Summary by CodeRabbit
/metricsendpoint and poll until expected metrics appear.component_idlabeling and ensureregionis present where expected.