Skip to content

UTF-8 Content Negotiation - #1102

Merged
csmarchbanks merged 1 commit into
masterfrom
owilliams/utf8-02
Jun 12, 2025
Merged

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

Conversation

@ywwg

@ywwgywwg 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
ywwg commented Apr 10, 2025
View reviewed changes
Comment threadprometheus_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'

@ywwgywwgApr 10, 2025•
edited
Loading

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.

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

@csmarchbankscsmarchbanksApr 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.

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?

@csmarchbankscsmarchbanksMay 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.

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.

@ywwgywwgMay 29, 2025

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

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

ywwg commented Apr 10, 2025

Copy link
Copy Markdown
MemberAuthor

TODO need more openmetrics/exposition tests

@ywwg
ywwgforce-pushed the owilliams/utf8-02 branch 2 times, most recently from 33ea581 to 630fb65CompareApril 10, 2025 14:42
@ywwg
ywwg marked this pull request as ready for review April 10, 2025 14:42
@ywwg
ywwg marked this pull request as draft April 10, 2025 14:42
@ywwg
ywwgforce-pushed the owilliams/utf8-02 branch from 60097b9 to 769f7fbCompareApril 10, 2025 15:00
@ywwg
ywwg marked this pull request as ready for review April 10, 2025 15:00
@ywwg
ywwg requested a review from vesariApril 10, 2025 15:03
ywwg
ywwg commented Apr 10, 2025
View reviewed changes
Comment threadprometheus_client/openmetrics/exposition.py Outdated
@ywwgywwg changed the title initial work on content negotiation for client pythonUTF-8 Content NegotiationApr 10, 2025
@ywwg
ywwgforce-pushed the owilliams/utf8-02 branch from d7d7dc2 to 80fa206CompareApril 10, 2025 18:20
csmarchbanks
csmarchbanks reviewed Apr 18, 2025
View reviewed changes
Comment threadprometheus_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'

@csmarchbankscsmarchbanksApr 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.

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 threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/openmetrics/exposition.py Outdated
Comment threadtools/simple_client.py

@csmarchbankscsmarchbanksApr 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.

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

@ywwgywwgApr 30, 2025

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.

just for testing locally

@ywwgywwgApr 30, 2025

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.

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

@ywwg

ywwg commented Apr 30, 2025

Copy link
Copy Markdown
MemberAuthor

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

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

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 threadprometheus_client/__init__.py Outdated
Comment threadprometheus_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'

@csmarchbankscsmarchbanksMay 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.

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 threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/openmetrics/exposition.py Outdated
Comment threadprometheus_client/openmetrics/exposition.py Outdated
Comment threadprometheus_client/validation.py Outdated
@ywwg
ywwg requested a review from csmarchbanksMay 29, 2025 15:21
csmarchbanks
csmarchbanks reviewed Jun 2, 2025
View reviewed changes

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

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

Comment threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/exposition.py Outdated
Comment threadprometheus_client/openmetrics/exposition.py Outdated
Comment threadprometheus_client/openmetrics/exposition.py Outdated
csmarchbanks
csmarchbanks reviewed Jun 2, 2025
View reviewed changes
Comment threadprometheus_client/openmetrics/exposition.py Outdated
@ywwg
ywwg requested a review from csmarchbanksJune 2, 2025 14:49
@ywwg

ywwg commented Jun 3, 2025

Copy link
Copy Markdown
MemberAuthor

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

csmarchbanks
csmarchbanks approved these changes Jun 11, 2025
View reviewed changes

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

👍 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 threadtests/openmetrics/test_parser.py Outdated
Comment threadtests/test_parser.py
@ywwg
ywwgforce-pushed the owilliams/utf8-02 branch 3 times, most recently from e87f3a1 to c45fbefCompareJune 12, 2025 15:18
@ywwg
UTF-8 Content Negotiation
831ed02
Part of #1013
Signed-off-by: Owen Williams <owen.williams@grafana.com>
@ywwg
ywwgforce-pushed the owilliams/utf8-02 branch from c45fbef to 831ed02CompareJune 12, 2025 15:27
@csmarchbanks
csmarchbanks merged commit a6726a4 into masterJun 12, 2025
@csmarchbanks
csmarchbanks deleted the owilliams/utf8-02 branch June 12, 2025 15:28
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@csmarchbankscsmarchbankscsmarchbanks approved these changes
@vesarivesariAwaiting 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

@ywwg@csmarchbanks