Skip to content

Support UTF-8 in metric creation, parsing, and exposition - #1070

Merged
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-01
Dec 2, 2024
Merged

Support UTF-8 in metric creation, parsing, and exposition#1070
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-01

Conversation

@ywwg

@ywwgywwg commented Nov 6, 2024

Copy link
Copy Markdown
Member

Adding support for UTF-8 required substantial reworking of the parsing code because the previous code relied on regexes and .index calls that would incorrectly find characters inside quote marks. Note that this means the previous code was already broken for parsing exposition text where characters like braces and hashes were in label values. I have added tests to exercise the edge cases I can think of.

Does not address content negotiation, which will be a followon.

part of #1013

Comment threadtests/test_core.py
@ywwg
ywwg marked this pull request as draft November 6, 2024 14:42
@ywwg

ywwg commented Nov 7, 2024

Copy link
Copy Markdown
MemberAuthor

I'll squash commits when we're done

@ywwg
ywwg marked this pull request as ready for review November 7, 2024 20:47
@ywwg
ywwg marked this pull request as draft November 7, 2024 20:55
@ywwg
ywwg marked this pull request as ready for review November 12, 2024 15:42

@csmarchbankscsmarchbanks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a first runthrough. Generally looking ok to me. A couple thoughts:

  1. If you want to split it up I think refactoring validation + removing regex parsing could be good opportunities. Plus have some tests that catch the incorrect behavior you mentioned (e.g. curly braces in a label value).
    • Tests for this would be nice in this PR too
  2. Some more tests for various permutations would be nice, specifically there are not many tests for escaped label names/values right now

Comment threadtests/openmetrics/test_parser.py Outdated
Comment threadtests/test_core.py
Comment threadprometheus_client/validation.py Outdated
_legacy_validation = True


def validate_metric_name(name: str) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want these to be public now? Might be worth having them still prefixed with _ but I also think they could be useful general functions for other libraries to use.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the functions are used by both prometheus_client and prometheus_client/openmetrics so I thought they had to be public

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yeah Python doesn't really do public/private, it's just a convention.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused what change you want me to make, then -- leave it? prepend _? something else?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh sorry, yeah if you could prepend with _ I think that would be safest to begin with to avoid others depending on the methods.

@ywwg

ywwg commented Nov 15, 2024

Copy link
Copy Markdown
MemberAuthor

Added a test, fixed an issue using .split

Comment threadprometheus_client/parser.py Outdated
return tokens


def unquote_unescape(text):

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe these should be private too?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say yes, thanks!

@csmarchbankscsmarchbanks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bit of prepending with more _ and I think this is good to go! Plus cleaning up the commit history and such.

Comment threadprometheus_client/openmetrics/exposition.py Outdated
Comment threadprometheus_client/parser.py Outdated
return tokens


def unquote_unescape(text):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say yes, thanks!

Comment threadprometheus_client/parser.py Outdated
@ywwgywwg changed the title WIP: Support UTF-8 in metric creation, parsing, and expositionSupport UTF-8 in metric creation, parsing, and expositionNov 20, 2024
part of #1013
Signed-off-by: Owen Williams <owen.williams@grafana.com>
@ywwg
ywwg requested a review from vesariNovember 27, 2024 14:59

@vesarivesari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this great work, LGTM!

Comment threadprometheus_client/openmetrics/exposition.py
Comment threadprometheus_client/openmetrics/exposition.py
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ywwg@csmarchbanks@vesari