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.

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

Store Gateway index stats response #3206

Merged
merged 8 commits into from
Oct 21, 2022
Merged

Store Gateway index stats response #3206

merged 8 commits into from
Oct 21, 2022

Conversation

treid314
Copy link
Contributor

@treid314 treid314 commented Oct 13, 2022 •
edited
Loading

What this PR does

This PR extends the storegateway RPC protocol to support passing a stats message with a series response to allow us to track and log the number of index bytes read in the query frontend.

Which issue(s) this PR fixes or relates to

Fixes #2923

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci
pracucci reviewed Oct 13, 2022
View reviewed changes
Copy link
Collaborator

@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.

Thanks for working on this! Being able to track query stats in the store-gateway is... great! I just did a partial review so far, sorry (I need to get back to a meeting now).

pkg/storegateway/storegatewaypb/rpc.proto Outdated Show resolved Hide resolved
pkg/storegateway/storegatewaypb/rpc.proto Outdated Show resolved Hide resolved
pkg/storegateway/storegatewaypb/rpc.proto Outdated Show resolved Hide resolved
@pracucci
Copy link
Collaborator

@treid314 In #3222 I'm proposing to import all Thanos protobufs, so you should be able to move forward cleanly with this PR.

@pracucci
Copy link
Collaborator

@treid314 I merged #3222. Could you rebase, please?

pracucci
pracucci reviewed Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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.

Good job! The implementation is very clean. I left a couple of comments.

Have you got a chance to manually test it using the local dev env?

pkg/storegateway/storepb/rpc.proto Outdated Show resolved Hide resolved
pkg/storegateway/storepb/rpc.proto Outdated Show resolved Hide resolved
pkg/querier/blocks_store_queryable_test.go Outdated Show resolved Hide resolved
pkg/storegateway/bucket.go Outdated Show resolved Hide resolved
@treid314 treid314 marked this pull request as ready for review October 18, 2022 21:28
@treid314 treid314 requested a review from a team as a code owner October 18, 2022 21:28
56quarters
56quarters approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM, just a few small changes.

pkg/storegateway/storepb/rpc.proto Show resolved Hide resolved
pkg/storegateway/storepb/rpc.proto Outdated Show resolved Hide resolved
pracucci
pracucci approved these changes Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@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.

LGTM, thanks!

@pracucci pracucci enabled auto-merge (squash) October 21, 2022 06:40
@pracucci pracucci merged commit a3fac18 into main Oct 21, 2022
@pracucci pracucci deleted the tsdb-index-stats branch October 21, 2022 06:56
@56quarters 56quarters mentioned this pull request Oct 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

@56quarters 56quarters 56quarters 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.

Include TSDB index downloads in query stats
3 participants
@treid314 @pracucci @56quarters