metrics, util/stmtsummary: add stmt summary window metrics#66700
Conversation
ti-chi-bot
Bot
added
the
release-note
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @jiong-nba. 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. |
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two Prometheus gauges for statement-summary window record and eviction counts, initializes and registers them, emits values from stmtsummary v2 during evictions/rotations, updates build dependencies/tests, and appends related panels to the Grafana dashboard. Changes
Sequence Diagram(s)sequenceDiagram
participant StmtSummary as StmtSummary(v2)
participant Metrics as Metrics package
participant Prometheus as Prometheus
participant Grafana as Grafana
StmtSummary->>StmtSummary: OnEvict -> increment evictedCount
StmtSummary->>StmtSummary: rotateLoop -> compute recordCount & reset old window
StmtSummary->>Metrics: updateMetrics(recordCount, evictedCount)
Metrics->>Prometheus: set gauges (tidb_stmt_summary_window_record_count, tidb_stmt_summary_window_evicted_count)
Grafana->>Prometheus: query metrics for dashboard panels
Prometheus-->>Grafana: return timeseries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds two new Prometheus gauge metrics for statement summary window observability in TiDB's V2 (persistent mode) statement summary:
tidb_stmt_summary_window_record_count: number of distinct statement digests in the current windowtidb_stmt_summary_window_evicted_count: number of LRU evictions in the current window
Changes:
- New
pkg/metrics/stmtsummary.godefines and initializes the two Prometheus gauges pkg/util/stmtsummary/v2/stmtsummary.goaddsevictedCounttostmtWindow, wires up the eviction callback, resets the counter onclear(), and reports both metrics every second inrotateLoop- New unit tests and Grafana dashboard panels for the new metrics; plus several new documentation/review files not mentioned in the PR description
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/metrics/stmtsummary.go |
New file: defines and initializes two Prometheus gauge metrics |
pkg/metrics/metrics.go |
Calls InitStmtSummaryMetrics() and registers the two new gauges |
pkg/metrics/BUILD.bazel |
Adds stmtsummary.go to the build library sources |
pkg/util/stmtsummary/v2/stmtsummary.go |
Adds evictedCount field, eviction callback, updateMetrics(), and clear() reset |
pkg/util/stmtsummary/v2/stmtsummary_test.go |
Adds assertions for eviction count and new TestWindowEvictedCountResetOnRotate and TestDefaultConfig tests |
pkg/util/stmtsummary/v2/BUILD.bazel |
Adds //pkg/metrics dependency and increases shard count |
pkg/metrics/grafana/tidb.json |
Adds two Grafana panels for the new metrics |
review.md |
Internal code review artifact (Chinese); should not be in the repository |
observability/*.md (5 files) |
Design analysis documents (Chinese); placed in non-standard directory per AGENTS.md |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
review.md (1)
72-89: Move PR-temporal “已修复” review log content out of permanent docs.This section reads like a one-off review log rather than stable repository documentation and will age quickly. Consider keeping this in PR/issue discussion and retaining only durable design/behavior facts in-repo.
As per coding guidelines, documentation updates should keep wording consistent and concrete, and avoid ambiguous/stale guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@review.md` around lines 72 - 89, The review log section ("4. Review 过程中发现并已修复的问题") is PR-temporal and should be removed from permanent docs; move this content into the PR or issue discussion and keep only durable, stable documentation in the repo. Specifically, remove or relocate the subsection entries referencing TestDefaultConfig/defaultRefreshInterval and the stmtWindow.clear()/evictedCount/TestStmtWindow fixes, and ensure the remaining docs contain only concrete, version-independent behavior or design notes (no one-off review logs or transient fix-by-fix history).observability/statement_summary_design.md (2)
179-179: Remove speculative wording and state a verified call path.“虽然 grep 没直接搜到，但通常在这里” is ambiguous and weakens trust in the doc. Either provide the confirmed caller/callee chain, or remove this parenthetical uncertainty.
As per coding guidelines, keep guidance executable and concrete; avoid ambiguous phrasing in documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability/statement_summary_design.md` at line 179, Remove the speculative parenthetical in the sentence referencing pkg/util/stmtsummary/reader.go and pkg/executor/infoschema_reader.go: either verify and document the exact caller/callee chain (e.g., confirm that infoschema_reader.go invokes reader.go and state the verified call path) or delete the parenthetical “(虽然 grep 没直接搜到，但通常在这里)” so the doc states only the confirmed relationship; update the line in statement_summary_design.md accordingly to be concrete and executable.
167-194: Add concrete metric usage for the two new window gauges.This PR introduces
tidb_stmt_summary_window_record_countandtidb_stmt_summary_window_evicted_count, but this section doesn’t show practical query/interpretation examples. Add a short “how to read these metrics” subsection (e.g., PromQL snippets and expected meaning under window pressure) so operators can act on it directly.Suggested doc patch
## 4. 数据暴露接口 @@ ### 4.3 诊断接口 PingCAP Dashboard 的 "SQL Statements" 功能并未直接查询 System Table，而是通过特定 HTTP API (`/status/statement`) 获取数据，底层也是复用了 `StmtSummary` 的数据结构。 + +### 4.4 Prometheus 指标（窗口观测） +针对窗口压力观测，新增以下指标： + +* `tidb_stmt_summary_window_record_count`：当前窗口中已存在的记录数。 +* `tidb_stmt_summary_window_evicted_count`：当前窗口内被驱逐的记录数。 + +可直接使用以下 PromQL： + +```promql +tidb_stmt_summary_window_record_count{instance="<tidb_instance>"} +``` + +```promql +tidb_stmt_summary_window_evicted_count{instance="<tidb_instance>"} +``` + +解读建议： +* `record_count` 持续接近容量上限且 `evicted_count` 持续上升，通常表示窗口容量不足或 SQL 指纹离散度过高。As per coding guidelines, keep guidance executable and concrete; avoid ambiguous phrasing in documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability/statement_summary_design.md` around lines 167 - 194, The docs are missing concrete, actionable examples for the two new metrics tidb_stmt_summary_window_record_count and tidb_stmt_summary_window_evicted_count; add a new "How to read these metrics" subsection under section 4 that includes the PromQL snippets (e.g., tidb_stmt_summary_window_record_count{instance="<tidb_instance>"} and tidb_stmt_summary_window_evicted_count{instance="<tidb_instance>"}), sample interpretation guidance (what sustained high record_count near capacity with rising evicted_count implies), and suggested alerting heuristics (e.g., sustained evicted_count increase or record_count near configured window size) so operators can immediately act on observed window pressure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@observability/Metrics` (Prometheus) 观测.md:
- Line 186: The doc references the wrong function name: replace mentions of
ResetMetrics with the actual entry point ToggleSimplifiedMode and ensure the
text refers to the unusedMetricsByGrafana list and its behavior under
ToggleSimplifiedMode (i.e., choosing not to register unused metrics in
simplified mode). Update any explanatory sentence that describes "ResetMetrics
logic" to instead describe the ToggleSimplifiedMode logic and its interaction
with unusedMetricsByGrafana so terminology matches pkg/metrics/metrics.go.
In `@observability/tracing_design.md`:
- Around line 170-171: Update the example command lines that use brace
placeholders to use angle-bracket placeholders for consistency: replace
"{tidb-ip}" with "<tidb-ip>" and "{status-port}" with "<status-port>" in the two
lines containing the curl example ("curl
http://{tidb-ip}:{status-port}/debug/pprof/trace?seconds=10 > trace.out") and
the following "go tool trace trace.out" example so the markdown shows explicit,
executable placeholders.
---
Nitpick comments:
In `@observability/statement_summary_design.md`:
- Line 179: Remove the speculative parenthetical in the sentence referencing
pkg/util/stmtsummary/reader.go and pkg/executor/infoschema_reader.go: either
verify and document the exact caller/callee chain (e.g., confirm that
infoschema_reader.go invokes reader.go and state the verified call path) or
delete the parenthetical “(虽然 grep 没直接搜到，但通常在这里)” so the doc states only the
confirmed relationship; update the line in statement_summary_design.md
accordingly to be concrete and executable.
- Around line 167-194: The docs are missing concrete, actionable examples for
the two new metrics tidb_stmt_summary_window_record_count and
tidb_stmt_summary_window_evicted_count; add a new "How to read these metrics"
subsection under section 4 that includes the PromQL snippets (e.g.,
tidb_stmt_summary_window_record_count{instance="<tidb_instance>"} and
tidb_stmt_summary_window_evicted_count{instance="<tidb_instance>"}), sample
interpretation guidance (what sustained high record_count near capacity with
rising evicted_count implies), and suggested alerting heuristics (e.g.,
sustained evicted_count increase or record_count near configured window size) so
operators can immediately act on observed window pressure.
In `@review.md`:
- Around line 72-89: The review log section ("4. Review 过程中发现并已修复的问题") is
PR-temporal and should be removed from permanent docs; move this content into
the PR or issue discussion and keep only durable, stable documentation in the
repo. Specifically, remove or relocate the subsection entries referencing
TestDefaultConfig/defaultRefreshInterval and the
stmtWindow.clear()/evictedCount/TestStmtWindow fixes, and ensure the remaining
docs contain only concrete, version-independent behavior or design notes (no
one-off review logs or transient fix-by-fix history).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 811b90ef-8509-4e51-aba0-b5fc5aa470a6
📒 Files selected for processing (14)
observability/Log_Analysis_Observability.mdobservability/Metrics (Prometheus) 观测.mdobservability/observability_summary.mdobservability/statement_summary_design.mdobservability/topsql_design.mdobservability/tracing_design.mdpkg/metrics/BUILD.bazelpkg/metrics/grafana/tidb.jsonpkg/metrics/metrics.gopkg/metrics/stmtsummary.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/stmtsummary.gopkg/util/stmtsummary/v2/stmtsummary_test.goreview.md
jiong-nba
force-pushed
the
feat/stmtsummary-window-metrics
branch
from
9531a65 to
6933ea0
Compare
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/XL
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66700 +/- ##
================================================
+ Coverage 77.7649% 77.8327% +0.0678%
================================================
Files 2019 1942 -77
Lines 553441 545260 -8181
================================================
- Hits 430383 424391 -5992
+ Misses 121316 120859 -457
+ Partials 1742 10 -1732
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jiong-nba
force-pushed
the
feat/stmtsummary-window-metrics
branch
from
6933ea0 to
bbb6368
Compare
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
1 similar comment
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/metrics/grafana/tidb.json`:
- Line 4648: Update the panel's "description" field (the JSON property currently
saying "The number of distinct statement digests currently tracked in the
statement summary LRU cache (V2 persistent mode only).") to accurately describe
that the metric is a window record count rather than the number of distinct
digests; reword to something like "The number of statement summary records
observed in the sampling window (record count over the time window; V2
persistent mode only)." Ensure you update the same "description" JSON entry for
the panel/metric so operators are not misled about semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b057fe1-4f1d-45a6-9b7a-765d4517fec6
📒 Files selected for processing (7)
pkg/metrics/BUILD.bazelpkg/metrics/grafana/tidb.jsonpkg/metrics/metrics.gopkg/metrics/stmtsummary.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/stmtsummary.gopkg/util/stmtsummary/v2/stmtsummary_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/metrics/BUILD.bazel
- pkg/metrics/metrics.go
- pkg/util/stmtsummary/v2/BUILD.bazel
- pkg/util/stmtsummary/v2/stmtsummary_test.go
- pkg/util/stmtsummary/v2/stmtsummary.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/util/stmtsummary/v2/stmtsummary_test.go (1)
75-98: Well-structured test for eviction count reset on rotation.Minor observation: Line 78
ss.SetMaxStmtCount(2)appears redundant sinceNewStmtSummary4Test(2)already sets the capacity to 2. Consider removing it to reduce test verbosity.♻️ Optional simplification
func TestWindowEvictedCountResetOnRotate(t *testing.T) { ss := NewStmtSummary4Test(2) defer ss.Close() - require.NoError(t, ss.SetMaxStmtCount(2)) // Fill the LRU cache and trigger evictions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/stmtsummary/v2/stmtsummary_test.go` around lines 75 - 98, The test TestWindowEvictedCountResetOnRotate calls SetMaxStmtCount(2) redundantly after creating the fixture with NewStmtSummary4Test(2); remove the ss.SetMaxStmtCount(2) invocation to simplify the test (leave NewStmtSummary4Test(2) as the source of capacity), keeping all other assertions and calls (ss.Add, ss.rotate, checks on ss.window.evictedCount and ss.window.lru.Size) unchanged.
🤖 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/util/stmtsummary/v2/stmtsummary_test.go`:
- Around line 75-98: The test TestWindowEvictedCountResetOnRotate calls
SetMaxStmtCount(2) redundantly after creating the fixture with
NewStmtSummary4Test(2); remove the ss.SetMaxStmtCount(2) invocation to simplify
the test (leave NewStmtSummary4Test(2) as the source of capacity), keeping all
other assertions and calls (ss.Add, ss.rotate, checks on ss.window.evictedCount
and ss.window.lru.Size) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa8ceae8-c3e5-4c00-a89d-f3c2539228e4
📒 Files selected for processing (2)
pkg/metrics/grafana/tidb.jsonpkg/util/stmtsummary/v2/stmtsummary_test.go
|
/retest |
|
@jiong-nba: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/retest |
|
@jiong-nba: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
jiong-nba
force-pushed
the
feat/stmtsummary-window-metrics
branch
from
dab8ec6 to
c7b5417
Compare
|
/retest |
|
@jiong-nba: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/retest |
1 similar comment
|
/retest |
… and Grafana panels
jiong-nba
force-pushed
the
feat/stmtsummary-window-metrics
branch
from
0f9b859 to
b10a777
Compare
| Help: "The number of LRU evictions in the current statement summary window.", | ||
| }, []string{LblType}) | ||
|
|
||
| stmtSummaryWindowRecordCountV1 = StmtSummaryWindowRecordCount.WithLabelValues(StmtSummaryTypeV1) |
There was a problem hiding this comment.
Is it intentional to pre-create both type=v1 and type=v2 gauge children here? Because the new dashboard queries do not filter or show type, this seems to produce two series per instance with the same {{instance}} legend, where the inactive implementation shows up as an extra zero line. Would it make sense to either avoid exporting the inactive child by default, or include/filter type in the dashboard panels?
| "steppedLine": false, | ||
| "targets": [ | ||
| { | ||
| "expr": "tidb_stmt_summary_window_record_count{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", instance=~\"$instance\"}", |
There was a problem hiding this comment.
Should we add the corresponding panels to pkg/metrics/nextgengrafana/tidb_with_keyspace_name.json as well? The underlying metrics are defined in shared pkg/metrics, and nextgen also exports them with keyspace_name as a const label, so updating only the classic dashboard seems to leave nextgen users without visibility for these new metrics.
ti-chi-bot
Bot
added
size/XXL
| "hide": false, | ||
| "intervalFactor": 2, | ||
| "legendFormat": "{{instance}}", | ||
| "legendFormat": "{{instance}} {{type}}", |
There was a problem hiding this comment.
Was the {{type}} suffix intended here (and in the similar 99p panel below)? This PromQL only groups by (le, instance), so the result series do not carry a type label. It looks unrelated to stmt summary and seems to turn the existing legends into instance + empty type.
| "dashLength": 10, | ||
| "dashes": false, | ||
| "datasource": "${DS_TEST-CLUSTER}", | ||
| "description": "The number of statement summary records currently tracked by statement summary.", |
There was a problem hiding this comment.
Could we move these two stmt summary panels into the Server row instead of KV Request? The classic dashboard places the same metrics under Server, and this nextgen dashboard still has its Server row ending much earlier, so adding them here looks more like a placement mistake than an intentional layout difference.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
Could you please add a screenshot of the local test results you ran for this PR? That would make the verification status much easier to confirm from the review thread. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nolouch The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
What problem does this PR solve?
Issue Number: close #66669
Problem Summary:
Enhancement
Add two statement summary observability metrics:
tidb_stmt_summary_window_record_counttidb_stmt_summary_window_evicted_countThe metrics now support both statement summary implementations and use a
typelabel to distinguishv1andv2.What changed and how does it work?
type=v1|v2.window_record_countreports current LRU size.window_evicted_countreports LRUonEvictcount in the current interval.Check List
Tests
Executed tests:
make bazel_preparebazel test --norun_validations --define gotags=deadlock,intest --remote_cache=https://cache.hawkingrei.com/bazelcache --noremote_upload_local_results //pkg/util/stmtsummary:stmtsummary_test --test_filter="TestStmtSummaryMetrics|TestStmtSummaryMetricsAfterCapacityChange|TestSetMaxStmtCountParallel|TestDisableStmtSummary"bazel test --norun_validations --define gotags=deadlock,intest --remote_cache=https://cache.hawkingrei.com/bazelcache --noremote_upload_local_results //pkg/util/stmtsummary/v2:stmtsummary_test --test_filter="TestWindowEvictedCountResetOnRotate|TestStmtWindow|TestStmtSummary|TestDefaultConfig"bazel test --norun_validations --define gotags=deadlock,intest --remote_cache=https://cache.hawkingrei.com/bazelcache --noremote_upload_local_results //pkg/metrics:metrics_test --test_filter="TestMetrics|TestRegisterMetrics|TestRetLabel|TestStmtSummaryMetricLabels"git diff --checkSide effects
Documentation
Release note
local test
stmt_summary_v1:
stmt_summary_v2:
