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

Add query results cache support for cardinality analysis API #5212

Merged
merged 8 commits into from
Jun 9, 2023

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Jun 9, 2023

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:

  • dskit has been updated to get a couple of more methods to cache.InstrumentedCacheMock (PR)
  • I manually tested it and looks doing its job

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci pracucci requested a review from pstibrany June 9, 2023 03:16
Copy link
Member

@pstibrany pstibrany left a 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!

pkg/frontend/querymiddleware/cardinality_query_cache.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/cardinality_query_cache.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/cardinality_query_cache.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/model.proto Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/model.proto Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/model_extra.go Show resolved Hide resolved
pkg/frontend/querymiddleware/cardinality_query_cache.go Outdated Show resolved Hide resolved
pkg/querier/cardinality/request.go Outdated Show resolved Hide resolved
pkg/querier/cardinality/request.go Outdated Show resolved Hide resolved
pkg/querier/cardinality/request.go Outdated Show resolved Hide resolved
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>
@pracucci pracucci force-pushed the add-query-results-cache-for-cardinality-api branch from 768a4a5 to a71b0fc Compare June 9, 2023 10:10
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review June 9, 2023 10:42
@pracucci pracucci requested review from a team as code owners June 9, 2023 10:42
@pracucci
Copy link
Collaborator Author

pracucci commented Jun 9, 2023

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:

  1. Improve HTTP API doc (since will require docs squad review, I prefer to not block this PR)
  2. Try to improve metrics we have to compute the query results cache hit ratio (right now we can't distinguish between range and cardinality queries, but this will impact dashboard too, so I prefer to keep it separated)

Copy link
Member

@pstibrany pstibrany left a 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!

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.

2 participants