-
Notifications
You must be signed in to change notification settings - Fork 524
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 query results cache support for cardinality analysis API #5212
Conversation
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.
Looks good, nice work!
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
768a4a5
to
a71b0fc
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Thanks @pstibrany for your review. I should have addressed all your comments and the TODO that I left when you did the 1st review. There are two things I would prefer to do in follow up PRs:
|
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.
Looks good, thank you!
What this PR does
We're seeing some customers issuing the same cardinality analysis query over and over, few seconds apart from each other (likely coming from a script or internal tooling, given the user agent). These requests can be pretty heavy for the ingester, in terms of CPU utilization and memory allocations.
The cardinality analysis API, whose returned value is already a close-to-reality approximation today (but not a 100% accurate count), doesn't need to guarantee read-after-write and some short cache TTL could be applied to it. After discussing it with few people at Grafana Labs, in this PR I'm introducing the optional support to cache cardinality query results (disabled by default, TTL configurable on a per-tenant basis).
Note to reviewers:
cache.InstrumentedCacheMock
(PR)Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]