-
Notifications
You must be signed in to change notification settings - Fork 644
Add metrics for subscription queries #3661
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
Conversation
dd12d06 to
7b59fe4
Compare
We have some metrics measuring various lower level things like index scans, but at the moment we have no easy way to figure out which columns might need an index. This commit introduces three new metrics that provide that information by labeling count, latency, and number of rows canned along with the scan type (index scan, table scan, mixed scan) and info about unindexed columns.
7b59fe4 to
9cf4564
Compare
joshua-spacetime
left a comment
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.
If the goal is to be able to identify columns that should be indexed, the only operation you care about is full table scan -> filter. It's unclear to me what the purpose of all of the different scan strategies is, and how they're actionable.
|
@joshua-spacetime I guess I wanted to add scan type in case we want to use these metrics for something else, but couldn't a partial index scan also benefit from adding a compound index? Like, you have |
4eb3416 to
9cf4564
Compare
joshua-spacetime
left a comment
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.
Approved with a small comment.
Also, right now these metrics are not recorded for (add|remove)_single_subscription. I don't believe these methods are invoked from the sdk at all, so it's probably fine. I just wanted to make sure this was what you intended.
As far as performance goes, I don't see any problem adding these metrics. It's just a small amount of overhead added to an initial subscribe. And in terms of prometheus, we already have table-level metrics, so it won't explode cardinality.
e3477bb to
f18cb14
Compare
Description of Changes
We have some metrics measuring various lower level things like index scans, but at the moment we have no easy way to figure out which columns might need an index. This commit introduces three new metrics that provide that information by labeling count, latency, and number of rows canned along with the scan type (index scan, table scan, mixed scan) and info about unindexed columns.
API and ABI breaking changes
None
Expected complexity level and risk
2
I'm honestly not sure. I don't think it's overly complex, but it adds some overhead in the subscriptions initial query path.
Testing