Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for custom validations in promlint #1311

Merged
merged 3 commits into from Oct 10, 2023
Merged

Add support for custom validations in promlint #1311

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c39535e
Refactor promlint validation structure
machadovilaca Jul 13, 2023
83b0350
Add support for custom validations in promlint
machadovilaca Jul 13, 2023
adc08b1
Keep backwards compatibility
machadovilaca Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions prometheus/testutil/lint.go
View file
Open in desktop
Expand Up @@ -18,12 +18,13 @@ import (

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/testutil/promlint"
"github.com/prometheus/client_golang/prometheus/testutil/promlint/validations"
)

// CollectAndLint registers the provided Collector with a newly created pedantic
// Registry. It then calls GatherAndLint with that Registry and with the
// provided metricNames.
func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]promlint.Problem, error) {
func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]validations.Problem, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid this?

client_golang is 1.0 and generally we can't break compatibility.

testutil might be experimental (?) - not sure if we ever mentioned it's compatibility level, but if we can-- let's not break the compatibility here. Let's move Problem back to promlint and work from there (perhaps that might require validations to be promlint package).

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concerns, I will update it and see if I can get a clean solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bwplotka updated

reg := prometheus.NewPedanticRegistry()
if err := reg.Register(c); err != nil {
return nil, fmt.Errorf("registering collector failed: %w", err)
Expand All @@ -34,7 +35,7 @@ func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]promlint.P
// GatherAndLint gathers all metrics from the provided Gatherer and checks them
// with the linter in the promlint package. If any metricNames are provided,
// only metrics with those names are checked.
func GatherAndLint(g prometheus.Gatherer, metricNames ...string) ([]promlint.Problem, error) {
func GatherAndLint(g prometheus.Gatherer, metricNames ...string) ([]validations.Problem, error) {
got, err := g.Gather()
if err != nil {
return nil, fmt.Errorf("gathering metrics failed: %w", err)
Expand Down
310 changes: 22 additions & 288 deletions prometheus/testutil/promlint/promlint.go
View file
Open in desktop
Expand Up @@ -16,15 +16,13 @@ package promlint

import (
"errors"
"fmt"
"io"
"regexp"
"sort"
"strings"

dto "github.com/prometheus/client_model/go"
"github.com/prometheus/common/expfmt"

dto "github.com/prometheus/client_model/go"
"github.com/prometheus/client_golang/prometheus/testutil/promlint/validations"
)

// A Linter is a Prometheus metrics linter. It identifies issues with metric
Expand All @@ -37,23 +35,8 @@ type Linter struct {
// of them.
r io.Reader
mfs []*dto.MetricFamily
}

// A Problem is an issue detected by a Linter.
type Problem struct {
// The name of the metric indicated by this Problem.
Metric string

// A description of the issue for this Problem.
Text string
}

// newProblem is helper function to create a Problem.
func newProblem(mf *dto.MetricFamily, text string) Problem {
return Problem{
Metric: mf.GetName(),
Text: text,
}
customValidations []validations.Validation
}

// New creates a new Linter that reads an input stream of Prometheus metrics in
Expand All @@ -72,11 +55,19 @@ func NewWithMetricFamilies(mfs []*dto.MetricFamily) *Linter {
}
}

// AddCustomValidations adds custom validations to the linter.
func (l *Linter) AddCustomValidations(vs ...validations.Validation) {
if l.customValidations == nil {
l.customValidations = make([]validations.Validation, 0, len(vs))
}
l.customValidations = append(l.customValidations, vs...)
}

// Lint performs a linting pass, returning a slice of Problems indicating any
// issues found in the metrics stream. The slice is sorted by metric name
// and issue description.
func (l *Linter) Lint() ([]Problem, error) {
var problems []Problem
func (l *Linter) Lint() ([]validations.Problem, error) {
var problems []validations.Problem

if l.r != nil {
d := expfmt.NewDecoder(l.r, expfmt.FmtText)
Expand All @@ -91,11 +82,11 @@ func (l *Linter) Lint() ([]Problem, error) {
return nil, err
}

problems = append(problems, lint(mf)...)
problems = append(problems, l.lint(mf)...)
}
}
for _, mf := range l.mfs {
problems = append(problems, lint(mf)...)
problems = append(problems, l.lint(mf)...)
}

// Ensure deterministic output.
Expand All @@ -110,276 +101,19 @@ func (l *Linter) Lint() ([]Problem, error) {
}

// lint is the entry point for linting a single metric.
func lint(mf *dto.MetricFamily) []Problem {
fns := []func(mf *dto.MetricFamily) []Problem{
lintHelp,
lintMetricUnits,
lintCounter,
lintHistogramSummaryReserved,
lintMetricTypeInName,
lintReservedChars,
lintCamelCase,
lintUnitAbbreviations,
}
func (l *Linter) lint(mf *dto.MetricFamily) []validations.Problem {
var problems []validations.Problem

var problems []Problem
for _, fn := range fns {
for _, fn := range validations.DefaultValidations {
problems = append(problems, fn(mf)...)
}

// TODO(mdlayher): lint rules for specific metrics types.
return problems
}

// lintHelp detects issues related to the help text for a metric.
func lintHelp(mf *dto.MetricFamily) []Problem {
var problems []Problem

// Expect all metrics to have help text available.
if mf.Help == nil {
problems = append(problems, newProblem(mf, "no help text"))
}

return problems
}

// lintMetricUnits detects issues with metric unit names.
func lintMetricUnits(mf *dto.MetricFamily) []Problem {
var problems []Problem

unit, base, ok := metricUnits(*mf.Name)
if !ok {
// No known units detected.
return nil
}

// Unit is already a base unit.
if unit == base {
return nil
}

problems = append(problems, newProblem(mf, fmt.Sprintf("use base unit %q instead of %q", base, unit)))

return problems
}

// lintCounter detects issues specific to counters, as well as patterns that should
// only be used with counters.
func lintCounter(mf *dto.MetricFamily) []Problem {
var problems []Problem

isCounter := mf.GetType() == dto.MetricType_COUNTER
isUntyped := mf.GetType() == dto.MetricType_UNTYPED
hasTotalSuffix := strings.HasSuffix(mf.GetName(), "_total")

switch {
case isCounter && !hasTotalSuffix:
problems = append(problems, newProblem(mf, `counter metrics should have "_total" suffix`))
case !isUntyped && !isCounter && hasTotalSuffix:
problems = append(problems, newProblem(mf, `non-counter metrics should not have "_total" suffix`))
}

return problems
}

// lintHistogramSummaryReserved detects when other types of metrics use names or labels
// reserved for use by histograms and/or summaries.
func lintHistogramSummaryReserved(mf *dto.MetricFamily) []Problem {
// These rules do not apply to untyped metrics.
t := mf.GetType()
if t == dto.MetricType_UNTYPED {
return nil
}

var problems []Problem

isHistogram := t == dto.MetricType_HISTOGRAM
isSummary := t == dto.MetricType_SUMMARY

n := mf.GetName()

if !isHistogram && strings.HasSuffix(n, "_bucket") {
problems = append(problems, newProblem(mf, `non-histogram metrics should not have "_bucket" suffix`))
}
if !isHistogram && !isSummary && strings.HasSuffix(n, "_count") {
problems = append(problems, newProblem(mf, `non-histogram and non-summary metrics should not have "_count" suffix`))
}
if !isHistogram && !isSummary && strings.HasSuffix(n, "_sum") {
problems = append(problems, newProblem(mf, `non-histogram and non-summary metrics should not have "_sum" suffix`))
}

for _, m := range mf.GetMetric() {
for _, l := range m.GetLabel() {
ln := l.GetName()

if !isHistogram && ln == "le" {
problems = append(problems, newProblem(mf, `non-histogram metrics should not have "le" label`))
}
if !isSummary && ln == "quantile" {
problems = append(problems, newProblem(mf, `non-summary metrics should not have "quantile" label`))
}
}
}

return problems
}

// lintMetricTypeInName detects when metric types are included in the metric name.
func lintMetricTypeInName(mf *dto.MetricFamily) []Problem {
var problems []Problem
n := strings.ToLower(mf.GetName())

for i, t := range dto.MetricType_name {
if i == int32(dto.MetricType_UNTYPED) {
continue
if l.customValidations != nil {
for _, fn := range l.customValidations {
problems = append(problems, fn(mf)...)
}

typename := strings.ToLower(t)
if strings.Contains(n, "_"+typename+"_") || strings.HasSuffix(n, "_"+typename) {
problems = append(problems, newProblem(mf, fmt.Sprintf(`metric name should not include type '%s'`, typename)))
}
}
return problems
}

// lintReservedChars detects colons in metric names.
func lintReservedChars(mf *dto.MetricFamily) []Problem {
var problems []Problem
if strings.Contains(mf.GetName(), ":") {
problems = append(problems, newProblem(mf, "metric names should not contain ':'"))
}
return problems
}

var camelCase = regexp.MustCompile(`[a-z][A-Z]`)

// lintCamelCase detects metric names and label names written in camelCase.
func lintCamelCase(mf *dto.MetricFamily) []Problem {
var problems []Problem
if camelCase.FindString(mf.GetName()) != "" {
problems = append(problems, newProblem(mf, "metric names should be written in 'snake_case' not 'camelCase'"))
}

for _, m := range mf.GetMetric() {
for _, l := range m.GetLabel() {
if camelCase.FindString(l.GetName()) != "" {
problems = append(problems, newProblem(mf, "label names should be written in 'snake_case' not 'camelCase'"))
}
}
}
return problems
}

// lintUnitAbbreviations detects abbreviated units in the metric name.
func lintUnitAbbreviations(mf *dto.MetricFamily) []Problem {
var problems []Problem
n := strings.ToLower(mf.GetName())
for _, s := range unitAbbreviations {
if strings.Contains(n, "_"+s+"_") || strings.HasSuffix(n, "_"+s) {
problems = append(problems, newProblem(mf, "metric names should not contain abbreviated units"))
}
}
// TODO(mdlayher): lint rules for specific metrics types.
return problems
}

// metricUnits attempts to detect known unit types used as part of a metric name,
// e.g. "foo_bytes_total" or "bar_baz_milligrams".
func metricUnits(m string) (unit, base string, ok bool) {
ss := strings.Split(m, "_")

for _, s := range ss {
if base, found := units[s]; found {
return s, base, true
}

for _, p := range unitPrefixes {
if strings.HasPrefix(s, p) {
if base, found := units[s[len(p):]]; found {
return s, base, true
}
}
}
}

return "", "", false
}

// Units and their possible prefixes recognized by this library. More can be
// added over time as needed.
var (
// map a unit to the appropriate base unit.
units = map[string]string{
// Base units.
"amperes": "amperes",
"bytes": "bytes",
"celsius": "celsius", // Also allow Celsius because it is common in typical Prometheus use cases.
"grams": "grams",
"joules": "joules",
"kelvin": "kelvin", // SI base unit, used in special cases (e.g. color temperature, scientific measurements).
"meters": "meters", // Both American and international spelling permitted.
"metres": "metres",
"seconds": "seconds",
"volts": "volts",

// Non base units.
// Time.
"minutes": "seconds",
"hours": "seconds",
"days": "seconds",
"weeks": "seconds",
// Temperature.
"kelvins": "kelvin",
"fahrenheit": "celsius",
"rankine": "celsius",
// Length.
"inches": "meters",
"yards": "meters",
"miles": "meters",
// Bytes.
"bits": "bytes",
// Energy.
"calories": "joules",
// Mass.
"pounds": "grams",
"ounces": "grams",
}

unitPrefixes = []string{
"pico",
"nano",
"micro",
"milli",
"centi",
"deci",
"deca",
"hecto",
"kilo",
"kibi",
"mega",
"mibi",
"giga",
"gibi",
"tera",
"tebi",
"peta",
"pebi",
}

// Common abbreviations that we'd like to discourage.
unitAbbreviations = []string{
"s",
"ms",
"us",
"ns",
"sec",
"b",
"kb",
"mb",
"gb",
"tb",
"pb",
"m",
"h",
"d",
}
)