Conversation
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' |
There was a problem hiding this comment.
text/plain v 0.0.4 is not the latest, so I thought it best to rename this
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Keep
CONTENT_TYPE_PLAINinternal, so rename it to_CONTENT_TYPE_PLAIN. - Support content types for each version, so have
CONTENT_TYPE_PLAIN_0_4_0,CONTENT_TYPE_PLAIN_1_0_0, and then aliasCONTENT_TYPE_LATESTtoCONTENT_TYPE_PLAIN_1_0_0.
|
TODO need more openmetrics/exposition tests |
ywwg
force-pushed
the
owilliams/utf8-02
branch
2 times, most recently
from
33ea581 to
630fb65
Compare
ywwg
changed the title
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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is this used anywhere? Or just a nice example for testing locally?
There was a problem hiding this comment.
but is also pretty nice for people to get off the ground
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 |
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
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?
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' |
There was a problem hiding this comment.
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:
- Keep
CONTENT_TYPE_PLAINinternal, so rename it to_CONTENT_TYPE_PLAIN. - Support content types for each version, so have
CONTENT_TYPE_PLAIN_0_4_0,CONTENT_TYPE_PLAIN_1_0_0, and then aliasCONTENT_TYPE_LATESTtoCONTENT_TYPE_PLAIN_1_0_0.
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
When everything is specified it seems to work well, though I ran into a few edge cases that need fixes and testing.
|
I ended up not using mocks, a simple regex check feels robust enough to confirm correctness. |
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
👍 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.
ywwg
force-pushed
the
owilliams/utf8-02
branch
3 times, most recently
from
e87f3a1 to
c45fbef
Compare
Part of #1013 Signed-off-by: Owen Williams <owen.williams@grafana.com>
2 participants
Part of #1013
The remaining piece is configuration to force escaping even if a scraper does not request it