replace whole value instead of only matches for relabeling #118
Conversation
Change the behavior for the dynamic relabeling so that for a match the whole value is replaced by the replacement instead of the matches within the value being replaced by the replacement. This way it is possible to use regular expressions for matching that don't cover the whole value but still replace it completely.
|
Code Climate has analyzed commit ee9e4e9 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
Hey @stempler -- thanks for preparing this PR. To be honest, this new behaviour feels counterintuitive to me (which I've addressed in a detailed comment on one of the new test cases). Besides, it would completely break backwards-compatibility.
However, couldn't the same result be achieved by simply adding a catch-all expression like .* at the beginning (or end, or both) of the regular expression? For example, using ^.*(Firefox)/(\\d+)\\.(\\d+)(pre|[ab]\\d+[a-z]*|).*$ instead of (Firefox)/(\\d+)\\.(\\d+)(pre|[ab]\\d+[a-z]*|).
| @@ -49,4 +49,21 @@ func TestRequestURIMapping(t *testing.T) { | |||
| } | |||
|
|
|||
| assertMapping(t, r, "GET /users/12345 HTTP/1.1", "/users/:id") | |||
| assertMapping(t, r, "GET /users/12345/about HTTP/1.1", "/users/:id") | |||
There was a problem hiding this comment.
Huh. To be honest, this seems counterintuitive to me. I would have expected /users/12345/about to be mapped to /users/:id/about (or, in other words, I would not expect for [0-9]+ to replace anything more than 12345).
Alternatively, one could also have used ^/users/[0-9]+.* as a regex, which would effectively match the entire string and yield the same result.
|
Thank you for the feedback @martin-helmich !
Yes, you are right. I had considered it but had it wrong in my head and thought the greedy |
Change the behavior for the dynamic relabeling so that for a match the whole value is replaced by the replacement instead of the matches within the value being replaced by the replacement.
This way it is possible to use regular expressions for matching that don't cover the whole value but still replace it completely.
I was a bit surprised going by the examples in the README that the behavior was to replace the matches in the value instead of the whole value. Especially since I would guess the usual use case is to reduce the number of different values for a label.
So I hope that this change is in line with your intentions for the behavior of the relabeling and can serve as a fix.
Otherwise, maybe it would make sense to make the changed behavior based on a configuration option?