Skip to content

Revert "pkg/distributor: shardByAllLabels didn't check for label names order correctly" #1961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Revert "pkg/distributor: shardByAllLabels didn't check for label names order correctly" #1961

wants to merge 1 commit into from

Conversation

gouthamve
Copy link
Contributor

Reverts #1957

We've deployed this into our env and are seeing 500s being thrown. Investigating.

pracucci
pracucci approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving being a correct revert of #1957. Sorry to hear it caused troubles. Will be very interesting to investigate the root cause (why input labels are not sorted).

pstibrany
pstibrany approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCO check failed ;-)

Sorry it has caused issues. If we cannot get sorted labels as input to this function, then we should sort them inside, in order to generate consistent result for each variations in incoming data.

Or we can use "fast fingerprint" hack, which only XORs individual hashes of pairs, and doesn't rely on ordering. (Which would however cause resharding of everything... so maybe not a good idea)

@gouthamve
Copy link
Contributor Author

Turns out this might be due to an internal client and not Prometheus. Prometheus sends us data with labels sorted..

@bboreham
Copy link
Contributor

bboreham commented Jan 8, 2020 •
edited
Loading

Labels are sorted once they hit the ingester , and we do use fast fingerprint to shard across ingesters.
Apologies, that is why the check is there.

Also note Prometheus did not sort labels before 2.0.

@pstibrany pstibrany mentioned this pull request Jan 8, 2020
Merged
2 tasks
@gouthamve
Revert "pkg/distributor: shardByAllLabels didn't check for label name…
0adca96 
…s order correctly (#1957)"

This reverts commit f49eb9e.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve gouthamve force-pushed the revert-1957-shard_by_all_labels_check branch from cde2483 to 0adca96 Compare January 9, 2020 11:13
@gouthamve
Copy link
Contributor Author

Superseded by #1964

@gouthamve gouthamve closed this Jan 14, 2020
@gouthamve gouthamve deleted the revert-1957-shard_by_all_labels_check branch January 14, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gouthamve @bboreham @pstibrany @pracucci