Support UTF-8 metric names and labels in web UI#15244
Conversation
Fixes most of #15202 This should address all areas of the UI except for the autocompletion in the codemirror-promql text editor. The strategy here is that any time we print or internally serialize (like for the PromLens tree view) either a metric name or a label name as part of a selector or in other relevant parts of PromQL, we check whether it contains characters beyond what was previously supported, and if so, quote and escape it. In the case of metric names, we also have to move them from the beginning of the selector into the curly braces. Signed-off-by: Julius Volz <[email protected]>
juliusv
force-pushed
the
utf8-web-ui-support
branch
from
8947dcb to
c861b31
Compare
|
looks like the tests are broken @juliusv. I don't know if you notice |
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
I can read some of this! 😅 Just a couple non-blocking comments on how I've been framing naming / logic choices when implementing this in other languages. Consistency is not required but might be nice to have in the future.
| @@ -1,12 +1,24 @@ | |||
| import { | |||
| maybeQuoteLabelName, | |||
| metricContainsExtendedCharset, | |||
There was a problem hiding this comment.
for what it's worth, I have been inverting this function call and naming it something like "isLegacyCompatible". I do this because UTF-8 is the new default, whereas the old restrictions only really apply to old versions of the code. This isn't a required fix but would be more consistent with how it's being implemented in other languages.
There was a problem hiding this comment.
I didn't like "legacy" because for me the old character set is still the main one and the one I'll keep advocating for for regular users of Prometheus - at least unless someone comes up with a better selector syntax for the extended character set 😅
| const labelNodes: React.ReactElement[] = []; | ||
| let first = true; | ||
|
|
||
| // If the metric name uses the extended new charset, we need to escape it, |
There was a problem hiding this comment.
continuing the reasoning from above, then the comment becomes something like: "If the metric name conforms to the legacy character set, it can appear outside the braces. All other metrics must be escaped and put into the label matcher list, and make sure it's the first item.")
Signed-off-by: Julius Volz <[email protected]>
Whoops, I forgot that it's not sufficient to do |
|
Build is passing - from my side it's fine to merge unless @ywwg feels strongly about the wording ;) |
|
Ok, will merge it for now, we can still change variable and function namings later on to make things more consistent one way or the other, if we want :) |
sorry I was on PTO -- no blocking comments |
3 participants
This should address all areas of the UI except for the autocompletion in the codemirror-promql text editor. The strategy here is that any time we print or internally serialize (like for the PromLens tree view) either a metric name or a label name as part of a selector or in other relevant parts of PromQL, we check whether it contains characters beyond what was previously supported, and if so, quote and escape it. In the case of metric names, we also have to move them from the beginning of the selector into the curly braces.
part of #15202