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

Split labels/series API endpoints in query frontend #3276

Merged
merged 16 commits into from
Oct 13, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Oct 5, 2020

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

Changes

This pr adds splitting and retry middleware support for all metadata API endpoints (label_name, label_value, series). This is the first step of #3230. Will add caching support in another pr since this pr is already very large.

Verification

Tests updated.

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24 yeya24 changed the title Split metadata API endpoints in the query frontend Split metadata API endpoints in query frontend Oct 5, 2020
@yeya24 yeya24 changed the title Split metadata API endpoints in query frontend WIP: Split metadata API endpoints in query frontend Oct 5, 2020
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Great work 🎉 I just skimmed over and looking good so far.

pkg/queryfrontend/roundtrip.go Outdated Show resolved Hide resolved
pkg/queryfrontend/roundtrip.go Outdated Show resolved Hide resolved
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@bwplotka bwplotka changed the title WIP: Split metadata API endpoints in query frontend WIP: Split labels/series API endpoints in query frontend Oct 8, 2020
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24 yeya24 changed the title WIP: Split labels/series API endpoints in query frontend Split labels/series API endpoints in query frontend Oct 8, 2020
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

@yeya24 Great work. It has taken some time to review the whole the PR. Overall it looks amazing. Thanks a lot for the contribution.

I have only added a small nit. Other than that we should test by integrating it with whole stack. It would be really nice to run it with quickstart script. WDYT?

pkg/queryfrontend/roundtrip.go Show resolved Hide resolved
@yeya24
Copy link
Contributor Author

yeya24 commented Oct 12, 2020

Other than that we should test by integrating it with whole stack. It would be really nice to run it with quickstart script. WDYT?

Yes, that's what I want as well. Since this task is trivial, I will open an issue and let someone else contribute.

Done: #3304

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Oct 12, 2020

While I am working on the caching part, I find that all response format must be protobuf because they will be marshaled in the results cache middlware.

I pushed another commit to switch the labels and series response to protobuf.

@kakkoyun kakkoyun merged commit e5b051d into thanos-io:master Oct 13, 2020
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing work, some post merge comments 🤗

}
}

func (c labelsCodec) MergeResponse(responses ...queryrange.Response) (queryrange.Response, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing exported comment

Copy link
Member

Choose a reason for hiding this comment

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

Some commentary why queryrange.Response is used for labels would be nice ;p

case *ThanosSeriesResponse:
seriesData := make([]labelpb.LabelSet, 0)

// seriesString is used in soring so we don't have to calculate the string of label sets again.
Copy link
Member

Choose a reason for hiding this comment

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

soring

OGKevin pushed a commit to OGKevin/thanos that referenced this pull request Oct 15, 2020
* split metadata endpoints

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add metadata codec implementation

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* cleanup go mod

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add back fix for thanos-io#3240

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* check set key exist

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* update

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add default metadata range flag

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* fix linting issues

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* refactor flags and add test cases

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* fix go lint issue

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add roundtrip tripperware tests for labels and series requests

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* fix all unit tests and e2e tests

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* fix lint issues

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add nolint unparam

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* update flags and changelog

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* switch response format to protobuf

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants