From 6433bcf819e24b6d1841cfc3a05e688fe6083480 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Fri, 24 Apr 2020 23:42:49 +0200 Subject: [PATCH 1/3] Add the capability to lint MetricFamilies directly Also, change all the `dto.MetricFamily` arguments to pointers to be more consistent with what we do in client_golang in general. Signed-off-by: beorn7 --- prometheus/testutil/promlint/promlint.go | 63 ++++++++++++++---------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/prometheus/testutil/promlint/promlint.go b/prometheus/testutil/promlint/promlint.go index 2e00822..18ca942 100644 --- a/prometheus/testutil/promlint/promlint.go +++ b/prometheus/testutil/promlint/promlint.go @@ -29,7 +29,8 @@ import ( // A Linter is a Prometheus metrics linter. It identifies issues with metric // names, types, and metadata, and reports them to the caller. type Linter struct { - r io.Reader + r io.Reader + mfs []*dto.MetricFamily } // A Problem is an issue detected by a Linter. @@ -42,40 +43,52 @@ type Problem struct { } // newProblem is helper function to create a Problem. -func newProblem(mf dto.MetricFamily, text string) Problem { +func newProblem(mf *dto.MetricFamily, text string) Problem { return Problem{ Metric: mf.GetName(), Text: text, } } -// New creates a new Linter that reads an input stream of Prometheus metrics. -// Only the Prometheus text exposition format is supported. +// New creates a new Linter that reads an input stream of Prometheus metrics in +// the Prometheus text exposition format. func New(r io.Reader) *Linter { return &Linter{ r: r, } } +// NewWithMetricFamilies creates a new Linter that reads from a slice of +// MetricFamily protobuf messages. +func NewWithMetricFamilies(mfs []*dto.MetricFamily) *Linter { + return &Linter{ + mfs: mfs, + } +} + // 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 +// issues found in the metrics stream. The slice is sorted by metric name // and issue description. func (l *Linter) Lint() ([]Problem, error) { - // TODO(mdlayher): support for protobuf exposition format? - d := expfmt.NewDecoder(l.r, expfmt.FmtText) - var problems []Problem - var mf dto.MetricFamily - for { - if err := d.Decode(&mf); err != nil { - if err == io.EOF { - break + if l.r != nil { + d := expfmt.NewDecoder(l.r, expfmt.FmtText) + + mf := &dto.MetricFamily{} + for { + if err := d.Decode(mf); err != nil { + if err == io.EOF { + break + } + + return nil, err } - return nil, err + problems = append(problems, lint(mf)...) } - + } + for _, mf := range l.mfs { problems = append(problems, lint(mf)...) } @@ -91,8 +104,8 @@ 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{ +func lint(mf *dto.MetricFamily) []Problem { + fns := []func(mf *dto.MetricFamily) []Problem{ lintHelp, lintMetricUnits, lintCounter, @@ -113,7 +126,7 @@ func lint(mf dto.MetricFamily) []Problem { } // lintHelp detects issues related to the help text for a metric. -func lintHelp(mf dto.MetricFamily) []Problem { +func lintHelp(mf *dto.MetricFamily) []Problem { var problems []Problem // Expect all metrics to have help text available. @@ -125,7 +138,7 @@ func lintHelp(mf dto.MetricFamily) []Problem { } // lintMetricUnits detects issues with metric unit names. -func lintMetricUnits(mf dto.MetricFamily) []Problem { +func lintMetricUnits(mf *dto.MetricFamily) []Problem { var problems []Problem unit, base, ok := metricUnits(*mf.Name) @@ -146,7 +159,7 @@ func lintMetricUnits(mf dto.MetricFamily) []Problem { // lintCounter detects issues specific to counters, as well as patterns that should // only be used with counters. -func lintCounter(mf dto.MetricFamily) []Problem { +func lintCounter(mf *dto.MetricFamily) []Problem { var problems []Problem isCounter := mf.GetType() == dto.MetricType_COUNTER @@ -165,7 +178,7 @@ func lintCounter(mf dto.MetricFamily) []Problem { // 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 { +func lintHistogramSummaryReserved(mf *dto.MetricFamily) []Problem { // These rules do not apply to untyped metrics. t := mf.GetType() if t == dto.MetricType_UNTYPED { @@ -206,7 +219,7 @@ func lintHistogramSummaryReserved(mf dto.MetricFamily) []Problem { } // lintMetricTypeInName detects when metric types are included in the metric name. -func lintMetricTypeInName(mf dto.MetricFamily) []Problem { +func lintMetricTypeInName(mf *dto.MetricFamily) []Problem { var problems []Problem n := strings.ToLower(mf.GetName()) @@ -224,7 +237,7 @@ func lintMetricTypeInName(mf dto.MetricFamily) []Problem { } // lintReservedChars detects colons in metric names. -func lintReservedChars(mf dto.MetricFamily) []Problem { +func lintReservedChars(mf *dto.MetricFamily) []Problem { var problems []Problem if strings.Contains(mf.GetName(), ":") { problems = append(problems, newProblem(mf, "metric names should not contain ':'")) @@ -235,7 +248,7 @@ func lintReservedChars(mf dto.MetricFamily) []Problem { var camelCase = regexp.MustCompile(`[a-z][A-Z]`) // lintCamelCase detects metric names and label names written in camelCase. -func lintCamelCase(mf dto.MetricFamily) []Problem { +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'")) @@ -252,7 +265,7 @@ func lintCamelCase(mf dto.MetricFamily) []Problem { } // lintUnitAbbreviations detects abbreviated units in the metric name. -func lintUnitAbbreviations(mf dto.MetricFamily) []Problem { +func lintUnitAbbreviations(mf *dto.MetricFamily) []Problem { var problems []Problem n := strings.ToLower(mf.GetName()) for _, s := range unitAbbreviations { From 39dbb24d13f7528d65c323822259c8bf3d09fd84 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Fri, 24 Apr 2020 23:44:21 +0200 Subject: [PATCH 2/3] Add helper functions for linting to testutil Signed-off-by: beorn7 --- prometheus/testutil/lint.go | 46 +++++++++++++++++++++ prometheus/testutil/lint_test.go | 70 ++++++++++++++++++++++++++++++++ prometheus/testutil/testutil.go | 4 ++ 3 files changed, 120 insertions(+) create mode 100644 prometheus/testutil/lint.go create mode 100644 prometheus/testutil/lint_test.go diff --git a/prometheus/testutil/lint.go b/prometheus/testutil/lint.go new file mode 100644 index 0000000..1e006f3 --- /dev/null +++ b/prometheus/testutil/lint.go @@ -0,0 +1,46 @@ +// Copyright 2020 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package testutil + +import ( + "fmt" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil/promlint" +) + +// CollectAndLint registers the provided Collector with a newly created +// pedantic Registry. It then does the same as GatherAndLint, gathering the +// metrics from the pedantic Registry. +func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]promlint.Problem, error) { + reg := prometheus.NewPedanticRegistry() + if err := reg.Register(c); err != nil { + return nil, fmt.Errorf("registering collector failed: %s", err) + } + return GatherAndLint(reg, metricNames...) +} + +// 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) { + got, err := g.Gather() + if err != nil { + return nil, fmt.Errorf("gathering metrics failed: %s", err) + } + if metricNames != nil { + got = filterMetrics(got, metricNames) + } + return promlint.NewWithMetricFamilies(got).Lint() +} diff --git a/prometheus/testutil/lint_test.go b/prometheus/testutil/lint_test.go new file mode 100644 index 0000000..1230cc6 --- /dev/null +++ b/prometheus/testutil/lint_test.go @@ -0,0 +1,70 @@ +// Copyright 2020 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package testutil + +import ( + "testing" + + "github.com/prometheus/client_golang/prometheus" +) + +func TestCollectAndLintGood(t *testing.T) { + cnt := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "some_total", + Help: "A value that represents a counter.", + ConstLabels: prometheus.Labels{ + "label1": "value1", + }, + }, + []string{"foo"}, + ) + cnt.WithLabelValues("bar") + cnt.WithLabelValues("baz") + + problems, err := CollectAndLint(cnt) + if err != nil { + t.Error("Unexpected error:", err) + } + if len(problems) > 0 { + t.Error("Unexpected lint problems:", problems) + } +} + +func TestCollectAndLintBad(t *testing.T) { + cnt := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "someThing_ms", + Help: "A value that represents a counter.", + ConstLabels: prometheus.Labels{ + "label1": "value1", + }, + }, + []string{"fooBar"}, + ) + cnt.WithLabelValues("bar") + cnt.WithLabelValues("baz") + + problems, err := CollectAndLint(cnt) + if err != nil { + t.Error("Unexpected error:", err) + } + if len(problems) < 5 { + // The exact nature of the lint problems found is tested within + // the promlint package itself. Here we only want to make sure + // that the collector successfully hit the linter and got enough + // problems flagged. + t.Error("Not enough lint problems found.") + } +} diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index cb09839..0e32d9d 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -31,6 +31,10 @@ // testing custom prometheus.Collector implementations and in particular whole // exporters, i.e. programs that retrieve telemetry data from a 3rd party source // and convert it into Prometheus metrics. +// +// In a similar pattern, CollectAndLint and GatherAndLint can be used to detect +// metrics that have issues with their name, type, or metadata without being +// necessarily invalid, e.g. a counter with a name missing the “_total” suffix. package testutil import ( From dc79bd60935b46c7a47b0e82d34db3912cca0083 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Sat, 25 Apr 2020 15:59:53 +0200 Subject: [PATCH 3/3] Improve various comments Signed-off-by: beorn7 --- prometheus/testutil/lint.go | 6 +++--- prometheus/testutil/lint_test.go | 5 +++-- prometheus/testutil/promlint/promlint.go | 5 +++++ prometheus/testutil/testutil.go | 4 ++-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/prometheus/testutil/lint.go b/prometheus/testutil/lint.go index 1e006f3..7681877 100644 --- a/prometheus/testutil/lint.go +++ b/prometheus/testutil/lint.go @@ -20,9 +20,9 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil/promlint" ) -// CollectAndLint registers the provided Collector with a newly created -// pedantic Registry. It then does the same as GatherAndLint, gathering the -// metrics from the pedantic Registry. +// 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) { reg := prometheus.NewPedanticRegistry() if err := reg.Register(c); err != nil { diff --git a/prometheus/testutil/lint_test.go b/prometheus/testutil/lint_test.go index 1230cc6..547465d 100644 --- a/prometheus/testutil/lint_test.go +++ b/prometheus/testutil/lint_test.go @@ -63,8 +63,9 @@ func TestCollectAndLintBad(t *testing.T) { if len(problems) < 5 { // The exact nature of the lint problems found is tested within // the promlint package itself. Here we only want to make sure - // that the collector successfully hit the linter and got enough - // problems flagged. + // that the collector successfully hits the linter and that at + // least the five problems that the linter could recognize at + // the time of writing this test are flagged. t.Error("Not enough lint problems found.") } } diff --git a/prometheus/testutil/promlint/promlint.go b/prometheus/testutil/promlint/promlint.go index 18ca942..e48e4d4 100644 --- a/prometheus/testutil/promlint/promlint.go +++ b/prometheus/testutil/promlint/promlint.go @@ -29,6 +29,11 @@ import ( // A Linter is a Prometheus metrics linter. It identifies issues with metric // names, types, and metadata, and reports them to the caller. type Linter struct { + // The linter will read metrics in the Prometheus text format from r and + // then lint it, _and_ it will lint the metrics provided directly as + // MetricFamily proto messages in mfs. Note, however, that the current + // constructor functions New and NewWithMetricFamilies only ever set one + // of them. r io.Reader mfs []*dto.MetricFamily } diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go index 0e32d9d..c47373c 100644 --- a/prometheus/testutil/testutil.go +++ b/prometheus/testutil/testutil.go @@ -140,8 +140,8 @@ func CollectAndCount(c prometheus.Collector) int { } // CollectAndCompare registers the provided Collector with a newly created -// pedantic Registry. It then does the same as GatherAndCompare, gathering the -// metrics from the pedantic Registry. +// pedantic Registry. It then calls GatherAndCompare with that Registry and with +// the provided metricNames. func CollectAndCompare(c prometheus.Collector, expected io.Reader, metricNames ...string) error { reg := prometheus.NewPedanticRegistry() if err := reg.Register(c); err != nil {