append labels server side by incoming query param #23
Conversation
norrs
force-pushed
the
serversidelabels
branch
from
d2c2b8a to
7d96b2d
Compare
This allows users to construct their own main(). This is useful for constructing your own webserver setup for adding features such as api signing.
norrs
force-pushed
the
serversidelabels
branch
from
7d96b2d to
fbf0a31
Compare
|
Fixed my commits to be signed. Forgot to update a few settings after migrating to yubikey5. |
|
We have some more code that is work in progress, need a few more changes so we can easily just depend on this as a module and override and make our own main(). |
norrs
changed the title
Allow to change default endpoint for where to retreive push metrics.
norrs
changed the title
|
This is ready for being reviewed, this allowed us to reuse your code while replacing main() with our own http server setup using api signing etc. |
|
Hi, thanks for the PR, and sorry it languished for a bit. It's quite big (at least superficially) so may take a while longer to figure out what changed. |
|
@bboreham : |
There was a problem hiding this comment.
I have read through this now; nothing jumps out as a problem, but I think it deserves some explanation in the README how to use this new feature.
It looks like you supply one name which will appear in the URL like http://x/...?name=foo:bar which will be appended as a Prometheus label foo=bar.
If anything goes wrong (e.g. the query param has the wrong format) then it is silently ignored.
Would it be simpler just to add query parameters ?foo=bar as labels? Then we don't need a flag and code to split around :.
If you are making changes I would prefer the split to a separate package to be a separate PR - that has nothing to do with labels.
Also #31 did a similar change for endpoint configuration, so that is a merge clash now.
cmd/prom-aggregation-gateway/main.go
Outdated
| for _, m := range metrics { | ||
| if _, foundLabel := queryArguments[labelQueryParam]; foundLabel { | ||
| for _, label := range queryArguments[labelQueryParam] { | ||
| l := strings.Split(label, ":") |
There was a problem hiding this comment.
This might be more flexible as SplitN() ?
There was a problem hiding this comment.
Yes, if we would want to support using the separator in its value.
Thanks, will update after #23 (comment)
|
@bboreham : > Would it be simpler just to add query parameters ?foo=bar as labels? Sadly this would include other query parameters we need to send to the service for doing API signing which we have added internally for our http router. Hence they are sent as request parameter (which is configurable) which is read as an array according to the http spec for query arguments. Not sure how I would add filtering on my side in additional if you wanted to create prometheus labels based from any query parameter and its value. (as I would then need to filter a few query parameters that shouldn't be prometheus labels). I guess I could split up into two PRs, but however I do think that #31 (review) needs to be fixed first before I rebase and split the PR. |
|
I notice the Prometheus Pushgateway has a similar feature, but the labels form part of the http path - see https://github.com/prometheus/pushgateway#url . Would that work for you? It would be great to make the projects more compatible. |
This feature is useful for mobile clients.
This allows us to provide an endpoint via configuration containing specific labels we want to have appended on metrics sent from the client.
It also makes client code less bloated, as they don't have to provide
client_versionetc everywhere they use a Counter. It could also be other relevant metrics such as client participating in a experiment.