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

UTF-8 Content Negotiation by ywwg · Pull Request #1102 · prometheus/client_python · GitHub

/ client_python Public

UTF-8 Content Negotiation - #1102

Merged
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-02
Jun 12, 2025
ConversationCommits (1)ChecksFiles changed
Merged

UTF-8 Content Negotiation#1102
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-02

Conversation

ywwg commented Apr 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Part of #1013

The remaining piece is configuration to force escaping even if a scraper does not request it

ywwg commented Apr 10, 2025
View reviewed changes
Comment thread prometheus_client/exposition.py Outdated
)

CONTENT_TYPE_LATEST = 'text/plain; version=0.0.4; charset=utf-8'
CONTENT_TYPE_PLAIN = 'text/plain; version=0.0.4; charset=utf-8'

ywwg Apr 10, 2025 •
edited
Loading

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

text/plain v 0.0.4 is not the latest, so I thought it best to rename this

csmarchbanks Apr 18, 2025

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

Though I agree with you, this will be a breaking change as other programs use CONTENT_TYPE_LATEST. Perhaps we should update this to version=1.0.0 instead?

csmarchbanks May 29, 2025

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

Since this is needed for compatibility, but LATEST and PLAIN are both really different versions of PLAIN that is confusing for external users to use. I'd propose a couple options:

  1. Keep CONTENT_TYPE_PLAIN internal, so rename it to _CONTENT_TYPE_PLAIN.
  2. Support content types for each version, so have CONTENT_TYPE_PLAIN_0_4_0, CONTENT_TYPE_PLAIN_1_0_0, and then alias CONTENT_TYPE_LATEST to CONTENT_TYPE_PLAIN_1_0_0.

ywwg May 29, 2025

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 like option 2

ywwg marked this pull request as ready for review April 10, 2025 14:08
ywwg marked this pull request as draft April 10, 2025 14:17
ywwg force-pushed the owilliams/utf8-02 branch from 61e4a01 to 3240802 Compare April 10, 2025 14:24

ywwg commented Apr 10, 2025

Copy link
Copy Markdown
Member Author

TODO need more openmetrics/exposition tests

ywwg force-pushed the owilliams/utf8-02 branch 2 times, most recently from 33ea581 to 630fb65 Compare April 10, 2025 14:42
ywwg marked this pull request as ready for review April 10, 2025 14:42
ywwg marked this pull request as draft April 10, 2025 14:42
ywwg force-pushed the owilliams/utf8-02 branch from 60097b9 to 769f7fb Compare April 10, 2025 15:00
ywwg marked this pull request as ready for review April 10, 2025 15:00
ywwg requested a review from vesari April 10, 2025 15:03
ywwg commented Apr 10, 2025
View reviewed changes
Comment thread prometheus_client/openmetrics/exposition.py Outdated
ywwg changed the title initial work on content negotiation for client python UTF-8 Content Negotiation Apr 10, 2025
ywwg force-pushed the owilliams/utf8-02 branch from d7d7dc2 to 80fa206 Compare April 10, 2025 18:20
csmarchbanks reviewed Apr 18, 2025
View reviewed changes
Comment thread prometheus_client/exposition.py Outdated
)

CONTENT_TYPE_LATEST = 'text/plain; version=0.0.4; charset=utf-8'
CONTENT_TYPE_PLAIN = 'text/plain; version=0.0.4; charset=utf-8'

csmarchbanks Apr 18, 2025

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

Though I agree with you, this will be a breaking change as other programs use CONTENT_TYPE_LATEST. Perhaps we should update this to version=1.0.0 instead?

Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/openmetrics/exposition.py Outdated
Comment thread tools/simple_client.py

csmarchbanks Apr 18, 2025

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

Is this used anywhere? Or just a nice example for testing locally?

ywwg Apr 30, 2025

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

just for testing locally

ywwg Apr 30, 2025

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

but is also pretty nice for people to get off the ground

ywwg commented Apr 30, 2025

Copy link
Copy Markdown
Member Author

Though I agree with you, this will be a breaking change as other programs use CONTENT_TYPE_LATEST. Perhaps we should update this to version=1.0.0 instead?

we do still need a 0.0.4 constant as the fallback when all else fails. What I could do is update LATEST to be 1.0.0 and then create the new PLAIN constant for 0.0.4

ywwg requested a review from csmarchbanks April 30, 2025 18:25
ywwg marked this pull request as draft April 30, 2025 18:31
ywwg marked this pull request as ready for review April 30, 2025 18:49
csmarchbanks reviewed May 29, 2025
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

Generally looks to be getting there! A few comments mostly around the content types still. CONTENT_TYPE_LATEST from just exposition.py (not open metrics) doesn't actually seem to be used?

Comment thread prometheus_client/__init__.py Outdated
Comment thread prometheus_client/exposition.py Outdated
)

CONTENT_TYPE_LATEST = 'text/plain; version=0.0.4; charset=utf-8'
CONTENT_TYPE_PLAIN = 'text/plain; version=0.0.4; charset=utf-8'

csmarchbanks May 29, 2025

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

Since this is needed for compatibility, but LATEST and PLAIN are both really different versions of PLAIN that is confusing for external users to use. I'd propose a couple options:

  1. Keep CONTENT_TYPE_PLAIN internal, so rename it to _CONTENT_TYPE_PLAIN.
  2. Support content types for each version, so have CONTENT_TYPE_PLAIN_0_4_0, CONTENT_TYPE_PLAIN_1_0_0, and then alias CONTENT_TYPE_LATEST to CONTENT_TYPE_PLAIN_1_0_0.

Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/openmetrics/exposition.py Outdated
Comment thread prometheus_client/openmetrics/exposition.py Outdated
Comment thread prometheus_client/validation.py Outdated
ywwg requested a review from csmarchbanks May 29, 2025 15:21
csmarchbanks reviewed Jun 2, 2025
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

When everything is specified it seems to work well, though I ran into a few edge cases that need fixes and testing.

Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/exposition.py Outdated
Comment thread prometheus_client/openmetrics/exposition.py Outdated
Comment thread prometheus_client/openmetrics/exposition.py Outdated
csmarchbanks reviewed Jun 2, 2025
View reviewed changes
Comment thread prometheus_client/openmetrics/exposition.py Outdated
ywwg requested a review from csmarchbanks June 2, 2025 14:49

ywwg commented Jun 3, 2025

Copy link
Copy Markdown
Member Author

I ended up not using mocks, a simple regex check feels robust enough to confirm correctness.

csmarchbanks approved these changes Jun 11, 2025
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

👍 Thanks! Tested it for a bit and it is working well for me. There are some test changes where generate_latest now takes an escaping that isn't necessary now that there is a default value, but I am not opposed to passing in explicit values either.

Comment thread tests/openmetrics/test_parser.py Outdated
Comment thread tests/test_parser.py
ywwg force-pushed the owilliams/utf8-02 branch 3 times, most recently from e87f3a1 to c45fbef Compare June 12, 2025 15:18
UTF-8 Content Negotiation
831ed02 
Part of #1013

Signed-off-by: Owen Williams <owen.williams@grafana.com>
ywwg force-pushed the owilliams/utf8-02 branch from c45fbef to 831ed02 Compare June 12, 2025 15:27
csmarchbanks merged commit a6726a4 into master Jun 12, 2025
csmarchbanks deleted the owilliams/utf8-02 branch June 12, 2025 15:28
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

csmarchbanks csmarchbanks approved these changes
vesari Awaiting requested review from vesari

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL