metrics: expose IA remote-read scan details#69606
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
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 IA remote read segment count, size, and wait-time observability across execution details, Prometheus metrics, slow-query logs, information schema, and statement summaries. ChangesIA Remote Read Segment Metrics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ScanDetail
participant QueryMetrics
participant SlowLog
participant StatementSummary
participant Prometheus
ScanDetail->>QueryMetrics: provide IA count, bytes, and wait time
QueryMetrics->>Prometheus: observe IA metrics
ScanDetail->>SlowLog: format IA fields
SlowLog->>StatementSummary: parse and expose IA columns
ScanDetail->>StatementSummary: aggregate sums and maxima
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
ti-chi-bot
Bot
added
the
size/XL
Connor1996
force-pushed
the
codex/tidb-ia-remote-read-observability
branch
from
cbda601 to
53462ff
Compare
ti-chi-bot
Bot
added
size/XXL
Connor1996
force-pushed
the
codex/tidb-ia-remote-read-observability
branch
2 times, most recently
from
9506fba to
d136bb6
Compare
ti-chi-bot
Bot
added
release-note
![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
🤖 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.
Inline comments:
In `@pkg/util/execdetails/execdetails.go`:
- Around line 213-231: The new IA remote-read fields are not available in the
current client-go version, so the added accessor in GetIARemoteReadSegmentStats
will not compile yet. Hold this change until the github.com/tikv/client-go/v2
bump lands, or guard/remove the references to
util.ScanDetail.IaRemoteReadSegmentCount, IaRemoteReadSegmentBytes, and
IaRemoteReadSegmentDuration so this package and its tests only use fields that
exist in the current dependency.
In `@pkg/util/stmtsummary/statement_summary.go`:
- Around line 202-207: The field naming is inconsistent across the statement
summary stats, where sumIAReadSegmentCount/maxIAReadSegmentCount omit “Remote”
while the related fields and upstream source use IARemoteReadSegment/
IaRemoteReadSegmentCount. Rename the struct fields in statement_summary.go and
update all matching references in reader.go and tables.go/v2 so the exported SQL
column becomes AVG_IA_REMOTE_READ_SEGMENT_COUNT (and related names stay
aligned). Use the existing sibling symbols like sumIARemoteReadSegmentSize,
maxIARemoteReadSegmentWaitTime, and GetIARemoteReadSegmentStats as the naming
reference when updating the public INFORMATION_SCHEMA.STATEMENTS_SUMMARY
columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9766269a-88a6-49f6-bd9b-cf863d6f288b
📒 Files selected for processing (21)
pkg/executor/slow_query.gopkg/executor/slow_query_test.gopkg/infoschema/metric_table_def.gopkg/infoschema/tables.gopkg/infoschema/test/clustertablestest/tables_test.gopkg/metrics/metrics.gopkg/metrics/server.gopkg/server/conn.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/tests/session_test.gopkg/util/execdetails/execdetails.gopkg/util/execdetails/execdetails_test.gopkg/util/stmtsummary/BUILD.bazelpkg/util/stmtsummary/evicted.gopkg/util/stmtsummary/reader.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/column.gopkg/util/stmtsummary/v2/column_test.gopkg/util/stmtsummary/v2/record.go
Connor1996
force-pushed
the
codex/tidb-ia-remote-read-observability
branch
from
d136bb6 to
a7faa73
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69606 +/- ##
================================================
- Coverage 76.3197% 75.8862% -0.4335%
================================================
Files 2041 2082 +41
Lines 560107 580295 +20188
================================================
+ Hits 427472 440364 +12892
- Misses 131734 137809 +6075
- Partials 901 2122 +1221
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Connor1996
force-pushed
the
codex/tidb-ia-remote-read-observability
branch
2 times, most recently
from
adf8b7a to
d463ca5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/util/stmtsummary/v2/record_test.go (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest only exercises
IaRemoteReadSegmentCount, notSizeorWaitTime.The count assertions are correct (Sum=3→6, Max=3 after merge), and the JSON key presence/absence checks are a good defensive measure. However, since only
IaRemoteReadSegmentCountis set on the ScanDetail, theSumIARemoteReadSegmentSize/MaxIARemoteReadSegmentSizeandSumIARemoteReadSegmentWaitTime/MaxIARemoteReadSegmentWaitTimepaths inNewStmtRecordandMerge(record.go lines 329–340, 547–558) are not validated with non-trivial values. Consider also settingIaRemoteReadSegmentSizeandIaRemoteReadSegmentWaitTimeon the fixture to cover those branches.Also applies to: 73-74, 91-92, 111-114
🤖 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/record_test.go` around lines 25 - 27, The test fixture only covers remote read segment count, leaving size and wait-time aggregation untested. In TestStmtRecord and the related cases, initialize ScanDetail.IaRemoteReadSegmentSize and IaRemoteReadSegmentWaitTime with non-zero values, then add or update Sum/Max assertions after NewStmtRecord and Merge to verify their aggregation and JSON output paths.pkg/util/stmtsummary/evicted_test.go (1)
379-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest only covers
IARemoteReadSegmentCount, notSizeorWaitTime.The merge math for count is correct (sum 8+8=16, max max(3,5)=5). However,
sumIARemoteReadSegmentSize,maxIARemoteReadSegmentSize,sumIARemoteReadSegmentWaitTime, andmaxIARemoteReadSegmentWaitTimeare left at zero in both fixtures, so the correspondingaddInfobranches (evicted.go lines 309–316) are not exercised with non-trivial values. Consider populating those fields to validate the size and wait-time merge logic.Also applies to: 479-480, 586-587
🤖 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/evicted_test.go` around lines 379 - 380, Extend the relevant fixtures in the evicted statement-summary tests, including the cases around the existing IARemoteReadSegmentCount fields and the additional referenced fixtures, by assigning non-zero values to sumIARemoteReadSegmentSize, maxIARemoteReadSegmentSize, sumIARemoteReadSegmentWaitTime, and maxIARemoteReadSegmentWaitTime. Update the expected merged results to verify the corresponding addInfo merge branches for size and wait time, not just count.
🤖 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/util/stmtsummary/evicted_test.go`:
- Around line 379-380: Extend the relevant fixtures in the evicted
statement-summary tests, including the cases around the existing
IARemoteReadSegmentCount fields and the additional referenced fixtures, by
assigning non-zero values to sumIARemoteReadSegmentSize,
maxIARemoteReadSegmentSize, sumIARemoteReadSegmentWaitTime, and
maxIARemoteReadSegmentWaitTime. Update the expected merged results to verify the
corresponding addInfo merge branches for size and wait time, not just count.
In `@pkg/util/stmtsummary/v2/record_test.go`:
- Around line 25-27: The test fixture only covers remote read segment count,
leaving size and wait-time aggregation untested. In TestStmtRecord and the
related cases, initialize ScanDetail.IaRemoteReadSegmentSize and
IaRemoteReadSegmentWaitTime with non-zero values, then add or update Sum/Max
assertions after NewStmtRecord and Merge to verify their aggregation and JSON
output paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37685cff-733d-401a-9eb5-bd5a37109414
📒 Files selected for processing (23)
pkg/executor/slow_query.gopkg/executor/slow_query_test.gopkg/infoschema/metric_table_def.gopkg/infoschema/tables.gopkg/infoschema/test/clustertablestest/tables_test.gopkg/metrics/metrics.gopkg/metrics/server.gopkg/server/conn.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/tests/session_test.gopkg/util/execdetails/execdetails.gopkg/util/execdetails/execdetails_test.gopkg/util/stmtsummary/BUILD.bazelpkg/util/stmtsummary/evicted.gopkg/util/stmtsummary/evicted_test.gopkg/util/stmtsummary/reader.gopkg/util/stmtsummary/statement_summary.gopkg/util/stmtsummary/statement_summary_test.gopkg/util/stmtsummary/v2/BUILD.bazelpkg/util/stmtsummary/v2/column.gopkg/util/stmtsummary/v2/column_test.gopkg/util/stmtsummary/v2/record.gopkg/util/stmtsummary/v2/record_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/util/stmtsummary/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/sessionctx/variable/tests/session_test.go
- pkg/util/execdetails/execdetails_test.go
- pkg/sessionctx/variable/slow_log.go
- pkg/util/stmtsummary/v2/BUILD.bazel
- pkg/metrics/server.go
- pkg/infoschema/tables.go
- pkg/util/stmtsummary/v2/column_test.go
- pkg/executor/slow_query.go
- pkg/server/conn.go
- pkg/util/stmtsummary/v2/record.go
- pkg/infoschema/metric_table_def.go
- pkg/metrics/metrics.go
- pkg/util/stmtsummary/reader.go
- pkg/util/execdetails/execdetails.go
- pkg/executor/slow_query_test.go
- pkg/util/stmtsummary/statement_summary_test.go
- pkg/util/stmtsummary/statement_summary.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, coocood, pingyu, 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 |
|
Please resolve the conflict |
Signed-off-by: Connor1996 <zbk602423539@gmail.com>
Connor1996
force-pushed
the
codex/tidb-ia-remote-read-observability
branch
from
d463ca5 to
486115f
Compare
5 participants
What problem does this PR solve?
Issue Number: ref #69624
Problem Summary:
TiDB does not currently expose IA remote-read scan detail fields in its own observability surfaces, so operators cannot inspect remote segment count, bytes, or wait time through slow logs, statement summary, or Prometheus metrics.
What changed and how does it work?
Adds TiDB observability plumbing for IA remote-read scan details:
INFORMATION_SCHEMA.SLOW_QUERYand statement summary columnsCheck 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
information_schemaviews, including average and maximum values.Bug Fixes