From 23ddbbaac4f62e11f87209cce85baa5074df991a Mon Sep 17 00:00:00 2001 From: Jan Fajerski Date: Wed, 8 Jul 2026 09:31:08 +0000 Subject: [PATCH 1/2] Test metrics for prometheus best practice 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 Signed-off-by: Jan Fajerski --- test/extended/prometheus/prometheus.go | 43 ++++ test/extended/util/prometheus/metrics.go | 119 +++++++++ test/extended/util/prometheus/metrics_test.go | 242 ++++++++++++++++++ test/extended/util/prometheus/promlint.go | 96 +++++++ 4 files changed, 500 insertions(+) create mode 100644 test/extended/util/prometheus/metrics.go create mode 100644 test/extended/util/prometheus/metrics_test.go create mode 100644 test/extended/util/prometheus/promlint.go diff --git a/test/extended/prometheus/prometheus.go b/test/extended/prometheus/prometheus.go index 19a6ba5b8837..fc9032b4bd8d 100644 --- a/test/extended/prometheus/prometheus.go +++ b/test/extended/prometheus/prometheus.go @@ -40,6 +40,7 @@ import ( "sigs.k8s.io/yaml" configv1 "github.com/openshift/api/config/v1" + ote "github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo" testresult "github.com/openshift/origin/pkg/test/ginkgo/result" exutil "github.com/openshift/origin/test/extended/util" @@ -951,6 +952,48 @@ var _ = g.Describe("[sig-instrumentation] Prometheus [apigroup:image.openshift.i }) }) +var _ = g.Describe("[sig-instrumentation][Late] Prometheus metric naming", func() { + defer g.GinkgoRecover() + oc := exutil.NewCLIWithoutNamespace("prometheus-lint") + + g.It("should follow Prometheus naming best practices", ote.Informing(), func(ctx g.SpecContext) { + promClient := oc.NewPrometheusClient(ctx) + + g.By("fetching all metric metadata from Prometheus") + metadata, err := promClient.Metadata(ctx, "", "") + o.Expect(err).NotTo(o.HaveOccurred(), "failed to query Prometheus metadata") + o.Expect(metadata).NotTo(o.BeEmpty(), "Prometheus metadata should not be empty") + e2e.Logf("fetched metadata for %d metrics", len(metadata)) + + families := helper.MetadataToMetricFamilies(metadata) + e2e.Logf("built %d metric families (recording rules excluded)", len(families)) + + g.By("finding camelCase labels") + camelLabels, err := helper.FindCamelCaseLabels(ctx, promClient) + o.Expect(err).NotTo(o.HaveOccurred(), "failed to query camelCase labels") + if len(camelLabels) > 0 { + e2e.Logf("found camelCase labels on %d metrics", len(camelLabels)) + } + helper.SetCamelCaseLabels(families, camelLabels) + + g.By("linting metrics with promlint") + linter := helper.NewLinterWithMetricFamilies(families) + problems, err := linter.Lint() + o.Expect(err).NotTo(o.HaveOccurred(), "promlint returned an error") + + if len(problems) > 0 { + e2e.Logf("found %d metric naming violations:", len(problems)) + for _, p := range problems { + e2e.Logf(" %s", p) + } + e2e.Logf("to add these to the exception list, use the following entries:\n%s", + helper.FormatExceptionEntries(problems)) + } + o.Expect(problems).To(o.BeEmpty(), + "metrics violating Prometheus naming best practices found; see test log for details and exception list entries") + }) +}) + func all(errs ...error) []error { var result []error for _, err := range errs { diff --git a/test/extended/util/prometheus/metrics.go b/test/extended/util/prometheus/metrics.go new file mode 100644 index 000000000000..9c2d9606b9d4 --- /dev/null +++ b/test/extended/util/prometheus/metrics.go @@ -0,0 +1,119 @@ +package prometheus + +import ( + "context" + "fmt" + "regexp" + "strings" + "time" + + prometheusv1 "github.com/prometheus/client_golang/api/prometheus/v1" + dto "github.com/prometheus/client_model/go" + "github.com/prometheus/common/model" +) + +var camelCaseRe = regexp.MustCompile(`[a-z][A-Z]`) + +// MetadataToMetricFamilies converts Prometheus API metadata into a slice of +// MetricFamily suitable for promlint validation. +func MetadataToMetricFamilies(metadata map[string][]prometheusv1.Metadata) []*dto.MetricFamily { + var families []*dto.MetricFamily + + for name, entries := range metadata { + if len(entries) == 0 { + continue + } + // Recording rules use colons in their names and are not subject to + // metric naming conventions. + if strings.Contains(name, ":") { + continue + } + + entry := entries[0] + name := name + help := entry.Help + metricType := convertMetricType(entry.Type) + families = append(families, &dto.MetricFamily{ + Name: &name, + Help: &help, + Type: &metricType, + Metric: []*dto.Metric{{}}, + }) + } + + return families +} + +// SetCamelCaseLabels attaches camelCase label pairs to the corresponding +// metric families so that promlint's LintCamelCase validation can detect them. +func SetCamelCaseLabels(families []*dto.MetricFamily, labelsPerMetric map[string][]*dto.LabelPair) { + for i, family := range families { + labels, ok := labelsPerMetric[family.GetName()] + if ok { + families[i].Metric[0].Label = labels + } + } +} + +// FindCamelCaseLabels queries Prometheus for all label names that contain +// camelCase, then for each such label, determines which metrics use it. +func FindCamelCaseLabels(ctx context.Context, promClient prometheusv1.API) (map[string][]*dto.LabelPair, error) { + labelNames, _, err := promClient.LabelNames(ctx, nil, time.Time{}, time.Time{}) + if err != nil { + return nil, fmt.Errorf("failed to get label names: %w", err) + } + + var camelCaseLabels []string + for _, label := range labelNames { + if camelCaseRe.MatchString(label) { + camelCaseLabels = append(camelCaseLabels, label) + } + } + + if len(camelCaseLabels) == 0 { + return nil, nil + } + + result := make(map[string][]*dto.LabelPair) + for _, label := range camelCaseLabels { + query := fmt.Sprintf(`count({__name__=~".+",%s=~".+"}) by (__name__)`, label) + val, _, err := promClient.Query(ctx, query, time.Time{}) + if err != nil { + return nil, fmt.Errorf("failed to query metrics for label %q: %w", label, err) + } + + vector, ok := val.(model.Vector) + if !ok { + continue + } + for _, sample := range vector { + metricName := string(sample.Metric[model.MetricNameLabel]) + if metricName == "" { + continue + } + lbl := label + val := "" + result[metricName] = append(result[metricName], &dto.LabelPair{ + Name: &lbl, + Value: &val, + }) + } + } + + return result, nil +} + +func convertMetricType(mt prometheusv1.MetricType) dto.MetricType { + switch mt { + case prometheusv1.MetricTypeCounter: + return dto.MetricType_COUNTER + case prometheusv1.MetricTypeGauge: + return dto.MetricType_GAUGE + case prometheusv1.MetricTypeHistogram: + return dto.MetricType_HISTOGRAM + case prometheusv1.MetricTypeSummary: + return dto.MetricType_SUMMARY + default: + return dto.MetricType_UNTYPED + } +} diff --git a/test/extended/util/prometheus/metrics_test.go b/test/extended/util/prometheus/metrics_test.go new file mode 100644 index 000000000000..9a8ad53ac715 --- /dev/null +++ b/test/extended/util/prometheus/metrics_test.go @@ -0,0 +1,242 @@ +package prometheus + +import ( + "strings" + "testing" + + prometheusv1 "github.com/prometheus/client_golang/api/prometheus/v1" + dto "github.com/prometheus/client_model/go" +) + +func TestMetadataToMetricFamilies(t *testing.T) { + tests := []struct { + name string + metadata map[string][]prometheusv1.Metadata + wantLen int + check func(t *testing.T, families []*dto.MetricFamily) + }{ + { + name: "empty metadata", + metadata: map[string][]prometheusv1.Metadata{}, + wantLen: 0, + }, + { + name: "counter metric", + metadata: map[string][]prometheusv1.Metadata{ + "http_requests_total": {{ + Type: prometheusv1.MetricTypeCounter, + Help: "Total number of HTTP requests.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + f := families[0] + if f.GetName() != "http_requests_total" { + t.Errorf("got name %q, want %q", f.GetName(), "http_requests_total") + } + if f.GetType() != dto.MetricType_COUNTER { + t.Errorf("got type %v, want COUNTER", f.GetType()) + } + if f.GetHelp() != "Total number of HTTP requests." { + t.Errorf("got help %q, want %q", f.GetHelp(), "Total number of HTTP requests.") + } + }, + }, + { + name: "gauge metric", + metadata: map[string][]prometheusv1.Metadata{ + "temperature_celsius": {{ + Type: prometheusv1.MetricTypeGauge, + Help: "Current temperature.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetType() != dto.MetricType_GAUGE { + t.Errorf("got type %v, want GAUGE", families[0].GetType()) + } + }, + }, + { + name: "histogram metric", + metadata: map[string][]prometheusv1.Metadata{ + "request_duration_seconds": {{ + Type: prometheusv1.MetricTypeHistogram, + Help: "Request duration.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetType() != dto.MetricType_HISTOGRAM { + t.Errorf("got type %v, want HISTOGRAM", families[0].GetType()) + } + }, + }, + { + name: "summary metric", + metadata: map[string][]prometheusv1.Metadata{ + "rpc_duration_seconds": {{ + Type: prometheusv1.MetricTypeSummary, + Help: "RPC duration.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetType() != dto.MetricType_SUMMARY { + t.Errorf("got type %v, want SUMMARY", families[0].GetType()) + } + }, + }, + { + name: "unknown type maps to UNTYPED", + metadata: map[string][]prometheusv1.Metadata{ + "some_metric": {{ + Type: "unknown", + Help: "Some metric.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetType() != dto.MetricType_UNTYPED { + t.Errorf("got type %v, want UNTYPED", families[0].GetType()) + } + }, + }, + { + name: "recording rules are filtered out", + metadata: map[string][]prometheusv1.Metadata{ + "namespace:container_cpu_usage:sum": {{ + Type: prometheusv1.MetricTypeGauge, + Help: "Recording rule.", + }}, + "http_requests_total": {{ + Type: prometheusv1.MetricTypeCounter, + Help: "Total HTTP requests.", + }}, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetName() != "http_requests_total" { + t.Errorf("expected recording rule to be filtered, got %q", families[0].GetName()) + } + }, + }, + { + name: "duplicate metadata entries use first", + metadata: map[string][]prometheusv1.Metadata{ + "up": { + {Type: prometheusv1.MetricTypeGauge, Help: "First help."}, + {Type: prometheusv1.MetricTypeGauge, Help: "Second help."}, + }, + }, + wantLen: 1, + check: func(t *testing.T, families []*dto.MetricFamily) { + if families[0].GetHelp() != "First help." { + t.Errorf("got help %q, want first entry", families[0].GetHelp()) + } + }, + }, + { + name: "entry with no metadata is skipped", + metadata: map[string][]prometheusv1.Metadata{ + "empty_metric": {}, + }, + wantLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + families := MetadataToMetricFamilies(tt.metadata) + if len(families) != tt.wantLen { + t.Fatalf("got %d families, want %d", len(families), tt.wantLen) + } + if tt.check != nil { + tt.check(t, families) + } + }) + } +} + +func TestCamelCaseRegex(t *testing.T) { + tests := []struct { + label string + want bool + }{ + {"snake_case", false}, + {"camelCase", true}, + {"CamelCase", true}, // matches the lC transition + {"myHTTPHandler", true}, // matches the PH transition + {"httpCode", true}, + {"__name__", false}, + {"le", false}, + {"quantile", false}, + {"mountPoint", true}, + {"ALL_CAPS", false}, + } + + for _, tt := range tests { + t.Run(tt.label, func(t *testing.T) { + got := camelCaseRe.MatchString(tt.label) + if got != tt.want { + t.Errorf("camelCaseRe.MatchString(%q) = %v, want %v", tt.label, got, tt.want) + } + }) + } +} + +func TestSetCamelCaseLabels(t *testing.T) { + name1 := "metric_a" + name2 := "metric_b" + metricType := dto.MetricType_GAUGE + help := "help" + families := []*dto.MetricFamily{ + {Name: &name1, Type: &metricType, Help: &help, Metric: []*dto.Metric{{}}}, + {Name: &name2, Type: &metricType, Help: &help, Metric: []*dto.Metric{{}}}, + } + + labelName := "camelCase" + labelVal := "" + labelsPerMetric := map[string][]*dto.LabelPair{ + "metric_a": {{Name: &labelName, Value: &labelVal}}, + } + + SetCamelCaseLabels(families, labelsPerMetric) + + if len(families[0].Metric[0].Label) != 1 { + t.Fatalf("metric_a should have 1 label, got %d", len(families[0].Metric[0].Label)) + } + if families[0].Metric[0].Label[0].GetName() != "camelCase" { + t.Errorf("got label name %q, want %q", families[0].Metric[0].Label[0].GetName(), "camelCase") + } + if len(families[1].Metric[0].Label) != 0 { + t.Errorf("metric_b should have 0 labels, got %d", len(families[1].Metric[0].Label)) + } +} + +func TestFormatExceptionEntries(t *testing.T) { + problems := []Problem{ + {Metric: "z_metric", Text: "counter should have _total suffix"}, + {Metric: "a_metric", Text: "use base unit"}, + {Metric: "z_metric", Text: "duplicate entry"}, + } + + got := FormatExceptionEntries(problems) + + if !strings.Contains(got, `"a_metric"`) { + t.Error("expected a_metric in output") + } + if !strings.Contains(got, `"z_metric"`) { + t.Error("expected z_metric in output") + } + // a_metric should appear before z_metric (sorted) + aIdx := strings.Index(got, "a_metric") + zIdx := strings.Index(got, "z_metric") + if aIdx > zIdx { + t.Error("entries should be sorted alphabetically") + } + // z_metric should only appear once (deduped) + if strings.Count(got, "z_metric") != 1 { + t.Error("duplicate metrics should be deduped") + } +} diff --git a/test/extended/util/prometheus/promlint.go b/test/extended/util/prometheus/promlint.go new file mode 100644 index 000000000000..eb64bfe30bbb --- /dev/null +++ b/test/extended/util/prometheus/promlint.go @@ -0,0 +1,96 @@ +package prometheus + +import ( + "fmt" + "sort" + "strings" + + "github.com/prometheus/client_golang/prometheus/testutil/promlint" + dto "github.com/prometheus/client_model/go" + + "k8s.io/apimachinery/pkg/util/sets" +) + +// exceptionMetrics is the set of metric names that are known to violate +// promlint rules. These are pre-existing violations that were present when the +// linting test was introduced. +// +// New metrics MUST NOT be added to this list — fix the violation instead. If a +// metric intentionally deviates from conventions (e.g. node-exporter metrics +// reflecting procfs entries), document the reason in a comment. +// +// 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]() + +// Problem wraps promlint.Problem. +type Problem struct { + Metric string + Text string +} + +func (p Problem) String() string { + return fmt.Sprintf("%s: %s", p.Metric, p.Text) +} + +// Linter wraps promlint.Linter with OpenShift exception filtering. +type Linter struct { + linter *promlint.Linter +} + +// NewLinterWithMetricFamilies creates a Linter from MetricFamily protos. +func NewLinterWithMetricFamilies(families []*dto.MetricFamily) *Linter { + return &Linter{ + linter: promlint.NewWithMetricFamilies(families), + } +} + +// Lint runs promlint and filters out excepted metrics. +func (l *Linter) Lint() ([]Problem, error) { + upstream, err := l.linter.Lint() + if err != nil { + return nil, err + } + + var problems []Problem + for _, p := range upstream { + if exceptionMetrics.Has(p.Metric) { + continue + } + problems = append(problems, Problem{ + Metric: p.Metric, + Text: p.Text, + }) + } + + return problems, nil +} + +// 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 { + seen := sets.New[string]() + type entry struct { + name string + text string + } + var entries []entry + for _, p := range problems { + if seen.Has(p.Metric) { + continue + } + seen.Insert(p.Metric) + entries = append(entries, entry{name: p.Metric, text: p.Text}) + } + sort.Slice(entries, func(i, j int) bool { + return entries[i].name < entries[j].name + }) + + var b strings.Builder + for _, e := range entries { + fmt.Fprintf(&b, "\t%q, // %s\n", e.name, e.text) + } + return b.String() +} From f60e96bafe6fbbbbc7268e739225b6129ff9cd08 Mon Sep 17 00:00:00 2001 From: Jan Fajerski Date: Thu, 9 Jul 2026 12:13:09 +0000 Subject: [PATCH 2/2] fix: address review comments and gofmt failure in promlint test - 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 --- test/extended/prometheus/prometheus.go | 2 +- test/extended/util/prometheus/metrics.go | 7 +++++-- test/extended/util/prometheus/metrics_test.go | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/test/extended/prometheus/prometheus.go b/test/extended/prometheus/prometheus.go index fc9032b4bd8d..5fc0a9e3b16e 100644 --- a/test/extended/prometheus/prometheus.go +++ b/test/extended/prometheus/prometheus.go @@ -39,8 +39,8 @@ import ( admissionapi "k8s.io/pod-security-admission/api" "sigs.k8s.io/yaml" - configv1 "github.com/openshift/api/config/v1" ote "github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo" + configv1 "github.com/openshift/api/config/v1" testresult "github.com/openshift/origin/pkg/test/ginkgo/result" exutil "github.com/openshift/origin/test/extended/util" diff --git a/test/extended/util/prometheus/metrics.go b/test/extended/util/prometheus/metrics.go index 9c2d9606b9d4..39d886a35e71 100644 --- a/test/extended/util/prometheus/metrics.go +++ b/test/extended/util/prometheus/metrics.go @@ -48,6 +48,9 @@ func MetadataToMetricFamilies(metadata map[string][]prometheusv1.Metadata) []*dt // metric families so that promlint's LintCamelCase validation can detect them. 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 @@ -92,10 +95,10 @@ func FindCamelCaseLabels(ctx context.Context, promClient prometheusv1.API) (map[ continue } lbl := label - val := "" + emptyVal := "" result[metricName] = append(result[metricName], &dto.LabelPair{ Name: &lbl, - Value: &val, + Value: &emptyVal, }) } } diff --git a/test/extended/util/prometheus/metrics_test.go b/test/extended/util/prometheus/metrics_test.go index 9a8ad53ac715..b9c158ecdc6e 100644 --- a/test/extended/util/prometheus/metrics_test.go +++ b/test/extended/util/prometheus/metrics_test.go @@ -165,8 +165,8 @@ func TestCamelCaseRegex(t *testing.T) { }{ {"snake_case", false}, {"camelCase", true}, - {"CamelCase", true}, // matches the lC transition - {"myHTTPHandler", true}, // matches the PH transition + {"CamelCase", true}, // matches the lC transition + {"myHTTPHandler", true}, // matches the yH transition {"httpCode", true}, {"__name__", false}, {"le", false},