prom-label-proxy icon indicating copy to clipboard operation
prom-label-proxy copied to clipboard

Published 20 hours ago • verified publisher icon prometheus-community

feat: Provide label value by http header

Open jkroepke opened this issue 2 years ago • 3 comments

Fixes #117

Closes #64

jkroepke avatar Jul 05 '22 06:07 jkroepke

Can we get this merged? Would be very useful.

kfox1111 avatar Aug 16 '22 19:08 kfox1111

@squat could you take a look here?

jkroepke avatar Sep 29 '22 06:09 jkroepke

Tagging all owners since there have been related PRs (#54 #64 #117 ) with the same feature pending for quite a while.

@brancz @krasi-georgiev @metalmatze @paulfantom @pgier @s-urbaniak @simonpasquier @squat @lilic

ThisIsQasim avatar Oct 04 '22 10:10 ThisIsQasim

@fpetkovski maybe?

jkroepke avatar Nov 27 '22 08:11 jkroepke

Would also be interested into this. Also into a new release because the actual 0.5.0 doesnt support the -values parameter (only the version in main does this).

Atomique avatar Dec 01 '22 16:12 Atomique

How we can proceed here?

jkroepke avatar Dec 22 '22 16:12 jkroepke

Hey @simonpasquier, I saw you suggestions, but I recommend to create an additional PR which superseeds this PR or modify to existing one. "Allow edits by maintainers" is enabled. if you have maintainer permissions on https://github.com/prometheus-community/prom-label-proxy, feel free to commit here. I wont do any refactorings, since I don't have the full golang knowledge.

jkroepke avatar Dec 23 '22 08:12 jkroepke

@jkroepke can you reopen my PR on your fork and merge it then? I could have pushed my changes on top of your branch but I didn't want to do it without you being aware :)

simonpasquier avatar Dec 23 '22 09:12 simonpasquier

Hey @simonpasquier please take a look that the merge conflict now which appears now after merging the PR

jkroepke avatar Dec 23 '22 09:12 jkroepke

@jkroepke sure, let me resolve this!

simonpasquier avatar Dec 23 '22 10:12 simonpasquier

@fpetkovski I'd like your input on this PR. In particular there's a slight difference compared to what you implemented in #116. With this change, users have to select the method to retrieve the label value from incoming requests:

  • from the HTTP form/query parameters (-query-param or -label)
  • from the HTTP headers (-header-name)
  • statically defined (-label-value)

It isn't possible to pass -label-value and -label at the same time hence when a static label value is used, the proxy will not remove anything from the HTTP parameters. Did you have a concrete use case for cleaning up the HTTP parameters when using the static configuration?

simonpasquier avatar Dec 23 '22 13:12 simonpasquier

It isn't possible to pass -label-value and -label at the same time hence when a static label value is used, the proxy will not remove anything from the HTTP parameters. Did you have a concrete use case for cleaning up the HTTP parameters when using the static configuration?

After discussing offline with @fpetkovski the statement above isn't correct: in the current version, the proxy will return an HTTP error code when configured with -label-value and the incoming HTTP request has a label query parameter. With this PR, the proxy configured with -label-value won't remove any HTTP query/form parameter but it sounds ok as it will be ignored anyway.

simonpasquier avatar Jan 04 '23 09:01 simonpasquier

thanks a lot @jkroepke for your work and patience!

simonpasquier avatar Jan 04 '23 09:01 simonpasquier