-
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
queryserving, observability: instrument vttablet query cache plan hits/misses #14947
queryserving, observability: instrument vttablet query cache plan hits/misses #14947
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14947 +/- ##
==========================================
+ Coverage 67.41% 67.44% +0.02%
==========================================
Files 1560 1561 +1
Lines 192752 193194 +442
==========================================
+ Hits 129952 130301 +349
- Misses 62800 62893 +93 ☔ View full report in Codecov by Sentry. |
changelog/19.0/19.0.0/summary.md
Outdated
#### <a id="vttablet-query-cache-hits-and-misses"/>VTTablet Query Cache Hits and Misses | ||
|
||
VTTablet exposes two counter stats: | ||
|
||
* `QueryCacheHits`: Query engine query cache hits | ||
* `QueryCacheMisses`: Query engine query cache misses | ||
|
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 should go in the changelog/20.0/20.0.0/summary.md
summary file, if it does not exist yet you can create 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.
Also a small nit pick, reading this it seems like VTTablet is only exposing these two counter stats now, maybe add "new" or something
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 but the release notes must be changed. 🥳
…s/misses Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
Signed-off-by: Max Englander <[email protected]>
33ae74f
to
5b81f8c
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.
Still LGTM
…-plan-cache-miss Signed-off-by: Florent Poinsard <[email protected]>
Conflicts are resolved, should be merged soon! |
Description
Add stats counters:
QueryCacheHits
: Query engine query cache hitsQueryCacheMisses
: Query engine query cache missesA query plan cache miss results in VTTablet parsing the query. This can add a significant amount of latency to a query execution. Having better visibility into query plan caching will help isolate or eliminate cache misses as a source of latency when analyzing query performance.
Related Issue(s)
Fixes #14946
Checklist
Deployment Notes