UTF-8 support in validation, and some parsers and formatters #537
Conversation
ywwg
force-pushed
the
owilliams/quoted-metric-name
branch
2 times, most recently
from
7ea4b4c
to
e2602ac
Compare
|
I'll try to get to this ASAP. @ywwg I assume this is ready for review and will change the title accordingly. @roidelapluie I plan to do a code level review, but eventually, we'll need your approval, too. |
There was a problem hiding this comment.
Thanks for doing this.
As discussed, the package-level "config variable" might be the least bad way of "flipping the switch". However, I'm not sure if just one switch is enough for our use case. I see three modes:
- Use this library in exactly the old way, i.e. this PR shouldn't change the behavior at all.
- Use this library in the new way, i.e. all UTF-8 characters are allowed in names, and new exposition formats are used accordingly.
- Allow the new way but still accommodate legacy clients, i.e. special UTF-8 characters need to be escaped with the schema from the design doc.
It seems this PR fulfills (1) and (2), by setting NameValidationScheme accordingly. However, the expfmt package also implements content negotiation, and this has to be modified to allow mode (3).
In different news, text_parse.go eventually needs an update to parse the new classic text format. In Prometheus, we use a different parser implementation, so it's not in the critical path to make Prometheus work, but there should be at least a TODO somewhere.
|
About the |
I believe this is covered with the existence of the IsValidLegacyMetricName function -- the library can be enabled for mode 2 in general, and then specific calls can apply that function when needed. Ah, I missed text_parse.go! I will put that on the list |
I was more thinking about the code in expfmt/encode.go and expfmt/expfmt.go. We need new Content-Type strings for "format XYZ but with arbitrary UTF-8 characters allowed in names" (see design doc). And then the |
|
ah ok. I'll take a look |
|
OK so the question is how to select the escaping mode for old recipients, and that mode might be different for different situations. I think we'll use a similar pattern to the parsing situation: a library-wide setting for default escaping, and then a new function where users can specify the scaping. So the current MetricFamilyToOpenMetrics and MetricFamilyToText functions will use the library-default escaping, and then I'll make a couple new functions that take an extra parameter to specify an escaping on a call-by-call basis. |
|
It might make more sense to shrink this PR down so it just handles the parsing, and then do another one to handing the exposition format creation, what do you think? Otherwise it might start to get large |
|
You can divide things as you see fit. Note that parsing isn't really urgent because the parser here in prometheus/common isn't even used by prometheus/prometheus. (I'm not even sure where it is used at all.) In contrast, the code path in expfmt/encode.go is critical for prometheus/client_golang because this is how the format is selected. |
|
open question: who is in charge of which escaping mechanism gets used? should that be part of the content negotiation, like the scraper says "I wish to have things escaped this way" or is that a setting on the metrics producer? |
|
converting to draft while I do some work |
The MVP here is that any UTF-8-enabled binary instrumented with prometheus/client_golang should still be able to be scraped by a non-UTF-8-enabled scraper. I would assume that means to use the Allowing other escaping schemas would IMHO be "nice to have" (so that users can pick them if they need them for compatibility with legacy systems). Those other escaping schemas already exist, but they have no support in client_golang. Therefore, I see no pressing needs to add support for them now. (One might even argue that we actually want to discourage the other schemas ASAP, and providing UTF-8 support plus a sophisticated escaping schema for old backends has been invented precisely to get rid of those other, problematic escaping schemas.) But if we want to support them, I guess we should use the same knob to select them, i.e. content negotiation. It ultimately depends on the use case, but I'm not sure yet what the use case for those other escaping schemas will be. |
|
After doing a bunch of work, indeed I think it's best to put content negotiation in a separate PR. I believe this PR still does what it says and should not negatively affect people using this library (unless they turn on the UTF8 validation mode, which is not really supported anywhere else and will result in errors) |
|
(I will squash this and sign the commit when review is done to solve the DCO failure) |
Fair enough. But I think we need to document a bit better what the precise behavior after this PR is and what still has to be done. I'm mostly worried that it could now happen very easily that somebody uses the code to produce metrics rendered in the new UTF-8 capable style after negotiating the existing old formats. (Once content negotiation is fully implemented, the encoders would use the underscore encoding described in the design doc in that case.) I guess the doc comment for |
|
To clarify: Despite the title, this PR doesn't really change the parsing side, does it? I mean, the configurable validation implicitly adds support to the protobuf decoder. But the other parsers don't change. Maybe that could be added as a TODO. Also, a TODO in the content negotiation part might help to avoid confusion. |
model/metric.go
Outdated
| "unicode/utf8" | ||
| ) | ||
|
|
||
| type validationSchemeId int |
There was a problem hiding this comment.
Go pseudo enums keep confusing me. Maybe it's better to make this type exported? And here or below add doc comments about how the two options change the behavior (which might just be a reference to IsValidMetricName below, like: "See IsValidMetricName for details how the validation changes.")
ywwg
force-pushed
the
owilliams/quoted-metric-name
branch
from
3078b7f
to
cbd00f5
Compare
ywwg
changed the title
the comment already says "this function assumes the input is already sanitized and does not perform any Do you want more than that? |
|
oh I get it now, adding... |
Signed-off-by: Owen Williams <owen.williams@grafana.com>
ywwg
force-pushed
the
owilliams/quoted-metric-name
branch
from
cbd00f5
to
0940b71
Compare
There was a problem hiding this comment.
Thank you very much. And sorry for requesting a few more doc comment tweaks.
About the TODO I would like to see in the content negotiation part: In https://github.com/prometheus/common/blob/main/expfmt/expfmt.go , there should be a TODO that reads something like this: "The content-type strings here are all for the legacy exposition formats, where valid characters for metric names and label names are limited. The support for arbitrary UTF-8 characters in those names is already partially implemented in this module (see model.ValidationScheme), but to actually use it on the wire, new content-type strings have to be agreed upon and added here."
expfmt/text_create.go
Outdated
| @@ -330,32 +324,62 @@ func writeSample( | |||
| return written, nil | |||
| } | |||
|
|
|||
| // writeLabelPairs converts a slice of LabelPair proto messages plus the | |||
| // writeNameAndLabelPairs converts a slice of LabelPair proto messages plus the | |||
| // explicitly given additional label pair into text formatted as required by the | |||
| // text format and writes it to 'w'. An empty slice in combination with an empty | |||
| // string 'additionalLabelName' results in nothing being written. Otherwise, the | |||
| // label pairs are written, escaped as required by the text format, and enclosed | |||
| // in '{...}'. The function returns the number of bytes written and any error | |||
| // encountered. | |||
There was a problem hiding this comment.
The doc comment here would benefit from an update for the new features (additionally passed name, special syntax/quoting for name and label names containing formerly invalid characters).
Signed-off-by: Owen Williams <owen.williams@grafana.com>
There was a problem hiding this comment.
Thank you very much.
But what do you think about my earlier comment? This one:
About the TODO I would like to see in the content negotiation part: In https://github.com/prometheus/common/blob/main/expfmt/expfmt.go , there should be a TODO that reads something like this: "The content-type strings here are all for the legacy exposition formats, where valid characters for metric names and label names are limited. The support for arbitrary UTF-8 characters in those names is already partially implemented in this module (see model.ValidationScheme), but to actually use it on the wire, new content-type strings have to be agreed upon and added here."
Maybe you just missed it?
Signed-off-by: Owen Williams <owen.williams@grafana.com>
|
ah, sorry about that! done now |
|
Thank you very much. |
Adds UTF8 metric and label name validation mode and adds support for parsing and formatting the UTF8 text format.
The expfmt package will use the new quoting syntax iff the metric name does not conform to the legacy name format. Old metrics will still be presented as
foo{}whereas utf8 metrics will be presented as{"foo"}. (Formatters already assume that names are already validated and will not throw an error if names are invalid.)The model package has a new library flag,
NameValidationScheme, which determines how validation will be done on all calls. If a caller sometimes needs to validate utf8 but sometimes also needs to validate against the legacy scheme, they should use IsValidLegacyMetricName where needed. Changing NameValidationScheme after startup, especially if there are multiple goroutines active, is not recommended and can result in undefined behavior.NameValidationScheme is Legacy by default, so clients of this library should see no changes in behavior unless they change that value.
There is some minor style cleanup of redundant type names in tests.
Works towards #527
Future work will be needed on the content negotiation code.