Ensure synthetic metric query format

[coleenquadros]

When perses is querying for labelname values for the metric acm_label_names, the query is of the format api/v1/label/label_name/values?match[]=acm_label_names{} when we dont provide any filters and we do not return any result since we are strictly checking if slices.Contains(matchers, proxyconfig.RBACProxyLabelMetricName) because of which the dashboard variables dont render.

Ive requested for a fix upstream but not sure if it will go through -> perses/plugins#611

Summary by CodeRabbit

• Bug Fixes

  • Improved routing for label-value queries so requests using Grafana-style match[] either with the bare metric name or with a "{}" suffix are correctly recognized and return label results.

• Tests

  • Added test coverage validating label-value queries when match[] uses the metric name with "{}" as well as the bare metric name.

[coderabbitai]

📝 Walkthrough

Walkthrough

isACMLabelQuery now recognizes Grafana match[] values that are either the bare RBACProxyLabelMetricName or RBACProxyLabelMetricName{} (empty label set), and a test was added to cover the {} form for the /api/v1/label/<name>/values endpoint.

Changes

Cohort / File(s)	Summary
RBAC Label Metric Query Handler proxy/pkg/proxy/proxy.go, proxy/pkg/proxy/proxy_test.go	Broadened matcher detection to accept both RBACProxyLabelMetricName and RBACProxyLabelMetricName + "{}" in Grafana match[] parameters; added a positive test case asserting handling of the {} form for /api/v1/label/<name>/values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

The proxy learned a twinned request,
bare name or braces — both are blessed.
Tests sit tidy, assertions clear,
one small tweak, less brittle gear.
Merge on green — we ship with cheer. 🔍🚨✅

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the core change: broadening the metric query detection to handle the synthetic metric format (acm_label_names{}) in addition to the bare metric name.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

proxy/pkg/proxy/proxy_test.go (1)

349-364: ⚠️ Potential issue | 🟡 Minor

Duplicate test case names — two tests walking around with the same badge. 🚨

Lines 350 and 358 both have the name "should handle GET to label values endpoint with correct metric". When one of these fails, you'll be playing detective to figure out which test actually broke. The subtest framework uses these names for identification, so this is like having two functions named main().

🔧 Proposed fix: Differentiate the test names

 		{
-			name:             "should handle GET to label values endpoint with correct metric",
+			name:             "should handle GET to label values endpoint with metric selector syntax",
 			method:           "GET",
 			path:             "/api/v1/label/label_name/values?match[]=" + config.RBACProxyLabelMetricName + "{}",
 			body:             nil,
 			expectedToHandle: true,
 			expectedBody:     `{"status":"success","data":["cloud","cluster_open_cluster_management_io_clusterset","name","vendor"]}`,
 		},
 		{
-			name:             "should handle GET to label values endpoint with correct metric",
+			name:             "should handle GET to label values endpoint with bare metric name",
 			method:           "GET",
 			path:             "/api/v1/label/label_name/values?match[]=" + config.RBACProxyLabelMetricName,
 			body:             nil,
 			expectedToHandle: true,
 			expectedBody:     `{"status":"success","data":["cloud","cluster_open_cluster_management_io_clusterset","name","vendor"]}`,
 		},

As per coding guidelines: "Focus on test coverage and test quality" — unique names ensure each test is clearly identifiable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@proxy/pkg/proxy/proxy_test.go` around lines 349 - 364, Two subtests share the
identical name "should handle GET to label values endpoint with correct metric"
which makes failures ambiguous; locate the two test table entries that build
paths with match[]=" + config.RBACProxyLabelMetricName + "{}" and match[]=" +
config.RBACProxyLabelMetricName and give them distinct names (for example append
" with braces" and " without braces" or include the path variant) so each
subtest (the entries that set path using config.RBACProxyLabelMetricName and
config.RBACProxyLabelMetricName + "{}") is uniquely identifiable.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@proxy/pkg/proxy/proxy_test.go`:
- Around line 349-364: Two subtests share the identical name "should handle GET
to label values endpoint with correct metric" which makes failures ambiguous;
locate the two test table entries that build paths with match[]=" +
config.RBACProxyLabelMetricName + "{}" and match[]=" +
config.RBACProxyLabelMetricName and give them distinct names (for example append
" with braces" and " without braces" or include the path variant) so each
subtest (the entries that set path using config.RBACProxyLabelMetricName and
config.RBACProxyLabelMetricName + "{}") is uniquely identifiable.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a776c319-19be-4455-a628-564d07b15a85

📥 Commits

Reviewing files that changed from the base of the PR and between e75d154 and a087990.

📒 Files selected for processing (2)

• proxy/pkg/proxy/proxy.go
• proxy/pkg/proxy/proxy_test.go

[coleenquadros]

/retest-required

[openshift-ci]

@coleenquadros: 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/e2e-kind	1f79a94	link	true	/test e2e-kind
ci/prow/test-e2e	1f79a94	link	true	/test test-e2e

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.

[thibaultmg]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coleenquadros, thibaultmg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [coleenquadros,thibaultmg]

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