-
Notifications
You must be signed in to change notification settings - Fork 470
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
store-gateway: add postings shortcutting for LabelValues #4789
Conversation
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
This reverts commit fe9b3cd.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
This PR also makes the existing |
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.
Fantastic job, as usual! You set the bar very high 🚀 I found a bit difficult to read the code because we have both "label values postings" and "matchers postings". I left some nit comments to try to clarify it with a bit of renaming.
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
} | ||
|
||
func (w labelValuesPostingsStrategy) name() string { | ||
return "lv-" + w.delegate.name() |
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 something which isn't directly obvious - this lv-
prefix means that we will cache expanded postings for LabelValues and Series under different keys. The premise is that the tradeoff for the two may be different for the same request matchers - Series may have pending matchers, LabelValues may not (see TestLabelValuesPostingsStrategy
).
We can potentially cache the expanded postings when they are fully resolved from LabelValues under the Series entry as well. I'm not sure if this is worth pursuing. I'm also not sure how to measure its potential impact.
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 didn't realise, but I don't think it's a problem. I think it's fine caching them separately.
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
8e8da21
to
226cad6
Compare
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Still LGTM ;) |
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
What this PR does
In a related fashion to what #4698 did for Series this PR adds some heuristics to reduce the resources used in LabelValues calls. The general flow is as follows
labelValuesPostingsStrategy
. This strategy makes a decision between three optionsThe strategy uses the configured regular strategy for Series calls to decide between A) and C). Deciding between (A or C) and B happens in a similar fashion to the decision in
worstCaseFetchedDataStrategy
: assuming 1 byte of series == 1 byte of postings.Benchmarks
I expected that decision C will be the most significant one in making a difference, but it seems that most of the optimizations happen because of choosing decision A over B. In fact none of the existing benchmarks showed an improvement when I eliminated decision A as an option.
BenchmarkBucketStoreLabelValues with options to choose from A, B or C
BenchmarkBucketStoreLabelValues with options to choose from B or C
Which issue(s) this PR fixes or relates to
related to #4593
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]