*: add statement summary for RUv2 and use float64 for calculations#67223
Conversation
Record and aggregate Request-Unit V2 (RUV2) metrics across statement summaries. Introduces StmtExecInfo.TotalRUV2 and a helper calculateStatementTotalRUV2 to compute total RUV2 from metrics and RUDetails. StmtRUSummary now tracks SumRUV2 and MaxRUV2 (with Add and Merge updated), and summary readers/columns (v1 and v2) expose AVG_REQUEST_UNIT_V2 and MAX_REQUEST_UNIT_V2. Info schema columns for avg/max RUV2 were added, and related tests and record generation were updated to include TotalRUV2. Signed-off-by: disksing <i@disksing.com>
Convert RU v2 related values from int64 to float64 across the codebase to preserve fractional RU values and avoid truncation. Key changes: RUV2 metrics CalculateRUValues/TotalRU now return float64; FormatRUV2Metrics prints floats with two decimals; runtime stats, slow log, resultset tracker, and statement summary fields updated to float64; info schema columns for RequestUnitV2 changed to DOUBLE; tests and summary reader/column factories adjusted accordingly. This ensures consistent handling and formatting of RUv2 totals (TiDB/TiKV/TiFlash) throughout the system. Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
the
release-note-none
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
Hi @disksing. 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:
📝 WalkthroughWalkthroughThis PR converts RU v2 accounting from integer types to float64 across metrics, reporting, formatting, and statement-summary plumbing; adds a statement-level TotalRUV2 field; and exposes AVG_REQUEST_UNIT_V2 / MAX_REQUEST_UNIT_V2 columns and factories. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
disksing
changed the title
disksing
requested review from
XuHuaiyu and
nolouch
and removed request for
XuHuaiyu
![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 (2)
pkg/util/stmtsummary/v2/record.go (1)
713-713: Use a fractionalTotalRUV2fixture value to better protect float64 behavior.Line 713 currently uses an integer-like value, which doesn’t exercise fractional RUv2 handling after the int64→float64 transition.
♻️ Suggested fixture tweak
- TotalRUV2: 12345, + TotalRUV2: 12345.67,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/stmtsummary/v2/record.go` at line 713, The TotalRUV2 fixture value is currently an integer-like literal which won't exercise fractional float64 behavior; update the struct literal field TotalRUV2 in pkg/util/stmtsummary/v2/record.go (the record/fixture where TotalRUV2 is set) to use a fractional value (for example 12345.67) so the float64 path is exercised after the int64→float64 transition.pkg/util/stmtsummary/v2/record_test.go (1)
66-67: Harden RUv2 float assertions and cover merged max value.For float64 aggregation checks, tolerant assertions reduce brittleness; also add a
MaxRUV2merge assertion to cover both aggregate dimensions.🧪 Suggested test refinement
- require.Equal(t, info.TotalRUV2, record1.MaxRUV2) - require.Equal(t, info.TotalRUV2, record1.SumRUV2) + require.InDelta(t, info.TotalRUV2, record1.MaxRUV2, 1e-9) + require.InDelta(t, info.TotalRUV2, record1.SumRUV2, 1e-9) ... - require.Equal(t, info.TotalRUV2*2, record2.SumRUV2) + require.InDelta(t, info.TotalRUV2*2, record2.SumRUV2, 1e-9) + require.InDelta(t, info.TotalRUV2, record2.MaxRUV2, 1e-9)Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/stmtsummary/v2/record_test.go` around lines 66 - 67, Replace brittle require.Equal float assertions in pkg/util/stmtsummary/v2/record_test.go with tolerant assertions (e.g., require.InDelta or require.InEpsilon) when comparing info.TotalRUV2 to record1.SumRUV2 and record1.MaxRUV2; specifically, change the comparisons that reference info.TotalRUV2 vs record1.SumRUV2 and the one against record1.MaxRUV2 to use a small delta/epsilon, and add an assertion that record1.MaxRUV2 matches the expected merged max (compare record1.MaxRUV2 to info.MaxRUV2) using the same tolerant matcher so the merged max dimension is covered.
🤖 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/util/execdetails/execdetails_test.go`:
- Around line 363-370: The test uses type-strict require.Equal on RU floats and
mismatched string formatting; update the assertions to compare floats (convert
expected ints to float64 or use require.InEpsilon/require.EqualValues) for
tidbRU (from metrics.CalculateRUValues) and totalRU (from metrics.TotalRU), and
adjust TestFormatRUV2MetricsIncludesRUValuesFirst expected output strings to
match FormatRUV2Metrics' "%.2f" formatting (i.e., include ".00" decimal
suffixes) so the string comparisons align with the float formatting.
In `@pkg/util/execdetails/ruv2_metrics.go`:
- Around line 437-439: Update the CalculateRUValues doc comment on RUV2Metrics
to accurately describe its behavior and return type: state that
CalculateRUValues(weights RUV2Weights) computes and returns the current TiDB RU
as a float64 (not a scaled integer), mention that the argument is RUV2Weights
used to weight components, and keep the comment as an exported-symbol doc
comment directly above the function declaration for clarity and tooling.
---
Nitpick comments:
In `@pkg/util/stmtsummary/v2/record_test.go`:
- Around line 66-67: Replace brittle require.Equal float assertions in
pkg/util/stmtsummary/v2/record_test.go with tolerant assertions (e.g.,
require.InDelta or require.InEpsilon) when comparing info.TotalRUV2 to
record1.SumRUV2 and record1.MaxRUV2; specifically, change the comparisons that
reference info.TotalRUV2 vs record1.SumRUV2 and the one against record1.MaxRUV2
to use a small delta/epsilon, and add an assertion that record1.MaxRUV2 matches
the expected merged max (compare record1.MaxRUV2 to info.MaxRUV2) using the same
tolerant matcher so the merged max dimension is covered.
In `@pkg/util/stmtsummary/v2/record.go`:
- Line 713: The TotalRUV2 fixture value is currently an integer-like literal
which won't exercise fractional float64 behavior; update the struct literal
field TotalRUV2 in pkg/util/stmtsummary/v2/record.go (the record/fixture where
TotalRUV2 is set) to use a fractional value (for example 12345.67) so the
float64 path is exercised after the int64→float64 transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d7c91117-35c8-4d5c-af74-cb513e0cc61b
📒 Files selected for processing (14)
pkg/executor/adapter.gopkg/executor/explain_unit_test.gopkg/infoschema/tables.gopkg/server/internal/resultset/resultset.gopkg/sessionctx/variable/slow_log.gopkg/util/execdetails/execdetails_test.gopkg/util/execdetails/runtime_stats.gopkg/util/execdetails/ruv2_metrics.gopkg/util/stmtsummary/reader.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/column.gopkg/util/stmtsummary/v2/record.gopkg/util/stmtsummary/v2/record_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67223 +/- ##
================================================
+ Coverage 77.7535% 79.7652% +2.0116%
================================================
Files 2022 1968 -54
Lines 554209 545476 -8733
================================================
+ Hits 430917 435100 +4183
+ Misses 121550 108913 -12637
+ Partials 1742 1463 -279
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Fix float comparisons in TestRUV2MetricsSnapshotCalculateRUValues by using require.InEpsilon instead of require.Equal for float64 values - Fix expected string values in TestFormatRUV2MetricsIncludesRUValuesFirst to match FormatRUV2Metrics' "%.2f" formatting (include decimal suffixes) - Fix CalculateRUValues doc comment to accurately describe return type as float64 instead of incorrect "scaled integer" Signed-off-by: disksing <i@disksing.com>
Update expected string in TestSlowLogFormatIncludesTiFlashRUInRUV2Metrics to match FormatRUV2Metrics' "%.2f" formatting (include ".00" decimal suffixes) for total_ru, tidb_ru, tikv_ru, and tiflash_ru values. Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
XuHuaiyu
left a comment
XuHuaiyu
left a comment
There was a problem hiding this comment.
Reviewed the latest head again. The follow-up commits fixed the test/assertion issues and updated the doc comment. I reran the targeted tests for the updated cases locally and they passed.\n\nThe remaining cursor stmt-summary RUv2 undercount is acknowledged in-thread and intentionally kept consistent with current RU v1 behavior for this PR. No other issues from my side.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Fix linter warning: unnecessary conversion (unconvert) at line 1732 Signed-off-by: disksing <i@disksing.com>
Fix linter warning: unnecessary conversion (unconvert) at line 497 Signed-off-by: disksing <i@disksing.com>
Fix linter warning: unnecessary conversion (unconvert) at line 203 Signed-off-by: disksing <i@disksing.com>
|
/test unit-test |
|
@disksing: The specified target(s) for Use 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nolouch, wjhuang2016, XuHuaiyu 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 |
|
/test check_dev |
|
@disksing: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
@disksing: The specified target(s) for Use 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. |
|
/test check-dev |
|
@disksing: The specified target(s) for Use 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. |
5 participants
What problem does this PR solve?
Issue Number: ref #67199
Problem Summary:
Add RUv2 totals to statement summaries and use float64 for RUv2 metrics calculations to improve precision.
What changed and how does it work?
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
Improvements
Tests