-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add skipChunk option in series API #1904
Conversation
e5cca30
to
f9d59aa
Compare
CHANGELOG.md
Outdated
@@ -18,6 +18,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
### Added | |||
- [#1852](https://github.com/thanos-io/thanos/pull/1852) Add support for `AWS_CONTAINER_CREDENTIALS_FULL_URI` by upgrading to minio-go v6.0.44 | |||
- [#1854](https://github.com/thanos-io/thanos/pull/1854) Update Rule UI to support alerts count displaying and filtering. | |||
- [#1904](https://github.com/thanos-io/thanos/pull/1904) Add a no-chunk option in Store Series API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps lets mention that this greatly improves performance of the /api/v1/series
endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/store/proxy_test.go
Outdated
@@ -420,6 +454,9 @@ func TestProxyStore_Series(t *testing.T) { | |||
|
|||
s := newStoreSeriesServer(context.Background()) | |||
|
|||
if tc.req.NoChunk == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably forgot to remove this? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
q.Add("start", string(startTime)) | ||
q.Add("end", string(endTime)) | ||
|
||
req, err := http.NewRequest("GET", u.String(), nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use http.NewRequestWithContext
here to shorten things :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also... can we use POST
here to avoid potential character limits with big queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, make sense! Thanks 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recheck the POST thing. Looks like https://github.com/prometheus/client_golang/blob/master/api/prometheus/v1/api.go also uses GET. So is it not necessary to use POST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we need to do GET to be fully compatible with old pre 2.8.x Prometheus versions.
pkg/store/matchers.go
Outdated
return m.Name + `!~"` + m.Value + `"` | ||
} | ||
|
||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe lets panic
here so that it would act as an assertion that this for sure cannot be reached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I am not sure whether there is a case to not match anything? Since the matchers parameters are passed from HTTP request, so it will validate the user input to ensure its validity
pkg/store/matchers.go
Outdated
return "{" + res + "}" | ||
} | ||
|
||
func labelMatcherToString(m storepb.LabelMatcher) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow... I didn't even find this is already implemented 😢 Thanks, I will switch to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a few things pretty good work! Thank you a lot!
547467b
to
80c1972
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think I am generally ok with this 👍
pkg/store/prometheus.go
Outdated
u.Path = path.Join(u.Path, "/api/v1/series") | ||
q := u.Query() | ||
|
||
metric, err := matchersToMetric(matchers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both are matchers, string vs protobuf, right? Can we name accordingly?
q.Add("start", string(startTime)) | ||
q.Add("end", string(endTime)) | ||
|
||
req, err := http.NewRequest("GET", u.String(), nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we need to do GET to be fully compatible with old pre 2.8.x Prometheus versions.
pkg/store/prometheus.go
Outdated
defer runutil.ExhaustCloseWithLogOnErr(p.logger, resp.Body, "series request body") | ||
|
||
if resp.StatusCode/100 != 2 { | ||
return nil, status.Error(codes.Internal, fmt.Sprintf("request Prometheus server failed, code %s", resp.Status)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, status.Error(codes.Internal, fmt.Sprintf("request Prometheus server failed, code %s", resp.Status)) | |
return nil, status.Errorf(codes.Internal, "request Prometheus server failed, code %s", resp.Status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still to do (:
pkg/store/storepb/rpc.proto
Outdated
@@ -92,6 +92,8 @@ message SeriesRequest { | |||
|
|||
// TODO(bwplotka): Move Thanos components to use strategy instead. Including QueryAPI. | |||
PartialResponseStrategy partial_response_strategy = 7; | |||
|
|||
bool no_chunk = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires a comment.
Overall are we happy with naming and this API field? Wonder how extendible this is. Overall having bool field to RPC is sometimes a code/design smell.
I think I am ok with this here as it's a caller decision to no skip chunks so maybe field should be named:
bool skip_chunks
orbool discard_chunks
? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer skip_chunks
. Do we need to make this as an enum? Currently I don't see some possible extensions of this field.
3b3c620
to
62882bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some comments, small nits, otherwise LGTM!
pkg/store/bucket_e2e_test.go
Outdated
MaxTime: maxt, | ||
MinTime: mint, | ||
MaxTime: maxt, | ||
SkipChunks: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false
statements we can skip as it's a default (:
pkg/store/matchers.go
Outdated
|
||
for _, m := range matchers { | ||
if m.Name == "__name__" { | ||
nameMatchers = append(nameMatchers, m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to separate those? I believe {__name__=..., ...}
form will work just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
pkg/store/prometheus.go
Outdated
defer runutil.ExhaustCloseWithLogOnErr(p.logger, resp.Body, "series request body") | ||
|
||
if resp.StatusCode/100 != 2 { | ||
return nil, status.Error(codes.Internal, fmt.Sprintf("request Prometheus server failed, code %s", resp.Status)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still to do (:
pkg/store/prometheus_test.go
Outdated
@@ -103,6 +104,7 @@ func testPrometheusStoreSeriesE2e(t *testing.T, prefix string) { | |||
Matchers: []storepb.LabelMatcher{ | |||
{Type: storepb.LabelMatcher_EQ, Name: "a", Value: "b"}, | |||
}, | |||
SkipChunks: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, false is a default
Signed-off-by: yeya24 <yb532204897@gmail.com> add e2e tests Signed-off-by: yeya24 <yb532204897@gmail.com> add tests for all stores Signed-off-by: yeya24 <yb532204897@gmail.com> add changelog Signed-off-by: yeya24 <yb532204897@gmail.com>
@bwplotka Thanks for your detailed review. I have addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks!
} | ||
|
||
for i, m := range matchers { | ||
res += m.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, we do strings.Join
but this is fine for now (:
Signed-off-by: yeya24 yb532204897@gmail.com
Changes
Fixes #1822 #1774
Add a no-chunk bool in series API to return labels instead of the whole series. Only in querier /api/v1/series API, this option will set to true, otherwise it set to false.
Verification
I have already added test cases for all stores.
For the response time using no-chunk, I tested in my local env.
Using Thanos v0.9.0
Using this PR