Check validity of label values #335
Conversation
so that we can reuse them in other parts of the code, not only as part of a metricVec.
|
Looks good at a first glance. Will do a more detailed review ASAP. |
prometheus/label_values.go
Outdated
|
|
||
| for _, val := range vals { | ||
| if !utf8.ValidString(val) { | ||
| return errors.New(fmt.Sprintf("label value %#v is not valid utf8", val)) |
There was a problem hiding this comment.
golint says: "should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)"
There was a problem hiding this comment.
I think %q is a bit more natural in the format string (has the same effect as %#v for strings, IIRC, but I had to think about it, while %q makes immediate sense to me).
Nit: It's called UTF-8.
prometheus/label_values.go
Outdated
|
|
||
| for name, val := range labels { | ||
| if !utf8.ValidString(val) { | ||
| return errors.New(fmt.Sprintf("label %s: %#v is not valid utf8", name, val)) |
There was a problem hiding this comment.
golint says: "should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)"
There was a problem hiding this comment.
I'd put in "value" somewhere here, e.g. "label %s: value %q is not valid UTF-8"
prometheus/label_values.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateLabels(labels Labels, expectedNumberOfValues int) error { |
There was a problem hiding this comment.
Since this function is also here, let's not call the file label_values.go but just labels.go. And now that you have created this file, how about moving the errInconsistentCardinality here, too? Perhaps even move checkLabelNames, reservedLabelPrefix, and the Labels type here...
There was a problem hiding this comment.
Thinking about it, perhaps this function should be called validateValuesInLabels or something to make clear it's only looking at the values, not at the names. (Which also makes me think there should be perhaps a doc comment next to the call to this function that explains how the label names are checked implicitly, e.g. line 231 in vec.go.)
prometheus/registry.go
Outdated
| @@ -23,6 +23,8 @@ import ( | |||
|
|
|||
| "github.com/golang/protobuf/proto" | |||
|
|
|||
| "unicode/utf8" | |||
There was a problem hiding this comment.
Standard library imports should go into the first block.
|
Thanks for the review! Will come back to this PR soonish and refine it :) |
|
Ok, feedback is implemented. |
prometheus/labels.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateValuesInLabels(vals []string, expectedNumberOfValues int) error { |
There was a problem hiding this comment.
You confused the two functions here. This was called validateLabelValues, which was a fine name. validateValuesInLabels should be the new name of validateLabels above.
prometheus/labels.go
Outdated
|
|
||
| var errInconsistentCardinality = errors.New("inconsistent label cardinality") | ||
|
|
||
| func validateLabels(labels Labels, expectedNumberOfValues int) error { |
There was a problem hiding this comment.
This should be called validateValuesInLabels. See next comment below.
|
Ah gotcha, thanks for the notice! |
|
Cool. Thank you. Merging now… |
This PR fixes #274.
@beorn7 I was not sure whether I should cover all similar implementation with individual unit tests. Maybe have a look first and tell me if you think we need more tests or on different places.
In regards to the
Gathering part, at the moment an invalid label value will make the wholeGatherrun fail. Might it be better to be not so restrictive and only remove the invalid metrics or should it be this strict in your opinion?