-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Support UTF-8 label matchers: Update Stringer for non-Prometheus compliant label names #3580
Support UTF-8 label matchers: Update Stringer for non-Prometheus compliant label names #3580
Conversation
689b3c2
to
1dd19e6
Compare
// matchers. Until then, the classic parser does not understand double quotes | ||
// around the label name, so we use this function as a heuristic to tell if | ||
// the matcher was parsed with the UTF-8 parser or the classic parser. | ||
func isReserved(r rune) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming making the function public and re-using it here was a no-go for some reason?
I'd love if we could please keep track of these tidbits that we need to remove once we're done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a task to #3486
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of an odd one I think. Remember how we made the lexer and all it's related functions package-private? It felt weird to me that we would expose this one function that checks for reserved characters when the rest of the lexer and it's related functions are inaccessible.
Besides, I'm thinking we'll delete this once we removed the old parser so I felt it was cleaner to duplicate it and add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember how we made the lexer and all it's related functions package-private? It felt weird to me that we would expose this one function
I think a comment may do the trick to signal that this function is going away as this is temporary anyway, but I'm fine either way now that is properly tracked in the main epic.
This commit updates the String method to print non-Prometheus compliant label names in a format that can be parsed in the UTF-8 parser. Such inputs are never valid in the classic parser. If the label name is Prometheus compliant, it is still printed unquoted. Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: George Robinson <george.robinson@grafana.com>
1dd19e6
to
f076c89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit updates the String method to print non-Prometheus compliant label names in a format that can be parsed in the UTF-8 parser. Such inputs are never valid in the classic parser. If the label name is Prometheus compliant, it is still printed unquoted.