[release-6.2] test(metrics): add functional tests verifying component_sent_bytes_total carries component_id labels

[Clee2691]

Description

This PR adds functional tests to the cloudwatch and http outputs to verify component_sent_bytes_total carries component_id labels

Relies on vector pr: ViaQ/vector#284

/cc @vparfonov
/assign @jcantrill

Links

• JIRA: https://redhat.atlassian.net/browse/LOG-7893

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1e624503-7a47-4989-8aae-f63685fea39d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-rh-openshift]

PR Summary by Qodo

Add functional metrics tests for component_sent_bytes_total component_id labels

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add CloudWatch functional regression test for [LOG-7893](https://redhat.atlassian.net/browse/LOG-7893) metric label loss.
• Add HTTP functional test as positive control for component_id labeling.
• Scrape collector /metrics and assert component_sent_bytes_total includes expected component_id.

Diagram

graph TD
  A["Ginkgo functional test"] --> B["Deploy CLF + collector"] --> C["Generate application logs"] --> D["Collector /metrics endpoint"] --> E["Parse Prometheus text"] --> F{component_id present?}
  F -->|yes| G["Test passes"]
  F -->|no| H["Fail with metric lines"]

  subgraph Legend
    direction LR
    _step["Process step"] ~~~ _dec{"Assertion"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Parse /metrics with a Prometheus text parser

• ➕ More robust than substring matching (handles ordering/spacing/escaping)
• ➕ Can assert multiple labels (component_kind/type) without brittle string checks
• ➖ Adds dependency/code complexity to tests
• ➖ May be overkill for a targeted regression guard

2. Factor out shared 'bytes metric has component_id' helper

• ➕ Reduces duplicated curl/split/filter/assert logic across output tests
• ➕ Makes it easier to add more sinks with the same assertion
• ➖ Requires introducing/maintaining a small test utility API
• ➖ Slightly obscures test intent if over-abstracted

Recommendation: The PR’s approach (end-to-end functional validation by scraping /metrics and asserting the expected component_id) is appropriate for guarding the Vector regression described in LOG-7893. If more sinks will get similar coverage, consider extracting a shared helper (and optionally a Prometheus text parser) to reduce duplication and brittleness.

Files changed (2) +112 / -0

Tests (2) +112 / -0

forward_to_cloudwatch_test.goAdd [LOG-7893](https://redhat.atlassian.net/browse/LOG-7893) regression test for CloudWatch bytes-sent metric labels +55/-0

Add LOG-7893 regression test for CloudWatch bytes-sent metric labels

• Adds a functional regression scenario that deploys a CloudWatch forwarder, generates logs, scrapes the collector metrics endpoint, and asserts that component_sent_bytes_total includes component_id="output_cloudwatch". The test documents the upstream Vector labeling issue when bytes metrics are emitted outside the component tracing span.

test/functional/outputs/cloudwatch/forward_to_cloudwatch_test.go

forward_to_http_test.goAdd HTTP output functional test validating component_id on bytes-sent metrics +57/-0

Add HTTP output functional test validating component_id on bytes-sent metrics

• Adds a functional scenario that deploys an HTTP output (with Vector HTTP output wiring), sends log messages, scrapes /metrics, and asserts component_sent_bytes_total includes component_id="output_http". Also adds required imports for logging and collector-name constants used when curling the metrics endpoint.

test/functional/outputs/http/forward_to_http_test.go

[qodo-for-rh-openshift]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 9 rules

1. Framework cleanup bypassed ✓ Resolved 🐞 Bug ☼ Reliability

Description

forward_to_http_test.go overwrites the framework created in BeforeEach, so AfterEach only
calls Cleanup() on the replacement and never closes the original test client/namespace. This leaks
a functional test namespace (and related resources) every time the new metrics test runs, which can
accumulate and destabilize CI.

Code

test/functional/outputs/http/forward_to_http_test.go[R160-166]

+	Context("When checking collector metrics for HTTP output", func() {
+		It("should emit component_sent_bytes_total with component_id label", func() {
+			framework = functional.NewCollectorFunctionalFramework()
+			obstestruntime.NewClusterLogForwarderBuilder(framework.Forwarder).
+				FromInput(obs.InputTypeApplication).
+				ToHttpOutput()
+

Relevance

⭐⭐⭐ High

Team recently changed HTTP functional tests to create fresh frameworks for reliability (PR #3337);
cleanup/resource-leak fixes likely accepted.

PR-#3337

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

BeforeEach always allocates a new framework and AfterEach cleans only the current framework
variable; the new test overwrites that variable, so the original framework never has Cleanup()
called. Cleanup() delegates to closeClient(), and client.Test.Close() is what removes the test
namespace, so skipping it leaks namespaces/resources.

test/functional/outputs/http/forward_to_http_test.go[31-49]
test/functional/outputs/http/forward_to_http_test.go[155-172]
test/framework/functional/framework.go[86-123]
test/framework/functional/framework.go[131-144]
test/client/test.go[22-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new HTTP metrics test reassigns the package-scoped `framework` variable inside the `It`, which breaks the `BeforeEach`/`AfterEach` lifecycle: the framework created in `BeforeEach` is never cleaned up, leaking the namespace created by `client.NewTest()`.

### Issue Context
- `BeforeEach` creates a framework for every test.
- `AfterEach` calls `framework.Cleanup()` once.
- The new test overwrites `framework`, so the original framework's `Cleanup()` (and thus `Test.Close()` namespace deletion) is skipped.

### Fix approach
Pick one:
1) **Preferred:** Remove the reassignment and just use the `framework` prepared by `BeforeEach` (adjust the forwarder spec in-place if needed).
2) If this test truly needs an isolated framework instance, create a **local** variable (e.g., `f := functional.NewCollectorFunctionalFramework()`), `defer f.Cleanup()`, and do not overwrite the shared `framework`.

### Fix Focus Areas
- test/functional/outputs/http/forward_to_http_test.go[31-49]
- test/functional/outputs/http/forward_to_http_test.go[155-172]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Fixed sleep before scrape ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new metrics tests use a fixed time.Sleep(15 * time.Second) before scraping /metrics, which
can be too short on slower clusters and cause intermittent failures. This also forces an
unconditional delay even when metrics are already updated, slowing the functional suite.

Code

test/functional/outputs/cloudwatch/forward_to_cloudwatch_test.go[R264-269]

+			// Wait for logs to be forwarded and metrics to be updated
+			time.Sleep(15 * time.Second)
+
+			metrics, err := framework.RunCommand(constants.CollectorName, "curl", "-ks",
+				fmt.Sprintf("https://%s.%s:24231/metrics", framework.Name, framework.Namespace))
+			Expect(err).To(BeNil(), "Expected no errors curling metrics endpoint")

Relevance

⭐⭐⭐ High

Team fixes functional flakes by switching to Eventually polling vs timing assumptions (PR #3256);
metrics scrape already uses Eventually (PR #3293).

PR-#3256
PR-#3293

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both new tests add a hard-coded 15s delay before scraping metrics exactly once; this does not
guarantee the metric has been emitted/updated and can fail on slower runs. The framework itself
demonstrates a pattern of polling for conditions (service endpoint readiness) rather than sleeping a
fixed duration.

test/functional/outputs/cloudwatch/forward_to_cloudwatch_test.go[264-270]
test/functional/outputs/http/forward_to_http_test.go[176-183]
test/framework/functional/framework.go[324-338]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The tests wait for asynchronous log forwarding/metric updates using a fixed sleep and then scrape once. This can be flaky (sleep too short) and slow (sleep always paid).

### Issue Context
The functional framework already uses condition-based polling for readiness; these tests should similarly poll until `component_sent_bytes_total{...,component_id="output_*"...}` is observed (or until a bounded timeout).

### Fix approach
Replace the fixed sleep with a bounded poll loop that:
- Repeatedly `curl -ks https://<svc>:24231/metrics`
- Ignores transient scrape/exec errors until timeout
- Stops once it finds a `component_sent_bytes_total` line containing the expected `component_id` label

You can implement this via `wait.PollUntilContextTimeout(...)` (already used by the framework) or Gomega `Eventually`.

### Fix Focus Areas
- test/functional/outputs/cloudwatch/forward_to_cloudwatch_test.go[264-298]
- test/functional/outputs/http/forward_to_http_test.go[176-207]
- test/framework/functional/framework.go[324-338]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Clee2691
Once this PR has been reviewed and has the lgtm label, please ask for approval from jcantrill. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Clee2691]

/cherry-pick master
/cherry-pick release-6.6

[openshift-cherrypick-robot]

@Clee2691: once the present PR merges, I will cherry-pick it on top of master, release-6.6 in new PRs and assign them to you.

Details

In response to this:

/cherry-pick master
/cherry-pick release-6.6

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.

[openshift-ci]

@Clee2691: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/functional-target	8dc5277	link	true	/test functional-target
ci/prow/e2e-target	8dc5277	link	true	/test e2e-target

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.