More promlint rules (#5515)

* Extra promlint rules for Issue 5453
* broaden type name and camelCase checks
* Expand unit test to catch non base units
* Use only celsius as the base unit for temperature
* Remove candela and moles from the unit check

Signed-off-by: David Ellis <ellisda@gmail.com>

util/promlint/promlint.go

@@ -17,6 +17,7 @@ package promlint
 import (
 	"fmt"
 	"io"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -101,6 +102,10 @@ func lint(mf dto.MetricFamily) []Problem {
 		lintMetricUnits,
 		lintCounter,
 		lintHistogramSummaryReserved,
+		lintMetricTypeInName,
+		lintReservedChars,
+		lintCamelCase,
+		lintUnitAbbreviations,
 	}
 
 	var problems []Problem
@@ -205,12 +210,70 @@ func lintHistogramSummaryReserved(mf dto.MetricFamily) []Problem {
 	return problems
 }
 
+// lintMetricTypeInName detects when metric types are included in the metric name.
+func lintMetricTypeInName(mf dto.MetricFamily) []Problem {
+	var problems problems
+	n := strings.ToLower(mf.GetName())
+
+	for i, t := range dto.MetricType_name {
+		if i == int32(dto.MetricType_UNTYPED) {
+			continue
+		}
+
+		typename := strings.ToLower(t)
+		if strings.Contains(n, "_"+typename+"_") || strings.HasSuffix(n, "_"+typename) {
+			problems.Add(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 problems
+	if strings.Contains(mf.GetName(), ":") {
+		problems.Add(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 problems
+	if camelCase.FindString(mf.GetName()) != "" {
+		problems.Add(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.Add(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 problems
+	n := strings.ToLower(mf.GetName())
+	for _, s := range unitAbbreviations {
+		if strings.Contains(n, "_"+s+"_") || strings.HasSuffix(n, "_"+s) {
+			problems.Add(mf, "metric names should not contain abbreviated units")
+		}
+	}
+	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 string, base string, ok bool) {
 	ss := strings.Split(m, "_")
 
-	for _, u := range baseUnits {
+	for unit, base := range units {
 		// Also check for "no prefix".
 		for _, p := range append(unitPrefixes, "") {
 			for _, s := range ss {
@@ -219,8 +282,8 @@ func metricUnits(m string) (unit string, base string, ok bool) {
 				//
 				// As an example, "thermometers" should not match "meters", but
 				// "kilometers" should.
-				if s == p+u {
+				if s == p+unit {
-					return p + u, u, true
+					return p + unit, base, true
 				}
 			}
 		}
@@ -232,17 +295,41 @@ func metricUnits(m string) (unit string, base string, ok bool) {
 // Units and their possible prefixes recognized by this library.  More can be
 // added over time as needed.
 var (
-	baseUnits = []string{
+	// map a unit to the appropriate base unit.
-		"amperes",
+	units = map[string]string{
-		"bytes",
+		// Base units.
-		"candela",
+		"amperes": "amperes",
-		"grams",
+		"bytes":   "bytes",
-		"kelvin", // Both plural and non-plural form allowed.
+		"celsius": "celsius", // Celsius is more common in practice than Kelvin.
-		"kelvins",
+		"grams":   "grams",
-		"meters", // Both American and international spelling permitted.
+		"joules":  "joules",
-		"metres",
+		"meters":  "meters", // Both American and international spelling permitted.
-		"moles",
+		"metres":  "metres",
-		"seconds",
+		"seconds": "seconds",
+		"volts":   "volts",
+
+		// Non base units.
+		// Time.
+		"minutes": "seconds",
+		"hours":   "seconds",
+		"days":    "seconds",
+		"weeks":   "seconds",
+		// Temperature.
+		"kelvin":     "celsius",
+		"kelvins":    "celsius",
+		"fahrenheit": "celsius",
+		"rankine":    "celsius",
+		// Length.
+		"inches": "meters",
+		"yards":  "meters",
+		"miles":  "meters",
+		// Bytes.
+		"bits": "bytes",
+		// Energy.
+		"calories": "joules",
+		// Mass.
+		"pounds": "grams",
+		"ounces": "grams",
 	}
 
 	unitPrefixes = []string{
@@ -265,4 +352,22 @@ var (
 		"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",
+	}
 )

util/promlint/promlint_test.go

@@ -14,6 +14,7 @@
 package promlint_test
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -21,14 +22,16 @@ import (
 	"github.com/prometheus/prometheus/util/promlint"
 )
 
-func TestLintNoHelpText(t *testing.T) {
+type test struct {
-	const msg = "no help text"
-
-	tests := []struct {
 	name     string
 	in       string
 	problems []promlint.Problem
-	}{
+}
+
+func TestLintNoHelpText(t *testing.T) {
+	const msg = "no help text"
+
+	tests := []test{
 		{
 			name: "no help",
 			in: `
@@ -81,22 +84,7 @@ go_goroutines 24
 `,
 		},
 	}
-
+	runTests(t, tests)
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			l := promlint.New(strings.NewReader(tt.in))
-
-			problems, err := l.Lint()
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-
-			if want, got := tt.problems, problems; !reflect.DeepEqual(want, got) {
-				t.Fatalf("unexpected problems:\n- want: %v\n-  got: %v",
-					want, got)
-			}
-		})
-	}
 }
 
 func TestLintMetricUnits(t *testing.T) {
@@ -105,9 +93,83 @@ func TestLintMetricUnits(t *testing.T) {
 		in       string
 		problems []promlint.Problem
 	}{
+		// good cases.
 		{
 			name: "amperes",
 			in: `
+# HELP x_amperes Test metric.
+# TYPE x_amperes untyped
+x_amperes 10
+`,
+		},
+		{
+			name: "bytes",
+			in: `
+# HELP x_bytes Test metric.
+# TYPE x_bytes untyped
+x_bytes 10
+`,
+		},
+		{
+			name: "grams",
+			in: `
+# HELP x_grams Test metric.
+# TYPE x_grams untyped
+x_grams 10
+`,
+		},
+		{
+			name: "celsius",
+			in: `
+# HELP x_celsius Test metric.
+# TYPE x_celsius untyped
+x_celsius 10
+`,
+		},
+		{
+			name: "meters",
+			in: `
+# HELP x_meters Test metric.
+# TYPE x_meters untyped
+x_meters 10
+`,
+		},
+		{
+			name: "metres",
+			in: `
+# HELP x_metres Test metric.
+# TYPE x_metres untyped
+x_metres 10
+`,
+		},
+		{
+			name: "moles",
+			in: `
+# HELP x_moles Test metric.
+# TYPE x_moles untyped
+x_moles 10
+`,
+		},
+		{
+			name: "seconds",
+			in: `
+# HELP x_seconds Test metric.
+# TYPE x_seconds untyped
+x_seconds 10
+`,
+		},
+		{
+			name: "joules",
+			in: `
+# HELP x_joules Test metric.
+# TYPE x_joules untyped
+x_joules 10
+`,
+		},
+		// bad cases.
+		{
+			name: "milliamperes",
+			in: `
 # HELP x_milliamperes Test metric.
 # TYPE x_milliamperes untyped
 x_milliamperes 10
@@ -118,7 +180,7 @@ x_milliamperes 10
 			}},
 		},
 		{
-			name: "bytes",
+			name: "gigabytes",
 			in: `
 # HELP x_gigabytes Test metric.
 # TYPE x_gigabytes untyped
@@ -130,19 +192,7 @@ x_gigabytes 10
 			}},
 		},
 		{
-			name: "candela",
+			name: "kilograms",
-			in: `
-# HELP x_kilocandela Test metric.
-# TYPE x_kilocandela untyped
-x_kilocandela 10
-`,
-			problems: []promlint.Problem{{
-				Metric: "x_kilocandela",
-				Text:   `use base unit "candela" instead of "kilocandela"`,
-			}},
-		},
-		{
-			name: "grams",
 			in: `
 # HELP x_kilograms Test metric.
 # TYPE x_kilograms untyped
@@ -154,31 +204,19 @@ x_kilograms 10
 			}},
 		},
 		{
-			name: "kelvin",
+			name: "nanocelsius",
 			in: `
-# HELP x_nanokelvin Test metric.
+# HELP x_nanocelsius Test metric.
-# TYPE x_nanokelvin untyped
+# TYPE x_nanocelsius untyped
-x_nanokelvin 10
+x_nanocelsius 10
 `,
 			problems: []promlint.Problem{{
-				Metric: "x_nanokelvin",
+				Metric: "x_nanocelsius",
-				Text:   `use base unit "kelvin" instead of "nanokelvin"`,
+				Text:   `use base unit "celsius" instead of "nanocelsius"`,
 			}},
 		},
 		{
-			name: "kelvins",
+			name: "kilometers",
-			in: `
-# HELP x_nanokelvins Test metric.
-# TYPE x_nanokelvins untyped
-x_nanokelvins 10
-`,
-			problems: []promlint.Problem{{
-				Metric: "x_nanokelvins",
-				Text:   `use base unit "kelvins" instead of "nanokelvins"`,
-			}},
-		},
-		{
-			name: "meters",
 			in: `
 # HELP x_kilometers Test metric.
 # TYPE x_kilometers untyped
@@ -190,31 +228,19 @@ x_kilometers 10
 			}},
 		},
 		{
-			name: "metres",
+			name: "picometers",
 			in: `
-# HELP x_kilometres Test metric.
+# HELP x_picometers Test metric.
-# TYPE x_kilometres untyped
+# TYPE x_picometers untyped
-x_kilometres 10
+x_picometers 10
 `,
 			problems: []promlint.Problem{{
-				Metric: "x_kilometres",
+				Metric: "x_picometers",
-				Text:   `use base unit "metres" instead of "kilometres"`,
+				Text:   `use base unit "meters" instead of "picometers"`,
 			}},
 		},
 		{
-			name: "moles",
+			name: "microseconds",
-			in: `
-# HELP x_picomoles Test metric.
-# TYPE x_picomoles untyped
-x_picomoles 10
-`,
-			problems: []promlint.Problem{{
-				Metric: "x_picomoles",
-				Text:   `use base unit "moles" instead of "picomoles"`,
-			}},
-		},
-		{
-			name: "seconds",
 			in: `
 # HELP x_microseconds Test metric.
 # TYPE x_microseconds untyped
@@ -226,12 +252,168 @@ x_microseconds 10
 			}},
 		},
 		{
-			name: "OK",
+			name: "minutes",
 			in: `
-# HELP thermometers_kelvin Test metric with name that looks like "meters".
+# HELP x_minutes Test metric.
-# TYPE thermometers_kelvin untyped
+# TYPE x_minutes untyped
-thermometers_kelvin 0
+x_minutes 10
 `,
+			problems: []promlint.Problem{{
+				Metric: "x_minutes",
+				Text:   `use base unit "seconds" instead of "minutes"`,
+			}},
+		},
+		{
+			name: "hours",
+			in: `
+# HELP x_hours Test metric.
+# TYPE x_hours untyped
+x_hours 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_hours",
+				Text:   `use base unit "seconds" instead of "hours"`,
+			}},
+		},
+		{
+			name: "days",
+			in: `
+# HELP x_days Test metric.
+# TYPE x_days untyped
+x_days 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_days",
+				Text:   `use base unit "seconds" instead of "days"`,
+			}},
+		},
+		{
+			name: "kelvin",
+			in: `
+# HELP x_kelvin Test metric.
+# TYPE x_kelvin untyped
+x_kelvin 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_kelvin",
+				Text:   `use base unit "celsius" instead of "kelvin"`,
+			}},
+		},
+		{
+			name: "kelvins",
+			in: `
+# HELP x_kelvins Test metric.
+# TYPE x_kelvins untyped
+x_kelvins 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_kelvins",
+				Text:   `use base unit "celsius" instead of "kelvins"`,
+			}},
+		},
+		{
+			name: "fahrenheit",
+			in: `
+# HELP thermometers_fahrenheit Test metric.
+# TYPE thermometers_fahrenheit untyped
+thermometers_fahrenheit 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "thermometers_fahrenheit",
+				Text:   `use base unit "celsius" instead of "fahrenheit"`,
+			}},
+		},
+		{
+			name: "rankine",
+			in: `
+# HELP thermometers_rankine Test metric.
+# TYPE thermometers_rankine untyped
+thermometers_rankine 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "thermometers_rankine",
+				Text:   `use base unit "celsius" instead of "rankine"`,
+			}},
+		}, {
+			name: "inches",
+			in: `
+# HELP x_inches Test metric.
+# TYPE x_inches untyped
+x_inches 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_inches",
+				Text:   `use base unit "meters" instead of "inches"`,
+			}},
+		}, {
+			name: "yards",
+			in: `
+# HELP x_yards Test metric.
+# TYPE x_yards untyped
+x_yards 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_yards",
+				Text:   `use base unit "meters" instead of "yards"`,
+			}},
+		}, {
+			name: "miles",
+			in: `
+# HELP x_miles Test metric.
+# TYPE x_miles untyped
+x_miles 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_miles",
+				Text:   `use base unit "meters" instead of "miles"`,
+			}},
+		}, {
+			name: "bits",
+			in: `
+# HELP x_bits Test metric.
+# TYPE x_bits untyped
+x_bits 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_bits",
+				Text:   `use base unit "bytes" instead of "bits"`,
+			}},
+		},
+		{
+			name: "calories",
+			in: `
+# HELP x_calories Test metric.
+# TYPE x_calories untyped
+x_calories 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_calories",
+				Text:   `use base unit "joules" instead of "calories"`,
+			}},
+		},
+		{
+			name: "pounds",
+			in: `
+# HELP x_pounds Test metric.
+# TYPE x_pounds untyped
+x_pounds 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_pounds",
+				Text:   `use base unit "grams" instead of "pounds"`,
+			}},
+		},
+		{
+			name: "ounces",
+			in: `
+# HELP x_ounces Test metric.
+# TYPE x_ounces untyped
+x_ounces 10
+`,
+			problems: []promlint.Problem{{
+				Metric: "x_ounces",
+				Text:   `use base unit "grams" instead of "ounces"`,
+			}},
 		},
 	}
 
@@ -253,11 +435,7 @@ thermometers_kelvin 0
 }
 
 func TestLintCounter(t *testing.T) {
-	tests := []struct {
+	tests := []test{
-		name     string
-		in       string
-		problems []promlint.Problem
-	}{
 		{
 			name: "counter without _total suffix",
 			in: `
@@ -316,29 +494,11 @@ x_bytes 10
 		},
 	}
 
-	for _, tt := range tests {
+	runTests(t, tests)
-		t.Run(tt.name, func(t *testing.T) {
-			l := promlint.New(strings.NewReader(tt.in))
-
-			problems, err := l.Lint()
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-
-			if want, got := tt.problems, problems; !reflect.DeepEqual(want, got) {
-				t.Fatalf("unexpected problems:\n- want: %v\n-  got: %v",
-					want, got)
-			}
-		})
-	}
 }
 
 func TestLintHistogramSummaryReserved(t *testing.T) {
-	tests := []struct {
+	tests := []test{
-		name     string
-		in       string
-		problems []promlint.Problem
-	}{
 		{
 			name: "gauge with _bucket suffix",
 			in: `
@@ -478,7 +638,138 @@ go_gc_duration_seconds_count 5962
 `,
 		},
 	}
+	runTests(t, tests)
+}
 
+func TestLintMetricTypeInName(t *testing.T) {
+	genTest := func(n, t, err string, problems ...promlint.Problem) test {
+		return test{
+			name: fmt.Sprintf("%s with _%s suffix", t, t),
+			in: fmt.Sprintf(`
+# HELP %s Test metric.
+# TYPE %s %s
+%s 10
+`, n, n, t, n),
+			problems: append(problems, promlint.Problem{
+				Metric: n,
+				Text:   fmt.Sprintf(`metric name should not include type '%s'`, err),
+			}),
+		}
+	}
+
+	twoProbTest := genTest("http_requests_counter", "counter", "counter", promlint.Problem{
+		Metric: "http_requests_counter",
+		Text:   `counter metrics should have "_total" suffix`,
+	})
+
+	tests := []test{
+		twoProbTest,
+		genTest("instance_memory_limit_bytes_gauge", "gauge", "gauge"),
+		genTest("request_duration_seconds_summary", "summary", "summary"),
+		genTest("request_duration_seconds_summary", "histogram", "summary"),
+		genTest("request_duration_seconds_histogram", "histogram", "histogram"),
+		genTest("request_duration_seconds_HISTOGRAM", "histogram", "histogram"),
+
+		genTest("instance_memory_limit_gauge_bytes", "gauge", "gauge"),
+	}
+	runTests(t, tests)
+}
+
+func TestLintReservedChars(t *testing.T) {
+	tests := []test{
+		{
+			name: "request_duration::_seconds",
+			in: `
+# HELP request_duration::_seconds Test metric.
+# TYPE request_duration::_seconds histogram
+request_duration::_seconds 10
+`,
+			problems: []promlint.Problem{
+				{
+					Metric: "request_duration::_seconds",
+					Text:   "metric names should not contain ':'",
+				},
+			},
+		},
+	}
+	runTests(t, tests)
+}
+
+func TestLintCamelCase(t *testing.T) {
+	tests := []test{
+		{
+			name: "requestDuration_seconds",
+			in: `
+# HELP requestDuration_seconds Test metric.
+# TYPE requestDuration_seconds histogram
+requestDuration_seconds 10
+`,
+			problems: []promlint.Problem{
+				{
+					Metric: "requestDuration_seconds",
+					Text:   "metric names should be written in 'snake_case' not 'camelCase'",
+				},
+			},
+		},
+		{
+			name: "request_duration_seconds",
+			in: `
+# HELP request_duration_seconds Test metric.
+# TYPE request_duration_seconds histogram
+request_duration_seconds{httpService="foo"} 10
+`,
+			problems: []promlint.Problem{
+				{
+					Metric: "request_duration_seconds",
+					Text:   "label names should be written in 'snake_case' not 'camelCase'",
+				},
+			},
+		},
+	}
+	runTests(t, tests)
+}
+
+func TestLintUnitAbbreviations(t *testing.T) {
+	genTest := func(n string) test {
+		return test{
+			name: fmt.Sprintf("%s with abbreviated unit", n),
+			in: fmt.Sprintf(`
+# HELP %s Test metric.
+# TYPE %s gauge
+%s 10
+`, n, n, n),
+			problems: []promlint.Problem{
+				promlint.Problem{
+					Metric: n,
+					Text:   "metric names should not contain abbreviated units",
+				},
+			},
+		}
+	}
+	tests := []test{
+		genTest("instance_memory_limit_b"),
+		genTest("instance_memory_limit_kb"),
+		genTest("instance_memory_limit_mb"),
+		genTest("instance_memory_limit_MB"),
+		genTest("instance_memory_limit_gb"),
+		genTest("instance_memory_limit_tb"),
+		genTest("instance_memory_limit_pb"),
+
+		genTest("request_duration_s"),
+		genTest("request_duration_ms"),
+		genTest("request_duration_us"),
+		genTest("request_duration_ns"),
+		genTest("request_duration_sec"),
+		genTest("request_sec_duration"),
+		genTest("request_duration_m"),
+		genTest("request_duration_h"),
+		genTest("request_duration_d"),
+	}
+	runTests(t, tests)
+}
+
+func runTests(t *testing.T, tests []test) {
+	t.Helper()
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			l := promlint.New(strings.NewReader(tt.in))