planner: add slow query QPS metric and show it in TiDB QPS panel#67249
planner: add slow query QPS metric and show it in TiDB QPS panel#67249qw4990 wants to merge 3 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note-none
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/M
|
Hi @qw4990. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a slow-query counter metric and wires it through server metrics, executor increments (internal vs. general), metric table definition, Grafana panels, and tests to expose slow-query QPS. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/adapter_test.go (1)
417-433: Extend this test to cover the internal SQL counter path too.The new assertions validate only
sql_type=general. Since production code now increments both general and internal counters, please add an internal-SQL assertion in this test (or companion test) to cover the restricted branch as well.✅ Minimal extension sketch
- readSlowQueryCounter := func() float64 { - counter := metrics.SlowQueryCounter.WithLabelValues(metrics.LblGeneral) + readSlowQueryCounter := func(sqlType string) float64 { + counter := metrics.SlowQueryCounter.WithLabelValues(sqlType) pb := &dto.Metric{} require.NoError(t, counter.Write(pb)) return pb.GetCounter().GetValue() } + // existing general-path checks - before := readSlowQueryCounter() + before := readSlowQueryCounter(metrics.LblGeneral) tk.MustExec(sql) - after := readSlowQueryCounter() + after := readSlowQueryCounter(metrics.LblGeneral) + // add internal-path check (example) + beforeInternal := readSlowQueryCounter(metrics.LblInternal) + rs, err := tk.Session().ExecuteInternal(context.Background(), sql) + require.NoError(t, err) + _, err = session.ResultSetToStringSlice(context.Background(), tk.Session(), rs) + require.NoError(t, err) + afterInternal := readSlowQueryCounter(metrics.LblInternal) + require.Equal(t, 1.0, afterInternal-beforeInternal)Based on learnings: "For SQL behavior changes in executor, perform targeted unit test plus relevant integration test."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/adapter_test.go` around lines 417 - 433, The test currently only reads the general slow-query metric; extend it to also assert the internal-SQL counter path by adding a reader for the internal label (e.g., add a function similar to readSlowQueryCounter that accepts or uses metrics.LblInternal and calls metrics.SlowQueryCounter.WithLabelValues(metrics.LblInternal)), then update or add assertions in checkWriteSlowLog (or create checkWriteSlowLogInternal) to capture the before/after delta for the internal counter and validate expected increments when expectWrite is true and no change when false; reference readSlowQueryCounter, checkWriteSlowLog, metrics.SlowQueryCounter, metrics.LblInternal to find where to add the new reader and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/adapter_test.go`:
- Around line 417-433: The test currently only reads the general slow-query
metric; extend it to also assert the internal-SQL counter path by adding a
reader for the internal label (e.g., add a function similar to
readSlowQueryCounter that accepts or uses metrics.LblInternal and calls
metrics.SlowQueryCounter.WithLabelValues(metrics.LblInternal)), then update or
add assertions in checkWriteSlowLog (or create checkWriteSlowLogInternal) to
capture the before/after delta for the internal counter and validate expected
increments when expectWrite is true and no change when false; reference
readSlowQueryCounter, checkWriteSlowLog, metrics.SlowQueryCounter,
metrics.LblInternal to find where to add the new reader and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc8ae555-a8f3-4b25-bf5d-7da4b44bfffb
📒 Files selected for processing (7)
pkg/executor/adapter.gopkg/executor/adapter_test.gopkg/executor/metrics/metrics.gopkg/infoschema/metric_table_def.gopkg/metrics/grafana/tidb.jsonpkg/metrics/metrics.gopkg/metrics/server.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67249 +/- ##
================================================
+ Coverage 77.7724% 79.2846% +1.5122%
================================================
Files 2022 1968 -54
Lines 554420 545905 -8515
================================================
+ Hits 431186 432819 +1633
+ Misses 121492 111753 -9739
+ Partials 1742 1333 -409
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AilinKid, GMHDBJD, guo-shaoge The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
| "refId": "B" | ||
| }, | ||
| { | ||
| "expr": "sum(rate(tidb_server_slow_query_total{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", instance=~\"$instance\"}[1m]))", |
There was a problem hiding this comment.
Do we need to add it to tidb_with_keyspace_name.json?
|
@qw4990: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
What problem does this PR solve?
Issue Number: close #67247
Problem Summary: planner: add slow query QPS metric and show it in TiDB QPS panel
What changed and how does it work?
tidb_server_slow_query_total(label:sql_type).METRICS_SCHEMAviatidb_slow_query_qps.pkg/metrics/grafana/tidb.jsonQPS panel to include a SlowQuery series:sum(rate(tidb_server_slow_query_total{...}[1m]))Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests