FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support UTF-8 in metric creation, parsing, and exposition by ywwg · Pull Request #1070 · prometheus/client_python · GitHub

/ client_python Public

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

Merged
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-01
Dec 2, 2024
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

ywwg commented Nov 6, 2024 •
edited
Loading

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

ywwg commented Nov 6, 2024
View reviewed changes
Comment thread tests/test_core.py
ywwg marked this pull request as draft November 6, 2024 14:42

ywwg commented Nov 7, 2024

Copy link
Copy Markdown
Member Author

I'll squash commits when we're done

ywwg marked this pull request as ready for review November 7, 2024 20:47
ywwg marked this pull request as draft November 7, 2024 20:55
ywwg marked this pull request as ready for review November 12, 2024 15:42
ywwg requested a review from csmarchbanks November 12, 2024 15:59
csmarchbanks reviewed Nov 12, 2024
View reviewed changes

csmarchbanks 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread tests/openmetrics/test_parser.py Outdated
Comment thread tests/test_core.py
Comment thread prometheus_client/validation.py Outdated
_legacy_validation = True


def validate_metric_name(name: str) -> None:

csmarchbanks Nov 12, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ywwg Nov 14, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

csmarchbanks Nov 14, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ywwg Nov 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

csmarchbanks Nov 15, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 15, 2024

Copy link
Copy Markdown
Member Author

Added a test, fixed an issue using .split

ywwg commented Nov 15, 2024
View reviewed changes
Comment thread prometheus_client/parser.py Outdated
return tokens


def unquote_unescape(text):

ywwg Nov 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe these should be private too?

csmarchbanks Nov 19, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would say yes, thanks!

ywwg requested a review from csmarchbanks November 18, 2024 15:13
csmarchbanks reviewed Nov 19, 2024
View reviewed changes

csmarchbanks 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread prometheus_client/openmetrics/exposition.py Outdated
Comment thread prometheus_client/parser.py Outdated
return tokens


def unquote_unescape(text):

csmarchbanks Nov 19, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would say yes, thanks!

Comment thread prometheus_client/parser.py Outdated
ywwg changed the title WIP: Support UTF-8 in metric creation, parsing, and exposition Support UTF-8 in metric creation, parsing, and exposition Nov 20, 2024
ywwg force-pushed the owilliams/utf8-01 branch from 303ff91 to d50068f Compare November 20, 2024 16:38
ywwg requested a review from csmarchbanks November 20, 2024 16:39
ywwg force-pushed the owilliams/utf8-01 branch from d50068f to cf5bdc0 Compare November 20, 2024 18:22
Support UTF-8 in metric creation, parsing, and exposition
b21a7a1 
part of #1013

Signed-off-by: Owen Williams <owen.williams@grafana.com>
ywwg force-pushed the owilliams/utf8-01 branch from cf5bdc0 to b21a7a1 Compare November 20, 2024 18:23
ywwg requested a review from vesari November 27, 2024 14:59
vesari approved these changes Nov 30, 2024
View reviewed changes

vesari 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for this great work, LGTM!

Comment thread prometheus_client/openmetrics/exposition.py
Comment thread prometheus_client/openmetrics/exposition.py
csmarchbanks merged commit 33e6828 into master Dec 2, 2024
csmarchbanks deleted the owilliams/utf8-01 branch December 2, 2024 18:18
dg98 mentioned this pull request Feb 24, 2025
Merged
csmarchbanks mentioned this pull request Jun 24, 2025
Open
renovate Bot mentioned this pull request Jul 26, 2025
Open
1 task
Nevon mentioned this pull request Aug 14, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vesari vesari approved these changes
csmarchbanks csmarchbanks approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL