release-8.5: cherry-pick improments of statement summary coverage#69129
Open
nolouch wants to merge 3 commits into
Open
release-8.5: cherry-pick improments of statement summary coverage#69129nolouch wants to merge 3 commits into
nolouch wants to merge 3 commits into
Conversation
close pingcap#68865 (cherry picked from commit 279a933)
ref pingcap#67924 (cherry picked from commit 2c307d6)
ref pingcap#67924 (cherry picked from commit 6c43104)
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
📝 WalkthroughWalkthroughThis PR integrates Prometheus metrics into statement summary tracking, implements per-record eviction persistence with async logging, and enables optional grouping by executing user. The changes span v1 and v2 statement summary backends, system variables, session state handling, and comprehensive test coverage. ChangesStatement Summary Metrics and Persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
nolouch
changed the title
ti-chi-bot
Bot
added
release-note
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69129 +/- ##
================================================
Coverage ? 55.0817%
================================================
Files ? 1847
Lines ? 665676
Branches ? 0
================================================
Hits ? 366666
Misses ? 271655
Partials ? 27355
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/metrics/stmtsummary.go (1)
91-106: ⚡ Quick winCombine duplicate switch cases for v1 and v2.
The v1 and v2 cases execute identical code (three lines each). Combine them into a single case using Go's multi-case syntax.
♻️ Refactor to eliminate duplication
func SetStmtSummaryWindowMetrics(typ string, recordCount, evictedCount float64) { switch typ { - case StmtSummaryTypeV1: - recordGauge, evictedGauge := getStmtSummaryWindowMetricsLocked(typ) - recordGauge.Set(recordCount) - evictedGauge.Set(evictedCount) - case StmtSummaryTypeV2: + case StmtSummaryTypeV1, StmtSummaryTypeV2: recordGauge, evictedGauge := getStmtSummaryWindowMetricsLocked(typ) recordGauge.Set(recordCount) evictedGauge.Set(evictedCount) default:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/stmtsummary.go` around lines 91 - 106, The switch in SetStmtSummaryWindowMetrics duplicates the same logic for StmtSummaryTypeV1 and StmtSummaryTypeV2; combine them into a single multi-case branch (case StmtSummaryTypeV1, StmtSummaryTypeV2:) that calls getStmtSummaryWindowMetricsLocked(typ) and sets recordGauge and evictedGauge, leaving the default branch that uses StmtSummaryWindowRecordCount.WithLabelValues(typ).Set(...) and StmtSummaryWindowEvictedCount.WithLabelValues(typ).Set(...) unchanged.pkg/util/stmtsummary/v2/stmtsummary.go (1)
856-876: ⚡ Quick winConsider making the dual-backend setter more robust to future error scenarios.
SetGroupByUserapplies the setting to both v1 and v2 backends sequentially. If v1 succeeds but v2 fails (or vice versa), the two backends will have inconsistent grouping modes. Currently both methods always returnnil, so this doesn't happen in practice, but future changes to either implementation could introduce real errors and cause subtle bugs.♻️ Proposed refactor to collect both errors
func SetGroupByUser(v bool) error { - if err := stmtsummary.StmtSummaryByDigestMap.SetGroupByUser(v); err != nil { - return err - } + var errs []error + if err := stmtsummary.StmtSummaryByDigestMap.SetGroupByUser(v); err != nil { + errs = append(errs, fmt.Errorf("v1: %w", err)) + } if GlobalStmtSummary != nil { - return GlobalStmtSummary.SetGroupByUser(v) + if err := GlobalStmtSummary.SetGroupByUser(v); err != nil { + errs = append(errs, fmt.Errorf("v2: %w", err)) + } + } + if len(errs) > 0 { + return errors.Join(errs...) } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/stmtsummary/v2/stmtsummary.go` around lines 856 - 876, SetGroupByUser currently calls stmtsummary.StmtSummaryByDigestMap.SetGroupByUser and GlobalStmtSummary.SetGroupByUser sequentially but returns early on the first error, risking inconsistent state; change it to call both targets unconditionally, capture any errors from stmtsummary.StmtSummaryByDigestMap.SetGroupByUser and (if GlobalStmtSummary != nil) GlobalStmtSummary.SetGroupByUser, and return a combined error (e.g., concatenate messages or wrap both using fmt.Errorf or a multi-error helper) so callers see all failures while both backends are attempted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/metrics/stmtsummary.go`:
- Around line 91-106: The switch in SetStmtSummaryWindowMetrics duplicates the
same logic for StmtSummaryTypeV1 and StmtSummaryTypeV2; combine them into a
single multi-case branch (case StmtSummaryTypeV1, StmtSummaryTypeV2:) that calls
getStmtSummaryWindowMetricsLocked(typ) and sets recordGauge and evictedGauge,
leaving the default branch that uses
StmtSummaryWindowRecordCount.WithLabelValues(typ).Set(...) and
StmtSummaryWindowEvictedCount.WithLabelValues(typ).Set(...) unchanged.
In `@pkg/util/stmtsummary/v2/stmtsummary.go`:
- Around line 856-876: SetGroupByUser currently calls
stmtsummary.StmtSummaryByDigestMap.SetGroupByUser and
GlobalStmtSummary.SetGroupByUser sequentially but returns early on the first
error, risking inconsistent state; change it to call both targets
unconditionally, capture any errors from
stmtsummary.StmtSummaryByDigestMap.SetGroupByUser and (if GlobalStmtSummary !=
nil) GlobalStmtSummary.SetGroupByUser, and return a combined error (e.g.,
concatenate messages or wrap both using fmt.Errorf or a multi-error helper) so
callers see all failures while both backends are attempted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34943a00-e56f-40b1-b4b6-6ab613137f29
📒 Files selected for processing (20)
pkg/executor/select.gopkg/metrics/BUILD.bazelpkg/metrics/metrics.gopkg/metrics/metrics_internal_test.gopkg/metrics/stmtsummary.gopkg/sessionctx/sessionstates/session_states_test.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gopkg/util/stmtsummary/BUILD.bazelpkg/util/stmtsummary/evicted_test.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/logger.gopkg/util/stmtsummary/v2/reader.gopkg/util/stmtsummary/v2/reader_test.gopkg/util/stmtsummary/v2/record_test.gopkg/util/stmtsummary/v2/stmtsummary.gopkg/util/stmtsummary/v2/stmtsummary_test.go
|
@nolouch: 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. |
terry1purcell
approved these changes
Jun 12, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terry1purcell, yibin87 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #66700
This is an automated cherry-pick of #68868
This is an automated cherry-pick of #68512
This is an automated cherry-pick of #68513
What problem does this PR solve?
Issue Number: close #68865, close #66669, ref #67924
Problem Summary:
Backport statement-summary improvements and the prerequisite ROW_COUNT restore fix to
release-8.5.SET SESSION_STATEScould restoreROW_COUNT()from a previousSELECTas current affected rows, polluting affected-row metrics and statement summary records.release-8.5.What changed and how does it work?
release-8.5required stmt-summary metrics pieces from metrics, util/stmtsummary: add stmt summary window metrics #66700, includingpkg/metrics/stmtsummary.go, window record/evicted metrics, and v2 eviction-count reporting.LastAffectedRowsinto previous affected rows instead of current statement affected rows.tidb_stmt_summary_group_by_userfor optional user-dimension grouping.tidb_stmt_summary_persist_evictedfor optional per-record persistence of v2 LRU-evicted statement summaries.release-8.5code layout, including sysvar definitions underpkg/sessionctx/variable/tidb_vars.goand local metrics wrappers.Check List
Tests
Manual test:
GOCACHE=/private/tmp/tidb-go-build-stmt-persist go test ./pkg/metrics -run "Test(StmtSummaryMetricLabels|RetLabel)"GOCACHE=/private/tmp/tidb-go-build-stmt-persist go test ./pkg/util/stmtsummary/v2 -run "Test(StmtSummaryPersistEvicted|StmtSummaryPersistEvictedDoesNotPersistLoggedRecordsAsAggregate|WindowEvictedCountResetOnRotate|StmtRecord)"GOCACHE=/private/tmp/tidb-go-build-stmt-persist go test ./pkg/sessionctx/variable -run "^$"git diff --checkSide effects
When
tidb_stmt_summary_group_by_useris ON, statement summary cardinality can grow by distinct users per digest. Whentidb_stmt_summary_persist_evictedis ON, stmt summary log volume grows with eviction rate. Both variables are default OFF.Documentation
Release note