test(registration-service): expose metrics for the git commit#1269
test(registration-service): expose metrics for the git commit#1269xcoulon wants to merge 2 commits intocodeready-toolchain:masterfrom
Conversation
xcoulon
force-pushed
the
prometheus_metric_commit
branch
from
cf06267 to
482a363
Compare
WalkthroughRefactors test metrics infrastructure to configure routes with explicit port and TLS settings, parameterize metrics retrieval by base URL, add registration-service metrics handling, and introduce new metric constants and reorganized subtests validating commit and version labels. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Wait as Awaitility
participant Route as OpenShift Route/Ingress
participant HostMetrics as host-operator-metrics-service
participant RegMetrics as registration-service-metrics
participant MemberOp as member-operator-instance
rect rgba(200,200,255,0.5)
Test->>Wait: SetupRouteForService(service, path, port, tls)
Wait->>Route: create/ensure Route with spec.Port & spec.TLS
end
rect rgba(200,255,200,0.5)
Test->>Wait: WaitForRouteToBeAvailable(ns,name,path)
Wait->>Route: poll route host
Route-->>Wait: host ready
Wait-->>Test: route host returned
end
rect rgba(255,200,200,0.5)
Test->>HostMetrics: GET {hostMetricsURL}/metrics
HostMetrics-->>Test: Prometheus metrics
Test->>RegMetrics: GET {regSvcMetricsURL}/metrics
RegMetrics-->>Test: Prometheus metrics
Test->>MemberOp: GET {memberMetricsURL}/metrics
MemberOp-->>Test: Prometheus metrics
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
also, test new `commit` and `short_commit` metrics for host and member operators Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
xcoulon
force-pushed
the
prometheus_metric_commit
branch
from
482a363 to
a75907b
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/metrics/metrics_test.go (1)
74-75:⚠️ Potential issue | 🟠 MajorBug:
member2Awaitis incorrectly assignedMember1()instead ofMember2().This appears to be a copy-paste error. Both
member1Awaitandmember2Awaitare assigned the same member operator, which means the test is effectively testing the same member twice rather than two different members.Proposed fix
// given member1Await := awaitilities.Member1() - member2Await := awaitilities.Member1() + member2Await := awaitilities.Member2()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 74 - 75, The test mistakenly assigns member2Await using awaitilities.Member1() instead of awaitilities.Member2(), causing both variables to refer to the same member; update the assignment of member2Await to call awaitilities.Member2() so the test exercises two distinct members (change the second line where member2Await is defined).
🧹 Nitpick comments (4)
testsupport/init.go (1)
209-245: Consider extracting the duplicated"/metrics"path literal.SonarCloud flagged this literal as duplicated 3 times. While the code is functional, extracting it to a constant would improve maintainability.
Suggested fix
Add a constant at the top of the function or file:
const metricsPath = "/metrics"Then use it in the route setup calls:
- hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics", + hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", metricsPath,Apply similarly for lines 222 and 235.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/init.go` around lines 209 - 245, Extract the duplicated "/metrics" literal into a single constant (e.g., const metricsPath = "/metrics") at the top of the function or file, then replace the three usages in the SetupRouteForService calls for initHostAwait.SetupRouteForService (host metrics), initHostAwait.SetupRouteForService (registration service metrics), and initMemberAwait.SetupRouteForService (member metrics) with that constant to remove duplication and improve maintainability.testsupport/metrics/metrics.go (1)
45-45: Inconsistent HTTP method casing.This line still uses
"Get"while line 139 was updated to"GET". While Go'shttp.NewRequestis case-insensitive for method names, using the canonical uppercase form is preferred for consistency and clarity.Suggested fix
- request, err := http.NewRequest("Get", uri, nil) + request, err := http.NewRequest("GET", uri, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/metrics/metrics.go` at line 45, The HTTP method string in the http.NewRequest call uses "Get" inconsistent with the canonical uppercase form; update the call in testsupport/metrics/metrics.go (the http.NewRequest invocation that assigns to request) to use "GET" instead of "Get" so it matches the other usage (e.g., the one on line 139) and keeps method casing consistent across the codebase.test/metrics/metrics_test.go (2)
102-107: Unnecessary/incorrect route wait for registration-service subtest.The test waits for the
host-operator-metrics-serviceroute but then useshostAwait.RegistrationServiceMetricsURLwhich is set up from a different route (registration-service-metrics) ininit.go. This wait is redundant sinceWaitForDeploymentsalready ensures the registration service metrics route is available.Suggested fix - remove unnecessary wait
t.Run("registration-service", func(t *testing.T) { - // given hostAwait := awaitilities.Host() - _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") - require.NoError(t, err) t.Run("commit", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 102 - 107, In the "registration-service" subtest remove the redundant call to hostAwait.WaitForRouteToBeAvailable that waits for "host-operator-metrics-service" because the test uses hostAwait.RegistrationServiceMetricsURL (which is derived from the "registration-service-metrics" route in init.go) and WaitForDeployments already guarantees the route is ready; update the subtest to rely on hostAwait.RegistrationServiceMetricsURL directly and delete the WaitForRouteToBeAvailable invocation and its error handling.
38-38: Consider extracting duplicated literals per SonarCloud findings.The string literals
"/metrics"and"host-operator-metrics-service"are duplicated multiple times. Consider defining constants to improve maintainability.Suggested fix
Add constants at the package or file level:
const ( metricsPath = "/metrics" hostOperatorMetricsService = "host-operator-metrics-service" )Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` at line 38, Several tests repeatedly use the same string literals for the metrics path and service name; define package/file-level constants (e.g., metricsPath and hostOperatorMetricsService) and replace all occurrences of "/metrics" and "host-operator-metrics-service" (notably in calls to hostAwait.WaitForRouteToBeAvailable and any other checks around metrics) with those constants to remove duplication and improve maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/init.go`:
- Line 218: The code assumes route.Status.Ingress is non-empty when assigning
initHostAwait.MetricsURL (using hostMetricsRoute.Status.Ingress[0].Host) and
similarly for the Logs and Health assignments; add defensive checks before
indexing each route's Ingress slice (for hostMetricsRoute, hostLogsRoute,
hostHealthRoute) to verify len(route.Status.Ingress) > 0 and call t.Fatalf with
a clear message if empty, then proceed to read Ingress[0].Host for the URL
assignments.
---
Outside diff comments:
In `@test/metrics/metrics_test.go`:
- Around line 74-75: The test mistakenly assigns member2Await using
awaitilities.Member1() instead of awaitilities.Member2(), causing both variables
to refer to the same member; update the assignment of member2Await to call
awaitilities.Member2() so the test exercises two distinct members (change the
second line where member2Await is defined).
---
Nitpick comments:
In `@test/metrics/metrics_test.go`:
- Around line 102-107: In the "registration-service" subtest remove the
redundant call to hostAwait.WaitForRouteToBeAvailable that waits for
"host-operator-metrics-service" because the test uses
hostAwait.RegistrationServiceMetricsURL (which is derived from the
"registration-service-metrics" route in init.go) and WaitForDeployments already
guarantees the route is ready; update the subtest to rely on
hostAwait.RegistrationServiceMetricsURL directly and delete the
WaitForRouteToBeAvailable invocation and its error handling.
- Line 38: Several tests repeatedly use the same string literals for the metrics
path and service name; define package/file-level constants (e.g., metricsPath
and hostOperatorMetricsService) and replace all occurrences of "/metrics" and
"host-operator-metrics-service" (notably in calls to
hostAwait.WaitForRouteToBeAvailable and any other checks around metrics) with
those constants to remove duplication and improve maintainability.
In `@testsupport/init.go`:
- Around line 209-245: Extract the duplicated "/metrics" literal into a single
constant (e.g., const metricsPath = "/metrics") at the top of the function or
file, then replace the three usages in the SetupRouteForService calls for
initHostAwait.SetupRouteForService (host metrics),
initHostAwait.SetupRouteForService (registration service metrics), and
initMemberAwait.SetupRouteForService (member metrics) with that constant to
remove duplication and improve maintainability.
In `@testsupport/metrics/metrics.go`:
- Line 45: The HTTP method string in the http.NewRequest call uses "Get"
inconsistent with the canonical uppercase form; update the call in
testsupport/metrics/metrics.go (the http.NewRequest invocation that assigns to
request) to use "GET" instead of "Get" so it matches the other usage (e.g., the
one on line 139) and keeps method casing consistent across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e50c05b-11a2-4f16-bfe6-dd1d9fd37c0d
📒 Files selected for processing (5)
test/metrics/metrics_test.gotestsupport/init.gotestsupport/metrics/metrics.gotestsupport/wait/awaitility.gotestsupport/wait/host.go
| }, | ||
| ) | ||
| require.NoError(t, err) | ||
| initHostAwait.MetricsURL = "https://" + hostMetricsRoute.Status.Ingress[0].Host |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SetupRouteForService guarantees non-empty Ingress on return
ast-grep --pattern 'func ($_ *Awaitility) SetupRouteForService($$$) ($_, error) {
$$$
}'Repository: codeready-toolchain/toolchain-e2e
Length of output: 2439
🏁 Script executed:
# First, let's locate and examine testsupport/init.go
fd init.go testsupport
# Also check the line count to understand file structure
wc -l testsupport/init.go 2>/dev/null || echo "File not found in expected path"Repository: codeready-toolchain/toolchain-e2e
Length of output: 120
🏁 Script executed:
# Search for files that use SetupRouteForService and access Ingress[0].Host
rg "SetupRouteForService|Ingress\[0\]\.Host" --type-list | head -20Repository: codeready-toolchain/toolchain-e2e
Length of output: 466
🏁 Script executed:
# Find WaitForRouteToBeAvailable implementation
ast-grep --pattern 'func ($_ *Awaitility) WaitForRouteToBeAvailable($$$) ($_, error) {
$$$
}'Repository: codeready-toolchain/toolchain-e2e
Length of output: 4243
🏁 Script executed:
# Examine testsupport/init.go around lines 218, 231, 244
sed -n '210,250p' testsupport/init.goRepository: codeready-toolchain/toolchain-e2e
Length of output: 1936
🏁 Script executed:
# Also check for the specific string from the review comment snippet
rg "initHostAwait.MetricsURL" testsupport/Repository: codeready-toolchain/toolchain-e2e
Length of output: 257
🏁 Script executed:
# Find all places accessing .Status.Ingress[0]
rg "Status\.Ingress\[0\]" testsupport/Repository: codeready-toolchain/toolchain-e2e
Length of output: 772
🏁 Script executed:
# Check all occurrences of accessing Ingress in the codebase
rg "\.Status\.Ingress" --type go -B 2 -A 2 | head -100Repository: codeready-toolchain/toolchain-e2e
Length of output: 2139
Add defensive checks before accessing route Ingress slice.
Lines 218, 231, and 244 access route.Status.Ingress[0].Host without validating that the slice is non-empty. Although SetupRouteForService internally calls WaitForRouteToBeAvailable which verifies the Ingress slice is populated before returning, the code in init.go relies on this implicit guarantee without a defensive check. Add validation before accessing the index to handle any edge cases:
if len(route.Status.Ingress) == 0 {
t.Fatalf("route has no ingress status")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testsupport/init.go` at line 218, The code assumes route.Status.Ingress is
non-empty when assigning initHostAwait.MetricsURL (using
hostMetricsRoute.Status.Ingress[0].Host) and similarly for the Logs and Health
assignments; add defensive checks before indexing each route's Ingress slice
(for hostMetricsRoute, hostLogsRoute, hostHealthRoute) to verify
len(route.Status.Ingress) > 0 and call t.Fatalf with a clear message if empty,
then proceed to read Ingress[0].Host for the URL assignments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testsupport/metrics/metrics.go (1)
138-139: Minor inconsistency: HTTP method casing differs from line 45.Line 45 in
getMetricValueuses"Get"while this function uses"GET". While both work (Go'snet/httpnormalizes the method), consider using the canonical uppercase form consistently across both functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/metrics/metrics.go` around lines 138 - 139, The HTTP method string is inconsistent between the two http.NewRequest calls (one uses "Get" in getMetricValue and this one uses "GET"); update the method argument in getMetricValue to the canonical uppercase "GET" (or change this call to match, but prefer "GET") so both http.NewRequest invocations use "GET" consistently; locate the http.NewRequest calls in getMetricValue and the current function and make the method string identical.test/metrics/metrics_test.go (1)
38-39: Extract duplicated service names and paths to constants.The literals
"host-operator-metrics-service"and"/metrics"are repeated multiple times. Consider extracting them to constants at the top of the file or in a shared location.♻️ Suggested constants
const ( hostOperatorMetricsService = "host-operator-metrics-service" registrationServiceMetricsService = "registration-service-metrics" metricsPath = "/metrics" )Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/metrics/metrics_test.go` around lines 38 - 39, Duplicate string literals for service names and the metrics path in test/metrics/metrics_test.go should be extracted to constants to avoid repetition; add constants (e.g., hostOperatorMetricsService, registrationServiceMetricsService, metricsPath) at the top of the test file and replace all occurrences of "host-operator-metrics-service", "registration-service-metrics" and "/metrics" with those constants where used (for example in calls to hostAwait.WaitForRouteToBeAvailable and similar assertions) so the file references the named constants instead of repeating string literals.testsupport/init.go (1)
209-245: Extract"/metrics"path to a constant to reduce duplication.The literal
"/metrics"is used three times. Extracting it to a constant improves maintainability and satisfies the static analysis hint.♻️ Suggested refactor
Add a constant at the package or function level:
const metricsPath = "/metrics"Then use it in the route setup calls:
- hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics", + hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", metricsPath,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/init.go` around lines 209 - 245, Extract the repeated literal "/metrics" into a single constant (e.g., metricsPath) and use it in all calls to SetupRouteForService to reduce duplication; update the three invocations in this block that call initHostAwait.SetupRouteForService and initMemberAwait.SetupRouteForService (the ones that populate initHostAwait.MetricsURL, initHostAwait.RegistrationServiceMetricsURL, and initMemberAwait.MetricsURL) to pass metricsPath instead of the string literal, and define the constant at the top of the file or function scope where these routes are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/metrics/metrics_test.go`:
- Around line 102-107: The test uses the wrong service name when waiting for the
metrics route: change the call to hostAwait.WaitForRouteToBeAvailable so it
waits for "registration-service-metrics" (the route created in init.go) instead
of "host-operator-metrics-service"; update the invocation in the
"registration-service" t.Run block that currently references hostAwait and
WaitForRouteToBeAvailable so it matches the route used by
hostAwait.RegistrationServiceMetricsURL.
---
Nitpick comments:
In `@test/metrics/metrics_test.go`:
- Around line 38-39: Duplicate string literals for service names and the metrics
path in test/metrics/metrics_test.go should be extracted to constants to avoid
repetition; add constants (e.g., hostOperatorMetricsService,
registrationServiceMetricsService, metricsPath) at the top of the test file and
replace all occurrences of "host-operator-metrics-service",
"registration-service-metrics" and "/metrics" with those constants where used
(for example in calls to hostAwait.WaitForRouteToBeAvailable and similar
assertions) so the file references the named constants instead of repeating
string literals.
In `@testsupport/init.go`:
- Around line 209-245: Extract the repeated literal "/metrics" into a single
constant (e.g., metricsPath) and use it in all calls to SetupRouteForService to
reduce duplication; update the three invocations in this block that call
initHostAwait.SetupRouteForService and initMemberAwait.SetupRouteForService (the
ones that populate initHostAwait.MetricsURL,
initHostAwait.RegistrationServiceMetricsURL, and initMemberAwait.MetricsURL) to
pass metricsPath instead of the string literal, and define the constant at the
top of the file or function scope where these routes are configured.
In `@testsupport/metrics/metrics.go`:
- Around line 138-139: The HTTP method string is inconsistent between the two
http.NewRequest calls (one uses "Get" in getMetricValue and this one uses
"GET"); update the method argument in getMetricValue to the canonical uppercase
"GET" (or change this call to match, but prefer "GET") so both http.NewRequest
invocations use "GET" consistently; locate the http.NewRequest calls in
getMetricValue and the current function and make the method string identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ea3974a-9b41-4ce6-a741-fdd2ccfaf986
📒 Files selected for processing (6)
.govulncheck.yamltest/metrics/metrics_test.gotestsupport/init.gotestsupport/metrics/metrics.gotestsupport/wait/awaitility.gotestsupport/wait/host.go
✅ Files skipped from review due to trivial changes (1)
- .govulncheck.yaml
| t.Run("registration-service", func(t *testing.T) { | ||
|
|
||
| // given | ||
| hostAwait := awaitilities.Host() | ||
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Bug: Wrong service name in route availability check for registration-service metrics.
Line 106 waits for "host-operator-metrics-service" but this test is validating registration-service metrics from hostAwait.RegistrationServiceMetricsURL. The route was set up in init.go as "registration-service-metrics", so the wait should match.
🐛 Proposed fix
t.Run("registration-service", func(t *testing.T) {
// given
hostAwait := awaitilities.Host()
- _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics")
+ _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.RegistrationServiceNs, "registration-service-metrics", "/metrics")
require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("registration-service", func(t *testing.T) { | |
| // given | |
| hostAwait := awaitilities.Host() | |
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | |
| require.NoError(t, err) | |
| t.Run("registration-service", func(t *testing.T) { | |
| // given | |
| hostAwait := awaitilities.Host() | |
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.RegistrationServiceNs, "registration-service-metrics", "/metrics") | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/metrics/metrics_test.go` around lines 102 - 107, The test uses the wrong
service name when waiting for the metrics route: change the call to
hostAwait.WaitForRouteToBeAvailable so it waits for
"registration-service-metrics" (the route created in init.go) instead of
"host-operator-metrics-service"; update the invocation in the
"registration-service" t.Run block that currently references hostAwait and
WaitForRouteToBeAvailable so it matches the route used by
hostAwait.RegistrationServiceMetricsURL.
| registrationServiceMetricsRoute, err := initHostAwait.SetupRouteForService(t, "registration-service-metrics", "/metrics", | ||
| &routev1.RoutePort{ | ||
| TargetPort: intstr.FromString("regsvc-metrics"), | ||
| }, | ||
| &routev1.TLSConfig{ | ||
| Termination: routev1.TLSTerminationEdge, | ||
| }, | ||
| ) | ||
| require.NoError(t, err) | ||
| initHostAwait.RegistrationServiceMetricsURL = "https://" + registrationServiceMetricsRoute.Status.Ingress[0].Host | ||
| t.Logf("registration service metrics URL: %s", initHostAwait.RegistrationServiceMetricsURL) |
There was a problem hiding this comment.
it looks like the route creation is also done in the tests
toolchain-e2e/test/e2e/parallel/registration_service_test.go
Lines 76 to 90 in 629dc7b
There was a problem hiding this comment.
ahh, good catch! I only ran the tests in the test/metrics package so I missed the route creation in test/e2e. Let me refactor that
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
it's just a detail, but you technically don't need this because WaitForRouteToBeAvailable is executed as part of SetupRouteForService
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | |
| require.NoError(t, err) |
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
the same
| _, err := hostAwait.WaitForRouteToBeAvailable(t, hostAwait.Namespace, "host-operator-metrics-service", "/metrics") | |
| require.NoError(t, err) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, xcoulon 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 |
|
|
@xcoulon: 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. |
5 participants
e2e tests for codeready-toolchain/registration-service#588
also, test new
commitandshort_commitmetrics for host and member operatorsSigned-off-by: Xavier Coulon xcoulon@redhat.com
Summary by CodeRabbit
Tests
Chores