Added support for the protobuf format for prometheus #602
Conversation
|
Looks like there's some clippy issues, will fix those this evening! |
alex
force-pushed
the
prometheus-protobuf-format
branch
3 times, most recently
from
01db2f9 to
69a470b
Compare
|
Ok, clippy issues should be resolved! |
|
Is a maintainer able to set the option on the repo so that new contributor's CI runs don't need to be explicitly approved? |
|
|
||
| // Process counters | ||
| for (name, by_labels) in &snapshot.counters { | ||
| let sanitized_name = sanitize_metric_name(name); |
There was a problem hiding this comment.
Just a general note: we might need to/want to evaluate, as part of a future PR, relaxing this sanitization since I think full UTF-8 metric names/tags are allowed for the Protobuf payloads? (and the text format, if you use a recent enough version and escape properly, yadda yadda)
There was a problem hiding this comment.
Presumably none of that escaping is required for the protobuf format. I'm happy to drop if you'd like? These lines were not added with a ton of thought :-/
There was a problem hiding this comment.
I think it's fine for now since it matches the existing behavior and it can always be removed later. More just noting it out loud in case people stumble through and say "hey, why it is it sanitizing my metrics like this with protobuf?"
It's explicitly set this way due to the potential for exploits to be invoked through automatically running CI against untrusted changes in PRs. I can appreciate that it makes it harder to iterate on CI-related fixes, but I don't want to want to have to think any harder than absolutely necessary about repo security, so this is how it stays. 😅 |
|
(Will look at any review feedback after work, Thanks!) |
this is a pre-req for metrics-rs#601, because native histograms are only supported with protobufs
alex
force-pushed
the
prometheus-protobuf-format
branch
from
69a470b to
ed7ac98
Compare
|
FWIW, based on my experience maintaining other OSS projects, I think the best way to handle that is to ensure that all jobs that run on pull_request envs don't require any secrets or do anything destructive, so that the worst thing anyone can do is waste resources. |
this is a pre-req for #601, because native histograms are only supported with protobufs
in addition to the unit tests, I also ran the server example with a local prometheus configured to only talk protobuf and confirmed that metrics flowed.
unfortunately, this format is not well documented so I made some reasonable inferences, but wasn't positive about many things, so a close eye is appreciated!