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.

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

Add linter helpers to testutil #743

Merged
merged 3 commits into from Apr 27, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
46 changes: 46 additions & 0 deletions 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// pedantic Registry. It then does the same as GatherAndLint, gathering the
// pedantic Registry. It then does the same as GatherAndLint, gathers the

Kind of scared to suggest this to you, but .... am I right here? 😄

Previous version was reading oddly

Copy link
Member Author

Choose a reason for hiding this comment

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

I could argue why my version is correct. But I agree it's convoluted. I'll reword to avoid the complication.

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer len(metricNames) > 0, less surprising.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that the linter will never recognize fewer problems in future versions, but it might recognize more. I changed the comment to clarify that.

Copy link
Member

Choose a reason for hiding this comment

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

Thx!

got = filterMetrics(got, metricNames)
}
return promlint.NewWithMetricFamilies(got).Lint()
}
70 changes: 70 additions & 0 deletions 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.")
}
}
63 changes: 38 additions & 25 deletions prometheus/testutil/promlint/promlint.go
Expand Up @@ -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
Copy link
Member

@bwplotka bwplotka Apr 25, 2020

Choose a reason for hiding this comment

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

I would comment that this is oneof between two. I was bit confused and I can imagine package devs might be as well..

Not relevant to package users

Copy link
Member Author

Choose a reason for hiding this comment

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

It would actually work with both having values, just that you don't have a constructor doing so (but we might have in the future). I'll add a comment explaining that.

mfs []*dto.MetricFamily
}

// A Problem is an issue detected by a Linter.
Expand All @@ -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)...)
}

Expand All @@ -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,
Expand All @@ -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.
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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())

Expand All @@ -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 ':'"))
Expand All @@ -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'"))
Expand All @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions prometheus/testutil/testutil.go
Expand Up @@ -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 (
Expand Down