Cache labels and series results#3315
Conversation
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
This looks amazing ❤️
Some high-level comments, but overall LGTM! (:
| case *ThanosSeriesRequest: | ||
| return fmt.Sprintf("%s:%d", tr.Matchers, currentInterval) | ||
| } | ||
| return fmt.Sprintf("%s:%d:%d", r.GetQuery(), r.GetStep(), currentInterval) |
There was a problem hiding this comment.
I am not sure but this shouldn't happen. It would error out in the first place when decoding the HTTP request if the request is not valid.
CHANGELOG.md
Outdated
|
|
||
| - [#3261](https://github.com/thanos-io/thanos/pull/3261) Thanos Store: Use segment files specified in meta.json file, if present. If not present, Store does the LIST operation as before. | ||
| - [#3276](https://github.com/thanos-io/thanos/pull/3276) Query Frontend: Support query splitting and retry for labels and series requests. | ||
| - [#3315](https://github.com/thanos-io/thanos/pull/3315) Query Frontend: Support results caching for labels and series requests. |
There was a problem hiding this comment.
label names ? label values? both?
There was a problem hiding this comment.
Yes both. I will make it more clear.
| for ; i < len(t.resolutions) && t.resolutions[i] > tr.MaxSourceResolution; i++ { | ||
| } | ||
| return fmt.Sprintf("%s:%d:%d:%d", tr.Query, tr.Step, currentInterval, i) | ||
| case *ThanosLabelsRequest: |
There was a problem hiding this comment.
ThanosLabelsRequest already covers both label_values query and label_names query. If it is a label_names query, then tr.Label is just an empty string. For label_values query, that's the queried label name.
I am using the same struct for them since their query response is the same []string
| // GetStep returns the step of the request in milliseconds. | ||
| func (r *ThanosLabelsRequest) GetStep() int64 { return 0 } | ||
| // GetStep returns the step of the request in milliseconds. Returns 1 is a trick to avoid panic in | ||
| // https://github.com/cortexproject/cortex/blob/master/pkg/querier/queryrange/results_cache.go#L447. |
There was a problem hiding this comment.
(: Yea it's getting very tricky without Cortex actually support our case, and it would be nice to help them as well 🤔
This is fine for now, but wonder how long term solution of truly shared code with cortex would looks like
There was a problem hiding this comment.
Yeah, I will take a look later. Would be good to have these query APIs natively supported on Cortex then we can re-use.
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
force-pushed
the
cache-labelseries
branch
from
97eb6cd to
651b1bd
Compare
|
Conflict fixed. Please give another round of review if you have time. |
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
LGTM. Great work. I think we can go ahead and merge it. We can open follow up PRs if needed.
3 participants
Signed-off-by: Ben Ye yb532204897@gmail.com
Changes
Fixes #3230
Verification