Validate incoming labels for order and duplicate names. #1964
Conversation
|
Given the remote_write API doesn't specify ordering and because Prometheus 1.x sends unordered labels, I think rejecting unordered labels is not the approach we want to take. Maybe we can sort them, but lets not reject them. |
|
To summarize our Slack discussion: Reason to make this PR was to make sure that sharding token is correct when sharding using all labels. Ordering issues or duplicate label names generate different tokens, eg. There is no question about that duplicate labels should be rejected. In order to detect them cheaply, I added check for ordering. I assumed that since the code was already there (although not working properly, hence #1957), that is what we wanted. If we don't want to reject unordered labels, then I suggest that we simply order them in distributor (if needed). This will help us to 1) fix above-mentioned problem with token, while not resharding the world again (by changing the sharding computation), 2) still detect duplicate label names cheaply. I'll update my PR to implement this change instead. |
There was a problem hiding this comment.
Good job @pstibrany ! The changes LGTM, with a couple of nits.
Signed-off-by: Peter Štibraný <[email protected]>
…licate labels. Signed-off-by: Peter Štibraný <[email protected]>
{__name__=""} will be shown formatted as {__name__=""} instead of just empty string.
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Rebased on top of |
* Release 0.5.0-rc0 Signed-off-by: Goutham Veeramachaneni <[email protected]> * Fix changelog to point to 0.5.0-rc.0 Signed-off-by: Goutham Veeramachaneni <[email protected]> * Validate incoming labels for order and duplicate names. (#1964) * Validate incoming labels for order and duplicate names. Signed-off-by: Peter Štibraný <[email protected]> * Document that cortex rejects requests with incorrectly ordered or duplicate labels. Signed-off-by: Peter Štibraný <[email protected]> * Ignore empty metric name. {__name__=""} will be shown formatted as {__name__=""} instead of just empty string. Signed-off-by: Peter Štibraný <[email protected]> * As we rely on sorted labels, sort them before using them. Signed-off-by: Peter Štibraný <[email protected]> * Updated CHANGELOG.md to reflect latest changes to PR. Signed-off-by: Peter Štibraný <[email protected]> * Put back redundant aliases. Signed-off-by: Peter Štibraný <[email protected]> * wrap migration commands (#1980) * wrap migration commands Signed-off-by: Jacob Lisi <[email protected]> * update changelog Signed-off-by: Jacob Lisi <[email protected]> * fix missing semicolon Signed-off-by: Jacob Lisi <[email protected]> * Fix typo to make lint pass. Signed-off-by: Goutham Veeramachaneni <[email protected]> Co-authored-by: Goutham Veeramachaneni <[email protected]> * Call out breaking changes better. Signed-off-by: Goutham Veeramachaneni <[email protected]> * Label this version as -rc1 Signed-off-by: Goutham Veeramachaneni <[email protected]> * We're abandoning the `0.5.0` release. Signed-off-by: Goutham Veeramachaneni <[email protected]> Co-authored-by: Peter Štibraný <[email protected]> Co-authored-by: Jacob Lisi <[email protected]>
Originally this PR added only check for sorted labels.This PR sorts unsorted labels in distributor, before using them for computing sharding token. It also adds check if there are duplicate label names, and reports them.To properly format the input, it reimplements formatting function. Previously used function merged duplicate labels, and also sorted labels, so error message didn't include real input, but "normalized" version already. That's not very useful for debugging.
Checklist
CHANGELOG.mdupdatedRelated to #1957 and #1961 which reverts it. This builds on #1957 and fixes it, while ignoring #1961.