From 957bba6f68bad79e4d710a0c0cfa9035b0b4a469 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sat, 19 Aug 2017 22:12:52 +0200 Subject: [PATCH 1/9] add label value validation to GetMetricWith and friends --- prometheus/counter_test.go | 61 ++++++++++++++++++++++++++++++++++++++ prometheus/label_values.go | 1 + prometheus/vec.go | 39 +++++++++++++++++++++--- 3 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 prometheus/label_values.go diff --git a/prometheus/counter_test.go b/prometheus/counter_test.go index 67391a2..fb2868a 100644 --- a/prometheus/counter_test.go +++ b/prometheus/counter_test.go @@ -14,6 +14,7 @@ package prometheus import ( + "fmt" "math" "testing" @@ -56,3 +57,63 @@ func decreaseCounter(c *counter) (err error) { c.Add(-1) return nil } + +func TestCounterVecGetMetricWithInvalidLabelValues(t *testing.T) { + testCases := []struct { + desc string + labels Labels + }{ + { + desc: "non utf8 label value", + labels: Labels{"a": "\xFF"}, + }, + { + desc: "not enough label values", + labels: Labels{}, + }, + { + desc: "too many label values", + labels: Labels{"a": "1", "b": "2"}, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + counterVec := NewCounterVec(CounterOpts{ + Name: "test", + }, []string{"a"}) + + labelValues := make([]string, len(test.labels)) + for _, val := range test.labels { + labelValues = append(labelValues, val) + } + + expectPanic(t, func() { + counterVec.WithLabelValues(labelValues...) + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) + expectPanic(t, func() { + counterVec.With(test.labels) + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) + + if _, err := counterVec.GetMetricWithLabelValues(labelValues...); err == nil { + t.Errorf("GetMetricWithLabelValues: expected error because: %s", test.desc) + } + if _, err := counterVec.GetMetricWith(test.labels); err == nil { + t.Errorf("GetMetricWith: expected error because: %s", test.desc) + } + }) + } +} + +func expectPanic(t *testing.T, op func(), errorMsg string) { + defer func() { + if err := recover(); err == nil { + t.Error(errorMsg) + } + }() + + op() +} diff --git a/prometheus/label_values.go b/prometheus/label_values.go new file mode 100644 index 0000000..7b1b4c0 --- /dev/null +++ b/prometheus/label_values.go @@ -0,0 +1 @@ +package prometheus diff --git a/prometheus/vec.go b/prometheus/vec.go index 40d92b7..1d1b395 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -14,8 +14,10 @@ package prometheus import ( + "errors" "fmt" "sync" + "unicode/utf8" "github.com/prometheus/common/model" ) @@ -206,10 +208,24 @@ func (m *metricVec) Reset() { } } -func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { +func (m *metricVec) validateLabelValues(vals []string) error { if len(vals) != len(m.desc.variableLabels) { - return 0, errInconsistentCardinality + return errInconsistentCardinality } + for _, val := range vals { + if !utf8.ValidString(val) { + return errors.New(fmt.Sprintf("label %q is not valid utf8", val)) + } + } + + return nil +} + +func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { + if err := m.validateLabelValues(vals); err != nil { + return 0, err + } + h := hashNew() for _, val := range vals { h = m.hashAdd(h, val) @@ -218,10 +234,25 @@ func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { return h, nil } -func (m *metricVec) hashLabels(labels Labels) (uint64, error) { +func (m *metricVec) validateLabels(labels Labels) error { if len(labels) != len(m.desc.variableLabels) { - return 0, errInconsistentCardinality + return errInconsistentCardinality } + + for name, val := range labels { + if !utf8.ValidString(val) { + return errors.New(fmt.Sprintf("label %s: %q is not valid utf8", name, val)) + } + } + + return nil +} + +func (m *metricVec) hashLabels(labels Labels) (uint64, error) { + if err := m.validateLabels(labels); err != nil { + return 0, err + } + h := hashNew() for _, label := range m.desc.variableLabels { val, ok := labels[label] From 459e88167e88598d8250263c8a6c2c54a72b34c9 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sat, 19 Aug 2017 22:56:18 +0200 Subject: [PATCH 2/9] extract and refactor label validation functions so that we can reuse them in other parts of the code, not only as part of a metricVec. --- prometheus/label_values.go | 34 ++++++++++++++++++++++++++++++++++ prometheus/vec.go | 33 ++------------------------------- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/prometheus/label_values.go b/prometheus/label_values.go index 7b1b4c0..eff35af 100644 --- a/prometheus/label_values.go +++ b/prometheus/label_values.go @@ -1 +1,35 @@ package prometheus + +import ( + "errors" + "fmt" + "unicode/utf8" +) + +func validateLabelValues(vals []string, expectedNumberOfValues int) error { + if len(vals) != expectedNumberOfValues { + return errInconsistentCardinality + } + + for _, val := range vals { + if !utf8.ValidString(val) { + return errors.New(fmt.Sprintf("label %q is not valid utf8", val)) + } + } + + return nil +} + +func validateLabels(labels Labels, expectedNumberOfValues int) error { + if len(labels) != expectedNumberOfValues { + return errInconsistentCardinality + } + + for name, val := range labels { + if !utf8.ValidString(val) { + return errors.New(fmt.Sprintf("label %s: %q is not valid utf8", name, val)) + } + } + + return nil +} diff --git a/prometheus/vec.go b/prometheus/vec.go index 1d1b395..728faf9 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -14,10 +14,8 @@ package prometheus import ( - "errors" "fmt" "sync" - "unicode/utf8" "github.com/prometheus/common/model" ) @@ -208,21 +206,8 @@ func (m *metricVec) Reset() { } } -func (m *metricVec) validateLabelValues(vals []string) error { - if len(vals) != len(m.desc.variableLabels) { - return errInconsistentCardinality - } - for _, val := range vals { - if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label %q is not valid utf8", val)) - } - } - - return nil -} - func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { - if err := m.validateLabelValues(vals); err != nil { + if err := validateLabelValues(m.desc, vals); err != nil { return 0, err } @@ -234,22 +219,8 @@ func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { return h, nil } -func (m *metricVec) validateLabels(labels Labels) error { - if len(labels) != len(m.desc.variableLabels) { - return errInconsistentCardinality - } - - for name, val := range labels { - if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label %s: %q is not valid utf8", name, val)) - } - } - - return nil -} - func (m *metricVec) hashLabels(labels Labels) (uint64, error) { - if err := m.validateLabels(labels); err != nil { + if err := validateLabels(m.desc, labels); err != nil { return 0, err } From 703c4a9c6facff371d25b4f58a8c6f365d824219 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sat, 19 Aug 2017 22:57:48 +0200 Subject: [PATCH 3/9] add label value validation to NewConstMetric and friends --- prometheus/histogram.go | 4 ++-- prometheus/summary.go | 4 ++-- prometheus/value.go | 4 ++-- prometheus/value_test.go | 48 ++++++++++++++++++++++++++++++++++++++++ prometheus/vec.go | 4 ++-- 5 files changed, 56 insertions(+), 8 deletions(-) create mode 100644 prometheus/value_test.go diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 4e0a9c8..6cc6e68 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -430,8 +430,8 @@ func NewConstHistogram( buckets map[float64]uint64, labelValues ...string, ) (Metric, error) { - if len(desc.variableLabels) != len(labelValues) { - return nil, errInconsistentCardinality + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + return nil, err } return &constHistogram{ desc: desc, diff --git a/prometheus/summary.go b/prometheus/summary.go index 964d8cb..56b0663 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -543,8 +543,8 @@ func NewConstSummary( quantiles map[float64]float64, labelValues ...string, ) (Metric, error) { - if len(desc.variableLabels) != len(labelValues) { - return nil, errInconsistentCardinality + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + return nil, err } return &constSummary{ desc: desc, diff --git a/prometheus/value.go b/prometheus/value.go index ff75ce5..c445f67 100644 --- a/prometheus/value.go +++ b/prometheus/value.go @@ -158,8 +158,8 @@ func (v *valueFunc) Write(out *dto.Metric) error { // the Collect method. NewConstMetric returns an error if the length of // labelValues is not consistent with the variable labels in Desc. func NewConstMetric(desc *Desc, valueType ValueType, value float64, labelValues ...string) (Metric, error) { - if len(desc.variableLabels) != len(labelValues) { - return nil, errInconsistentCardinality + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + return nil, err } return &constMetric{ desc: desc, diff --git a/prometheus/value_test.go b/prometheus/value_test.go new file mode 100644 index 0000000..329d938 --- /dev/null +++ b/prometheus/value_test.go @@ -0,0 +1,48 @@ +package prometheus + +import ( + "fmt" + "testing" +) + +func TestNewConstMetricInvalidLabelValues(t *testing.T) { + testCases := []struct { + desc string + labels Labels + }{ + { + desc: "non utf8 label value", + labels: Labels{"a": "\xFF"}, + }, + { + desc: "not enough label values", + labels: Labels{}, + }, + { + desc: "too many label values", + labels: Labels{"a": "1", "b": "2"}, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + metricDesc := NewDesc( + "sample_value", + "sample value", + []string{"a"}, + Labels{}, + ) + + expectPanic(t, func() { + MustNewConstMetric(metricDesc, CounterValue, 0.3, "\xFF") + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) + + if _, err := NewConstMetric(metricDesc, CounterValue, 0.3, "\xFF"); err == nil { + t.Errorf("NewConstMetric: expected error because: %s", test.desc) + } + }) + } +} diff --git a/prometheus/vec.go b/prometheus/vec.go index 728faf9..4e19995 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -207,7 +207,7 @@ func (m *metricVec) Reset() { } func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { - if err := validateLabelValues(m.desc, vals); err != nil { + if err := validateLabelValues(vals, len(m.desc.variableLabels)); err != nil { return 0, err } @@ -220,7 +220,7 @@ func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { } func (m *metricVec) hashLabels(labels Labels) (uint64, error) { - if err := validateLabels(m.desc, labels); err != nil { + if err := validateLabels(labels, len(m.desc.variableLabels)); err != nil { return 0, err } From 7ee20d77cbb00ce040169993da778d3c85c70dcf Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sat, 19 Aug 2017 23:31:56 +0200 Subject: [PATCH 4/9] validate ConstLabels values in NewDesc --- prometheus/desc.go | 7 +++++++ prometheus/desc_test.go | 17 +++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 prometheus/desc_test.go diff --git a/prometheus/desc.go b/prometheus/desc.go index 1835b16..4c67346 100644 --- a/prometheus/desc.go +++ b/prometheus/desc.go @@ -122,6 +122,12 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * for _, labelName := range labelNames { labelValues = append(labelValues, constLabels[labelName]) } + // Validate the const label values. They can't have a wrong cardinality, so + // use in len(labelValues) as expectedNumberOfValues. + if err := validateLabelValues(labelValues, len(labelValues)); err != nil { + d.err = err + return d + } // Now add the variable label names, but prefix them with something that // cannot be in a regular label name. That prevents matching the label // dimension with a different mix between preset and variable labels. @@ -137,6 +143,7 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * d.err = errors.New("duplicate label names") return d } + vh := hashNew() for _, val := range labelValues { vh = hashAdd(vh, val) diff --git a/prometheus/desc_test.go b/prometheus/desc_test.go new file mode 100644 index 0000000..2f96265 --- /dev/null +++ b/prometheus/desc_test.go @@ -0,0 +1,17 @@ +package prometheus + +import ( + "testing" +) + +func TestNewDescInvalidLabelValues(t *testing.T) { + desc := NewDesc( + "sample_label", + "sample label", + nil, + Labels{"a": "\xFF"}, + ) + if desc.err == nil { + t.Errorf("NewDesc: expected error because: %s", desc.err) + } +} From 685a3c90d4734ccac01cc9bd3bf382f61bd06603 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sun, 20 Aug 2017 00:07:21 +0200 Subject: [PATCH 5/9] fail Gather'ing when label value is not utf8 --- prometheus/registry.go | 8 +++++++ prometheus/registry_test.go | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/prometheus/registry.go b/prometheus/registry.go index 34aa3b3..1e84531 100644 --- a/prometheus/registry.go +++ b/prometheus/registry.go @@ -23,6 +23,8 @@ import ( "github.com/golang/protobuf/proto" + "unicode/utf8" + dto "github.com/prometheus/client_model/go" ) @@ -679,6 +681,12 @@ func checkMetricConsistency( ) } + for _, labelPair := range dtoMetric.GetLabel() { + if !utf8.ValidString(*labelPair.Value) { + return fmt.Errorf("collected metric's label %s is not utf8: %#v", *labelPair.Name, *labelPair.Value) + } + } + // Is the metric unique (i.e. no other metric with the same name and the same label values)? h := hashNew() h = hashAdd(h, metricFamily.GetName()) diff --git a/prometheus/registry_test.go b/prometheus/registry_test.go index d016a15..d136bba 100644 --- a/prometheus/registry_test.go +++ b/prometheus/registry_test.go @@ -209,6 +209,34 @@ metric: < expectedMetricFamilyMergedWithExternalAsProtoCompactText := []byte(`name:"name" help:"docstring" type:COUNTER metric: label: counter: > metric: label: counter: > metric: label: counter: > `) + externalMetricFamilyWithInvalidLabelValue := &dto.MetricFamily{ + Name: proto.String("name"), + Help: proto.String("docstring"), + Type: dto.MetricType_COUNTER.Enum(), + Metric: []*dto.Metric{ + { + Label: []*dto.LabelPair{ + { + Name: proto.String("constname"), + Value: proto.String("\xFF"), + }, + { + Name: proto.String("labelname"), + Value: proto.String("different_val"), + }, + }, + Counter: &dto.Counter{ + Value: proto.Float64(42), + }, + }, + }, + } + + expectedMetricFamilyInvalidLabelValueAsText := []byte(`An error has occurred during metrics gathering: + +collected metric's label constname is not utf8: "\xff" +`) + type output struct { headers map[string]string body []byte @@ -452,6 +480,22 @@ metric: < externalMetricFamilyWithSameName, }, }, + { // 16 + headers: map[string]string{ + "Accept": "application/vnd.google.protobuf;proto=io.prometheus.client.MetricFamily;encoding=compact-text", + }, + out: output{ + headers: map[string]string{ + "Content-Type": `text/plain; charset=utf-8`, + }, + body: expectedMetricFamilyInvalidLabelValueAsText, + }, + collector: metricVec, + externalMF: []*dto.MetricFamily{ + externalMetricFamily, + externalMetricFamilyWithInvalidLabelValue, + }, + }, } for i, scenario := range scenarios { registry := prometheus.NewPedanticRegistry() From 555018f3c987d8e501b2543b8f4d5f7a11f956e0 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sun, 20 Aug 2017 00:53:55 +0200 Subject: [PATCH 6/9] make code compatible with go 1.6 --- prometheus/counter_test.go | 43 +++++++++++++++++--------------------- prometheus/value_test.go | 29 +++++++++++-------------- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/prometheus/counter_test.go b/prometheus/counter_test.go index fb2868a..8d5cd0b 100644 --- a/prometheus/counter_test.go +++ b/prometheus/counter_test.go @@ -78,33 +78,28 @@ func TestCounterVecGetMetricWithInvalidLabelValues(t *testing.T) { } for _, test := range testCases { - test := test - t.Run(test.desc, func(t *testing.T) { - t.Parallel() + counterVec := NewCounterVec(CounterOpts{ + Name: "test", + }, []string{"a"}) - counterVec := NewCounterVec(CounterOpts{ - Name: "test", - }, []string{"a"}) + labelValues := make([]string, len(test.labels)) + for _, val := range test.labels { + labelValues = append(labelValues, val) + } - labelValues := make([]string, len(test.labels)) - for _, val := range test.labels { - labelValues = append(labelValues, val) - } + expectPanic(t, func() { + counterVec.WithLabelValues(labelValues...) + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) + expectPanic(t, func() { + counterVec.With(test.labels) + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) - expectPanic(t, func() { - counterVec.WithLabelValues(labelValues...) - }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) - expectPanic(t, func() { - counterVec.With(test.labels) - }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) - - if _, err := counterVec.GetMetricWithLabelValues(labelValues...); err == nil { - t.Errorf("GetMetricWithLabelValues: expected error because: %s", test.desc) - } - if _, err := counterVec.GetMetricWith(test.labels); err == nil { - t.Errorf("GetMetricWith: expected error because: %s", test.desc) - } - }) + if _, err := counterVec.GetMetricWithLabelValues(labelValues...); err == nil { + t.Errorf("GetMetricWithLabelValues: expected error because: %s", test.desc) + } + if _, err := counterVec.GetMetricWith(test.labels); err == nil { + t.Errorf("GetMetricWith: expected error because: %s", test.desc) + } } } diff --git a/prometheus/value_test.go b/prometheus/value_test.go index 329d938..eed517e 100644 --- a/prometheus/value_test.go +++ b/prometheus/value_test.go @@ -25,24 +25,19 @@ func TestNewConstMetricInvalidLabelValues(t *testing.T) { } for _, test := range testCases { - test := test - t.Run(test.desc, func(t *testing.T) { - t.Parallel() + metricDesc := NewDesc( + "sample_value", + "sample value", + []string{"a"}, + Labels{}, + ) - metricDesc := NewDesc( - "sample_value", - "sample value", - []string{"a"}, - Labels{}, - ) + expectPanic(t, func() { + MustNewConstMetric(metricDesc, CounterValue, 0.3, "\xFF") + }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) - expectPanic(t, func() { - MustNewConstMetric(metricDesc, CounterValue, 0.3, "\xFF") - }, fmt.Sprintf("WithLabelValues: expected panic because: %s", test.desc)) - - if _, err := NewConstMetric(metricDesc, CounterValue, 0.3, "\xFF"); err == nil { - t.Errorf("NewConstMetric: expected error because: %s", test.desc) - } - }) + if _, err := NewConstMetric(metricDesc, CounterValue, 0.3, "\xFF"); err == nil { + t.Errorf("NewConstMetric: expected error because: %s", test.desc) + } } } From 6df742e1324da0b236ba351da34493acd707126b Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Sun, 20 Aug 2017 00:54:11 +0200 Subject: [PATCH 7/9] improve formatting of invalid label value error messages --- prometheus/label_values.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prometheus/label_values.go b/prometheus/label_values.go index eff35af..519290e 100644 --- a/prometheus/label_values.go +++ b/prometheus/label_values.go @@ -13,7 +13,7 @@ func validateLabelValues(vals []string, expectedNumberOfValues int) error { for _, val := range vals { if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label %q is not valid utf8", val)) + return errors.New(fmt.Sprintf("label value %#v is not valid utf8", val)) } } @@ -27,7 +27,7 @@ func validateLabels(labels Labels, expectedNumberOfValues int) error { for name, val := range labels { if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label %s: %q is not valid utf8", name, val)) + return errors.New(fmt.Sprintf("label %s: %#v is not valid utf8", name, val)) } } From 0b8aef084ef71aa52c2a2618658e804cfc330249 Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Fri, 25 Aug 2017 14:50:43 +0200 Subject: [PATCH 8/9] implement review feedback --- prometheus/desc.go | 20 +------------ prometheus/histogram.go | 2 +- prometheus/label_values.go | 35 ----------------------- prometheus/labels.go | 57 ++++++++++++++++++++++++++++++++++++++ prometheus/registry.go | 3 +- prometheus/summary.go | 2 +- prometheus/value.go | 5 +--- prometheus/vec.go | 2 +- 8 files changed, 63 insertions(+), 63 deletions(-) delete mode 100644 prometheus/label_values.go create mode 100644 prometheus/labels.go diff --git a/prometheus/desc.go b/prometheus/desc.go index 4c67346..a6c37e2 100644 --- a/prometheus/desc.go +++ b/prometheus/desc.go @@ -25,19 +25,6 @@ import ( dto "github.com/prometheus/client_model/go" ) -// reservedLabelPrefix is a prefix which is not legal in user-supplied -// label names. -const reservedLabelPrefix = "__" - -// Labels represents a collection of label name -> value mappings. This type is -// commonly used with the With(Labels) and GetMetricWith(Labels) methods of -// metric vector Collectors, e.g.: -// myVec.With(Labels{"code": "404", "method": "GET"}).Add(42) -// -// The other use-case is the specification of constant label pairs in Opts or to -// create a Desc. -type Labels map[string]string - // Desc is the descriptor used by every Prometheus Metric. It is essentially // the immutable meta-data of a Metric. The normal Metric implementations // included in this package manage their Desc under the hood. Users only have to @@ -124,7 +111,7 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * } // Validate the const label values. They can't have a wrong cardinality, so // use in len(labelValues) as expectedNumberOfValues. - if err := validateLabelValues(labelValues, len(labelValues)); err != nil { + if err := validateValuesInLabels(labelValues, len(labelValues)); err != nil { d.err = err return d } @@ -200,8 +187,3 @@ func (d *Desc) String() string { d.variableLabels, ) } - -func checkLabelName(l string) bool { - return model.LabelName(l).IsValid() && - !strings.HasPrefix(l, reservedLabelPrefix) -} diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 6cc6e68..530811d 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -430,7 +430,7 @@ func NewConstHistogram( buckets map[float64]uint64, labelValues ...string, ) (Metric, error) { - if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constHistogram{ diff --git a/prometheus/label_values.go b/prometheus/label_values.go deleted file mode 100644 index 519290e..0000000 --- a/prometheus/label_values.go +++ /dev/null @@ -1,35 +0,0 @@ -package prometheus - -import ( - "errors" - "fmt" - "unicode/utf8" -) - -func validateLabelValues(vals []string, expectedNumberOfValues int) error { - if len(vals) != expectedNumberOfValues { - return errInconsistentCardinality - } - - for _, val := range vals { - if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label value %#v is not valid utf8", val)) - } - } - - return nil -} - -func validateLabels(labels Labels, expectedNumberOfValues int) error { - if len(labels) != expectedNumberOfValues { - return errInconsistentCardinality - } - - for name, val := range labels { - if !utf8.ValidString(val) { - return errors.New(fmt.Sprintf("label %s: %#v is not valid utf8", name, val)) - } - } - - return nil -} diff --git a/prometheus/labels.go b/prometheus/labels.go new file mode 100644 index 0000000..3755a9f --- /dev/null +++ b/prometheus/labels.go @@ -0,0 +1,57 @@ +package prometheus + +import ( + "errors" + "fmt" + "strings" + "unicode/utf8" + + "github.com/prometheus/common/model" +) + +// Labels represents a collection of label name -> value mappings. This type is +// commonly used with the With(Labels) and GetMetricWith(Labels) methods of +// metric vector Collectors, e.g.: +// myVec.With(Labels{"code": "404", "method": "GET"}).Add(42) +// +// The other use-case is the specification of constant label pairs in Opts or to +// create a Desc. +type Labels map[string]string + +// reservedLabelPrefix is a prefix which is not legal in user-supplied +// label names. +const reservedLabelPrefix = "__" + +var errInconsistentCardinality = errors.New("inconsistent label cardinality") + +func validateLabels(labels Labels, expectedNumberOfValues int) error { + if len(labels) != expectedNumberOfValues { + return errInconsistentCardinality + } + + for name, val := range labels { + if !utf8.ValidString(val) { + return fmt.Errorf("label %s: value %q is not valid UTF-8", name, val) + } + } + + return nil +} + +func validateValuesInLabels(vals []string, expectedNumberOfValues int) error { + if len(vals) != expectedNumberOfValues { + return errInconsistentCardinality + } + + for _, val := range vals { + if !utf8.ValidString(val) { + return fmt.Errorf("label value %q is not valid UTF-8", val) + } + } + + return nil +} + +func checkLabelName(l string) bool { + return model.LabelName(l).IsValid() && !strings.HasPrefix(l, reservedLabelPrefix) +} diff --git a/prometheus/registry.go b/prometheus/registry.go index 1e84531..8f2094c 100644 --- a/prometheus/registry.go +++ b/prometheus/registry.go @@ -20,11 +20,10 @@ import ( "os" "sort" "sync" + "unicode/utf8" "github.com/golang/protobuf/proto" - "unicode/utf8" - dto "github.com/prometheus/client_model/go" ) diff --git a/prometheus/summary.go b/prometheus/summary.go index 56b0663..bf69a50 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -543,7 +543,7 @@ func NewConstSummary( quantiles map[float64]float64, labelValues ...string, ) (Metric, error) { - if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constSummary{ diff --git a/prometheus/value.go b/prometheus/value.go index c445f67..b0d7e60 100644 --- a/prometheus/value.go +++ b/prometheus/value.go @@ -14,7 +14,6 @@ package prometheus import ( - "errors" "fmt" "math" "sort" @@ -37,8 +36,6 @@ const ( UntypedValue ) -var errInconsistentCardinality = errors.New("inconsistent label cardinality") - // value is a generic metric for simple values. It implements Metric, Collector, // Counter, Gauge, and Untyped. Its effective type is determined by // ValueType. This is a low-level building block used by the library to back the @@ -158,7 +155,7 @@ func (v *valueFunc) Write(out *dto.Metric) error { // the Collect method. NewConstMetric returns an error if the length of // labelValues is not consistent with the variable labels in Desc. func NewConstMetric(desc *Desc, valueType ValueType, value float64, labelValues ...string) (Metric, error) { - if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { + if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constMetric{ diff --git a/prometheus/vec.go b/prometheus/vec.go index 4e19995..6c483b6 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -207,7 +207,7 @@ func (m *metricVec) Reset() { } func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { - if err := validateLabelValues(vals, len(m.desc.variableLabels)); err != nil { + if err := validateValuesInLabels(vals, len(m.desc.variableLabels)); err != nil { return 0, err } From a956c5fdd6aa66f58fcdce478572fadf960a7dff Mon Sep 17 00:00:00 2001 From: Marco Jantke Date: Fri, 25 Aug 2017 17:58:59 +0200 Subject: [PATCH 9/9] improve validation function naming --- prometheus/desc.go | 2 +- prometheus/histogram.go | 2 +- prometheus/labels.go | 4 ++-- prometheus/summary.go | 2 +- prometheus/value.go | 2 +- prometheus/vec.go | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/prometheus/desc.go b/prometheus/desc.go index a6c37e2..920abc9 100644 --- a/prometheus/desc.go +++ b/prometheus/desc.go @@ -111,7 +111,7 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * } // Validate the const label values. They can't have a wrong cardinality, so // use in len(labelValues) as expectedNumberOfValues. - if err := validateValuesInLabels(labelValues, len(labelValues)); err != nil { + if err := validateLabelValues(labelValues, len(labelValues)); err != nil { d.err = err return d } diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 530811d..6cc6e68 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -430,7 +430,7 @@ func NewConstHistogram( buckets map[float64]uint64, labelValues ...string, ) (Metric, error) { - if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constHistogram{ diff --git a/prometheus/labels.go b/prometheus/labels.go index 3755a9f..2502e37 100644 --- a/prometheus/labels.go +++ b/prometheus/labels.go @@ -24,7 +24,7 @@ const reservedLabelPrefix = "__" var errInconsistentCardinality = errors.New("inconsistent label cardinality") -func validateLabels(labels Labels, expectedNumberOfValues int) error { +func validateValuesInLabels(labels Labels, expectedNumberOfValues int) error { if len(labels) != expectedNumberOfValues { return errInconsistentCardinality } @@ -38,7 +38,7 @@ func validateLabels(labels Labels, expectedNumberOfValues int) error { return nil } -func validateValuesInLabels(vals []string, expectedNumberOfValues int) error { +func validateLabelValues(vals []string, expectedNumberOfValues int) error { if len(vals) != expectedNumberOfValues { return errInconsistentCardinality } diff --git a/prometheus/summary.go b/prometheus/summary.go index bf69a50..56b0663 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -543,7 +543,7 @@ func NewConstSummary( quantiles map[float64]float64, labelValues ...string, ) (Metric, error) { - if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constSummary{ diff --git a/prometheus/value.go b/prometheus/value.go index b0d7e60..4a9cca6 100644 --- a/prometheus/value.go +++ b/prometheus/value.go @@ -155,7 +155,7 @@ func (v *valueFunc) Write(out *dto.Metric) error { // the Collect method. NewConstMetric returns an error if the length of // labelValues is not consistent with the variable labels in Desc. func NewConstMetric(desc *Desc, valueType ValueType, value float64, labelValues ...string) (Metric, error) { - if err := validateValuesInLabels(labelValues, len(desc.variableLabels)); err != nil { + if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } return &constMetric{ diff --git a/prometheus/vec.go b/prometheus/vec.go index 6c483b6..65d13fe 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -207,7 +207,7 @@ func (m *metricVec) Reset() { } func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { - if err := validateValuesInLabels(vals, len(m.desc.variableLabels)); err != nil { + if err := validateLabelValues(vals, len(m.desc.variableLabels)); err != nil { return 0, err } @@ -220,7 +220,7 @@ func (m *metricVec) hashLabelValues(vals []string) (uint64, error) { } func (m *metricVec) hashLabels(labels Labels) (uint64, error) { - if err := validateLabels(labels, len(m.desc.variableLabels)); err != nil { + if err := validateValuesInLabels(labels, len(m.desc.variableLabels)); err != nil { return 0, err }