metric: add labels for sql.*.started.count
#151946
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
| StaticLabels: metric.MakeLabelPairs(metric.LabelQueryType, "started-select"), | ||
| Essential: true, | ||
| Category: metric.Metadata_SQL, | ||
| HowToUse: "This high-level metric reflects workload volume. Monitor this metric to identify abnormal application behavior or patterns over time. If abnormal patterns emerge, apply the metric's time range to the SQL Activity pages to investigate interesting outliers or patterns. For example, on the Transactions page and the Statements page, sort on the Execution Count column. To find problematic sessions, on the Sessions page, sort on the Transaction Count column. Find the sessions with high transaction counts and trace back to a user or application.", |
There was a problem hiding this comment.
Can you say anything more specific about started vs other metrics that would be helpful here?
I'm also not sure this metric is "essential". Most customers use the executed metrics to count QPS so we should justify why they should also pay attention to these.
There was a problem hiding this comment.
Good point. I searched Slack and found only 2 threads that explicitly mentioned using the started metrics for troubleshooting:
- https://cockroachlabs.slack.com/archives/C02119Q1G05/p1623169882143700?thread_ts=1623161302.135100&cid=C02119Q1G05
- https://cockroachlabs.slack.com/archives/C0713QV7XHP/p1714432731834549
Additionally, the started metrics was introduced in #37264, which also introduced the executed metrics as a claimed more useful one.
So maybe we should close it for now until receiving explicit ask for this feature?
There was a problem hiding this comment.
Removed the "Essential:true" field per offline discussion.
There was a problem hiding this comment.
I agree that "started" shouldn't be essential.
ZhouXing19
force-pushed
the
add-label-started
branch
from
68713da to
7b76724
Compare
Previously, only metrics for *executed* `SELECT|UPDATE|DELETE|INSERT` statements are labeled, while their *started* equivalent are not labeled. This commit is to label these metrics. Ref cockroachdb#143536 Release note (ops change): TBD <what was there before: Previously, ...> <why it needed to change: This was inadequate because ...> <what you did about it: To address this, this patch ...>
ZhouXing19
force-pushed
the
add-label-started
branch
from
7b76724 to
fc51076
Compare
| Help: "Number of SQL UPDATE statements started", | ||
| Measurement: "SQL Statements", | ||
| Unit: metric.Unit_COUNT, | ||
| LabeledName: "sql.count", |
There was a problem hiding this comment.
Sorry I was mistaken: these counts should not be aggregated along with other sql.count metrics because "started" and "executed" should not be added together.
This should be sql.started.count with label update for example.
There was a problem hiding this comment.
@yuzefovich reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @ZhouXing19)
-- commits line 10 at r2:
nit: this needs an update.
| StaticLabels: metric.MakeLabelPairs(metric.LabelQueryType, "started-select"), | ||
| Essential: true, | ||
| Category: metric.Metadata_SQL, | ||
| HowToUse: "This high-level metric reflects workload volume. Monitor this metric to identify abnormal application behavior or patterns over time. If abnormal patterns emerge, apply the metric's time range to the SQL Activity pages to investigate interesting outliers or patterns. For example, on the Transactions page and the Statements page, sort on the Execution Count column. To find problematic sessions, on the Sessions page, sort on the Transaction Count column. Find the sessions with high transaction counts and trace back to a user or application.", |
There was a problem hiding this comment.
I agree that "started" shouldn't be essential.
Previously, only metrics for executed
SELECT|UPDATE|DELETE|INSERTstatements are labeled, while their started equivalent are not labeled. This commit is to label these metrics. Since they are not frequently used metrics, they are not marked as Essential.Fixes: #152074
Release note (ops change):
sql.select.started.count,sql.insert.started.count,sql.update.started.count,sql.delete.started.countare now labeled withsql.count.