Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions test/extended/prometheus/prometheus.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 {
Expand Down
119 changes: 119 additions & 0 deletions test/extended/util/prometheus/metrics.go
Original file line number Diff line number Diff line change
@@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FindCamelCaseLabels queries Prometheus for all label names that contain
// FindCamelCaseLabels queries Prometheus for all label names that may 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
}
}
Loading