Skip to content
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.

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

Remove hardcoded labels in store shard matcher and inject unshardable label in query analyzer #5641

Merged
merged 2 commits into from
Sep 26, 2022

Conversation

haanhvu
Copy link
Contributor

@haanhvu haanhvu commented Aug 24, 2022

Signed-off-by: haanhvu haanh6594@gmail.com

fixes #5625

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Remove hardcoded __name__ label in store shard matcher to make it shardable. Inject unshardable le label in query analyzer.

Verification

@haanhvu haanhvu marked this pull request as draft August 24, 2022 23:50
@haanhvu
Copy link
Contributor Author

haanhvu commented Aug 24, 2022

Convert to draft to fix errors and warnings

@haanhvu haanhvu changed the title Remove hardcoded labels in store shard matcher Store: Remove hardcoded labels in shard matcher. Query: Inject unshardable 'le' label in query analyzer. Aug 28, 2022
@haanhvu haanhvu changed the title Store: Remove hardcoded labels in shard matcher. Query: Inject unshardable 'le' label in query analyzer. Store: Remove hardcoded labels in shard matcher. Query: Inject unshardable le label in query analyzer. Aug 28, 2022
@haanhvu haanhvu changed the title Store: Remove hardcoded labels in shard matcher. Query: Inject unshardable le label in query analyzer. Remove hardcoded labels in store shard matcher and inject unshardable label in query analyzer Aug 28, 2022
@haanhvu haanhvu marked this pull request as ready for review August 28, 2022 19:44
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. We should also add tests for this change.

pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
@haanhvu
Copy link
Contributor Author

haanhvu commented Aug 29, 2022

Thanks for the contribution. We should also add tests for this change.

@fpetkovski actually I didn't add the test because I see we already have one test case with le label?

expression: "histogram_quantile(0.95, sum(rate(metric[1m])) by (le, cluster))",
shardingLabels: []string{"cluster"},

@pull-request-size pull-request-size bot added size/S and removed size/M labels Aug 29, 2022
@haanhvu haanhvu marked this pull request as draft August 29, 2022 11:17
@haanhvu haanhvu marked this pull request as ready for review August 29, 2022 12:22
@haanhvu
Copy link
Contributor Author

haanhvu commented Aug 29, 2022

@fpetkovski I fixed all the suggestions. Pls review.

Thanks for the contribution. We should also add tests for this change.

@fpetkovski actually I didn't add the test because I see we already have one test case with le label?

expression: "histogram_quantile(0.95, sum(rate(metric[1m])) by (le, cluster))",
shardingLabels: []string{"cluster"},

We already have a shardableByLabels test case. So I just add a shardableWithoutLabels test case.

Also, could you review #5653 too?

@fpetkovski
Copy link
Contributor

Thanks, I can take a look at this tomorrow.

Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Looks good, I would just prefer to use locally scoped variables instead of declaring shardingLabels with a broad scope.

pkg/querysharding/analyzer.go Show resolved Hide resolved
pkg/querysharding/analyzer.go Outdated Show resolved Hide resolved
@haanhvu
Copy link
Contributor Author

haanhvu commented Sep 11, 2022

@fpetkovski done converting to local variables

fpetkovski
fpetkovski previously approved these changes Sep 12, 2022
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Thanks, this is nice work 👍

LGTM.

matej-g
matej-g previously approved these changes Sep 13, 2022
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Looks good 🙌, one nit

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: haanhvu <haanh6594@gmail.com>
fpetkovski
fpetkovski previously approved these changes Sep 26, 2022
matej-g
matej-g previously approved these changes Sep 26, 2022
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Nice work @haanhvu ❤️

Signed-off-by: Matej Gera <38492574+matej-g@users.noreply.github.com>
@matej-g matej-g dismissed stale reviews from fpetkovski and themself via 2b5d72e September 26, 2022 09:41
@matej-g matej-g merged commit 2fd75cd into thanos-io:main Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove hardcoded labels in store shard matcher
3 participants