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
|
machine424
left a comment
machine424
left a comment
There was a problem hiding this comment.
lgtm, just some propositions.
400 faulty metrics, the list will be long :)
(not sure how the linter reacts on utf-8, but I don;t know we have any of those for now)
| // To regenerate this list from a cluster run the promlint test with an empty | ||
| // set, collect the failing metric names from the test output, and add them | ||
| // here sorted alphabetically. | ||
| var exceptionMetrics = sets.New[string]() |
There was a problem hiding this comment.
If it's empty, let's remove it for now and add it back when needed. just trying to keep changes to a min for a more convenient human review :)
| // FormatExceptionEntries returns sorted Go source code for the exception set | ||
| // derived from the given problems, suitable for pasting into the | ||
| // exceptionMetrics declaration. | ||
| func FormatExceptionEntries(problems []Problem) string { |
There was a problem hiding this comment.
Do we need to merge this?
We don't epxect the exception list to grow, but leaving this in implies otherwise. we could just use it locally for the follow up to build the initial/final list, and then get rid of it.
And I'm sure every agent can update the list from logs if that's really really needed (because fetched from upstream or whatever).
(Again, just minor nits for better human code readability.)
There was a problem hiding this comment.
no you're right. The intention is just to use it once.
Though it's convenient to have the properly formatted list in the test output
| } | ||
|
|
||
| // Linter wraps promlint.Linter with OpenShift exception filtering. | ||
| type Linter struct { |
There was a problem hiding this comment.
nit: we could just func LintFamilies(families []*dto.MetricFamily) ([]promlint.Problem, error) {
| Text string | ||
| } | ||
|
|
||
| func (p Problem) String() string { |
There was a problem hiding this comment.
nit:
func formatProblem(p promlint.Problem) string {
return fmt.Sprintf("%s: %s", p.Metric, p.Text)
}
only called one we could even inline.
(for less redirections)
| } | ||
| } | ||
|
|
||
| // FindCamelCaseLabels queries Prometheus for all label names that contain |
There was a problem hiding this comment.
| // FindCamelCaseLabels queries Prometheus for all label names that contain | |
| // FindCamelCaseLabels queries Prometheus for all label names that may contain |
| families := helper.MetadataToMetricFamilies(metadata) | ||
| e2e.Logf("built %d metric families (recording rules excluded)", len(families)) | ||
|
|
||
| g.By("finding camelCase labels") |
There was a problem hiding this comment.
When I first read camelCase, I thought it was a standalone check. I later realized that fetching only the camelCase labels is just an optimization because that is all the linter currently checks for. (If new checks are added upstream on labels, the labels won't all be set, but that's not a problem; we can easily adapt the tests once we learn about that)
To be clear, I'm not saying we should populate labels for all metrics right now, as that would be unnecessarily expensive I assume.
But, to make the test easier to read, I suggest we hide the fetching and setting of these camelCase labels inside a util, and just have a helper that prepares/builds the metric families to run the linter on.
| if len(entries) == 0 { | ||
| continue | ||
| } | ||
| // Recording rules use colons in their names and are not subject to |
There was a problem hiding this comment.
I'm afraid there are some recording rules that don't have colons in their names, we'd need to cross-references with GET /api/v1/rules or sth else...
That being said, /api/v1/metadata doesn't return recording rules, so this check isn't needed.
yeah I expected a long list but this is quite something. Also I'm discouraged by the metrics that pop up and question whether there is actually a path to not having a substantial amount of exceptions. After all there are metrics in there with a Perhaps an alternative path forward is to document the list in the Jira ticket and say that we think OCP e2e tests is too late to enforce best practices? |
|
Actually, I'm down to give this a try, that's the only way we'll laern if it's worth it. It'd be great to make the test blocking as fast as possible (and ensure TRT has bandwidth and are in the look for that) once the initial exception lists are available. From there, we'll see if the tests are easy to maintain, or if we just end up lazily expanding the exception lists. If it turns out there is always nothing we can do (because upstream, because they're already doing it like that, because they want utf-8 or sth else), we can always pivot, document the findings, and remove the tests altogether. |
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