MON-1157: Test metrics for prometheus best practice#31370
Conversation
This commit adds a testing for prometheus metric naming best practices with an exception list. The exception list in empty initially and should be filled in a followup. After the initial setup, the list should ideally not grow. The test is informative for now. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jan--f: This pull request references MON-1157 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds Prometheus metric-family helpers, a promlint wrapper with exception filtering, and an extended Ginkgo test that runs the naming lint over live Prometheus metadata. ChangesPrometheus Metric Naming Lint
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f 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
![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 (4)
test/extended/util/prometheus/metrics.go (2)
49-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against empty
Metricslice before indexingMetric[0].
SetCamelCaseLabelsassumes every family has at least oneMetricentry. This holds today sinceMetadataToMetricFamiliesalways appends a single&dto.Metric{}, but the function has no defensive check of its own — if it's ever called with families built differently (or the invariant is broken in a future refactor), this will panic with an index-out-of-range.🛡️ Proposed defensive check
func SetCamelCaseLabels(families []*dto.MetricFamily, labelsPerMetric map[string][]*dto.LabelPair) { for i, family := range families { + if len(families[i].Metric) == 0 { + continue + } labels, ok := labelsPerMetric[family.GetName()] if ok { families[i].Metric[0].Label = labels } } }🤖 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/extended/util/prometheus/metrics.go` around lines 49 - 56, SetCamelCaseLabels currently indexes families[i].Metric[0] without verifying that Metric contains any entries, so add a defensive guard before assigning labels. Update the SetCamelCaseLabels function to check that the matched family has at least one Metric element before touching Metric[0], and only set the Label field when that slice is non-empty. Keep the existing label lookup by family name and preserve the current behavior for valid MetricFamily values.
79-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePromQL query built with raw label name; also
valvariable shadowing reduces readability.Line 79 interpolates
labeldirectly into a PromQL selector viafmt.Sprintf. Sincelabelcomes frompromClient.LabelNamesresults on the live cluster, valid Prometheus label names shouldn't break the query syntax, but this is still string-built query construction worth double-checking against the "no fmt.Sprintf in queries" guidance for query-building code.Separately, at Lines 94-95,
val := ""shadows the outerval(themodel.Valuefrom Line 80'spromClient.Querycall), which is confusing to read even though it's harmless here.♻️ Suggested rename to avoid shadowing
for _, sample := range vector { metricName := string(sample.Metric[model.MetricNameLabel]) if metricName == "" { continue } lbl := label - val := "" + emptyVal := "" result[metricName] = append(result[metricName], &dto.LabelPair{ Name: &lbl, - Value: &val, + Value: &emptyVal, }) }🤖 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/extended/util/prometheus/metrics.go` around lines 79 - 99, The query construction in the metrics helper still uses fmt.Sprintf with a raw label name, so update the PromQL building logic around promClient.Query to avoid string-interpolating the selector directly and follow the no-fmt.Sprintf-in-queries guidance. Also rename the inner val string in the sample loop to avoid shadowing the outer val from promClient.Query, making the code in this metrics aggregation path easier to read and maintain.Source: Path instructions
test/extended/util/prometheus/metrics_test.go (1)
11-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNo unit test for
FindCamelCaseLabels.All other new helpers (
MetadataToMetricFamilies,camelCaseRe,SetCamelCaseLabels,FormatExceptionEntries) have table-driven tests, butFindCamelCaseLabels(which contains the query-building and result-parsing logic) is untested. A mock implementingprometheusv1.APIwould let you cover the camelCase filtering and label-pair construction logic without a live cluster.🤖 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/extended/util/prometheus/metrics_test.go` around lines 11 - 242, Add unit coverage for FindCamelCaseLabels, since its query construction and response parsing are currently untested. Create a table-driven test around FindCamelCaseLabels using a mock prometheusv1.API to verify it issues the camelCase-focused query, filters only matching metrics, and builds the expected label pairs. Keep the test alongside the existing Metrics helpers tests and reference FindCamelCaseLabels plus the label-pair handling used by SetCamelCaseLabels.test/extended/util/prometheus/promlint.go (1)
27-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider embedding upstream
promlint.Probleminstead of redeclaring fields.
Problemduplicatespromlint.Problem'sMetric/Textfields via a plain struct rather than wrapping/embedding the upstream type. Functionally correct as-is (confirmedpromlint.Problemhas these fields), just a minor duplication given the doc comment says it "wraps" the upstream type.🤖 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/extended/util/prometheus/promlint.go` around lines 27 - 41, Update the local Problem type in the prometheus promlint utility to actually wrap the upstream promlint.Problem instead of duplicating its Metric and Text fields. Use embedding or a direct alias-style wrapper so the existing String method and any callers still work with the upstream type, and keep the Linter wrapper unchanged.
🤖 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/extended/util/prometheus/metrics_test.go`:
- Line 169: Update the misleading test comment in the Prometheus metrics tests
so it matches the actual regex behavior: in the `metrics_test.go` case for
`myHTTPHandler`, the `[a-z][A-Z]` match is the `yH` transition, not `PH`. Keep
the assertion as-is and only correct the inline explanation near the
`myHTTPHandler` test case.
---
Nitpick comments:
In `@test/extended/util/prometheus/metrics_test.go`:
- Around line 11-242: Add unit coverage for FindCamelCaseLabels, since its query
construction and response parsing are currently untested. Create a table-driven
test around FindCamelCaseLabels using a mock prometheusv1.API to verify it
issues the camelCase-focused query, filters only matching metrics, and builds
the expected label pairs. Keep the test alongside the existing Metrics helpers
tests and reference FindCamelCaseLabels plus the label-pair handling used by
SetCamelCaseLabels.
In `@test/extended/util/prometheus/metrics.go`:
- Around line 49-56: SetCamelCaseLabels currently indexes families[i].Metric[0]
without verifying that Metric contains any entries, so add a defensive guard
before assigning labels. Update the SetCamelCaseLabels function to check that
the matched family has at least one Metric element before touching Metric[0],
and only set the Label field when that slice is non-empty. Keep the existing
label lookup by family name and preserve the current behavior for valid
MetricFamily values.
- Around line 79-99: The query construction in the metrics helper still uses
fmt.Sprintf with a raw label name, so update the PromQL building logic around
promClient.Query to avoid string-interpolating the selector directly and follow
the no-fmt.Sprintf-in-queries guidance. Also rename the inner val string in the
sample loop to avoid shadowing the outer val from promClient.Query, making the
code in this metrics aggregation path easier to read and maintain.
In `@test/extended/util/prometheus/promlint.go`:
- Around line 27-41: Update the local Problem type in the prometheus promlint
utility to actually wrap the upstream promlint.Problem instead of duplicating
its Metric and Text fields. Use embedding or a direct alias-style wrapper so the
existing String method and any callers still work with the upstream type, and
keep the Linter wrapper unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9992f1dc-639f-439e-9172-8a748ffe71dd
📒 Files selected for processing (4)
test/extended/prometheus/prometheus.gotest/extended/util/prometheus/metrics.gotest/extended/util/prometheus/metrics_test.gotest/extended/util/prometheus/promlint.go
- gofmt: fix import ordering in prometheus.go and alignment in metrics_test.go, fixing the failing ci/prow/verify job - fix misleading comment in TestCamelCaseRegex: the myHTTPHandler case matches the yH transition, not PH - guard SetCamelCaseLabels against MetricFamily values with an empty Metric slice to avoid a future index-out-of-range panic - rename the inner shadowed val variable in FindCamelCaseLabels to emptyVal for clarity Assisted-by: claude-sonnet-5 Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
|
Pushed a follow-up commit addressing review feedback and the failing
Left two nitpicks as-is:
|
openshift-ci
Bot
added
the
ready-for-human-review
|
/pipeline required |
|
Scheduling required tests: |
|
@jan--f: The following tests 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f60e96b
New tests seen in this PR at sha: f60e96b
|
3 participants
This commit adds a testing for prometheus metric naming best practices with an exception list. The exception list in empty initially and should be filled in a followup. After the initial setup, the list should ideally not grow.
The test is informative for now.
Summary by CodeRabbit