sink: make sink statistics consistent and flush-aware - #5801
sink: make sink statistics consistent and flush-aware#58013AceShowHand wants to merge 14 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
the
size/M
|
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:
📝 WalkthroughWalkthroughThe PR moves sink statistics ownership into ChangesSink metrics and accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DMLSource
participant Sink
participant Writer
participant Statistics
DMLSource->>Sink: deliver DML event
Sink->>Statistics: TrackDMLEvent
Sink->>Writer: send or flush data
Writer->>Statistics: RecordBatchExecution after completion
Statistics->>Statistics: record original event bytes on PostFlush
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
size/XL
ti-chi-bot
Bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/statistics/statistics_test.go`:
- Around line 28-29: Update the statistics test setup and cleanup around
RecordBatchExecution to avoid resetting shared global metric state: use unique
label values for this test, register statistics.Close() with t.Cleanup so it
runs on assertion failure, and remove/assert only this test’s exact metric
series rather than requiring the entire collector to be empty. Preserve focused
deterministic assertions and follow the existing test guidance.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8b9cfa9-55ef-45dc-a7fc-fd8bff55d5e0
📒 Files selected for processing (15)
downstreamadapter/sink/blackhole/sink.godownstreamadapter/sink/cloudstorage/dml_writers.godownstreamadapter/sink/cloudstorage/sink.godownstreamadapter/sink/cloudstorage/writer.godownstreamadapter/sink/cloudstorage/writer_test.godownstreamadapter/sink/kafka/sink.godownstreamadapter/sink/mysql/sink.godownstreamadapter/sink/pulsar/sink.godownstreamadapter/sink/pulsar/sink_test.gopkg/metrics/statistics_test.gopkg/sink/mysql/mysql_writer.gopkg/sink/mysql/mysql_writer_ddl_ts_test.gopkg/sink/mysql/mysql_writer_test.gopkg/statistics/statistics.gopkg/statistics/statistics_test.go
💤 Files with no reviewable changes (1)
- pkg/metrics/statistics_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/sink/mysql/mysql_writer_ddl_ts_test.go
- downstreamadapter/sink/cloudstorage/writer.go
- downstreamadapter/sink/kafka/sink.go
- downstreamadapter/sink/pulsar/sink.go
- downstreamadapter/sink/cloudstorage/writer_test.go
- downstreamadapter/sink/pulsar/sink_test.go
- pkg/sink/mysql/mysql_writer_test.go
3AceShowHand
changed the title
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
downstreamadapter/sink/pulsar/sink_test.go (1)
127-134: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace the fixed sleep with an explicit bounded completion wait.
time.Sleep(1 * time.Second)does not synchronize with the asynchronous send/callback, so slow CI can still reach the assertions before completion while fast runs always pay an unnecessary delay. Wait on the callback or producer condition with a timeout, then assert the event counts.As per coding guidelines:
**/*_test.go: Prefer focused deterministic tests; see docs/agents/testing.md before adding or changing tests.🤖 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 `@downstreamadapter/sink/pulsar/sink_test.go` around lines 127 - 134, Replace the fixed time.Sleep in the test around ddlEvent2.PostFlush with an explicit bounded wait tied to the asynchronous send/callback or producer completion condition. Await completion with a timeout, then retain the existing producer event-count and count.Load assertions; do not add an unbounded wait or unrelated test changes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@downstreamadapter/sink/pulsar/sink_test.go`:
- Around line 127-134: Replace the fixed time.Sleep in the test around
ddlEvent2.PostFlush with an explicit bounded wait tied to the asynchronous
send/callback or producer completion condition. Await completion with a timeout,
then retain the existing producer event-count and count.Load assertions; do not
add an unbounded wait or unrelated test changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e46f93a-b863-4de8-8b21-902c084fcf9b
📒 Files selected for processing (7)
downstreamadapter/sink/pulsar/sink_test.gopkg/metrics/ddl.gopkg/metrics/init.gopkg/metrics/sink.gopkg/sink/mysql/mysql_writer_test.gopkg/statistics/metrics.gopkg/statistics/statistics.go
💤 Files with no reviewable changes (2)
- pkg/metrics/ddl.go
- pkg/sink/mysql/mysql_writer_test.go
|
/test all |
ti-chi-bot
Bot
added
release-note-none
|
/test all |
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 `@downstreamadapter/sink/cloudstorage/writer.go`:
- Around line 226-239: The batch execution accounting around
RecordBatchExecution must encompass the index-file flush as well as the
data-file write and close. Move the indexFilePath write into the executor
callback, or defer RecordBatchExecution until both writes succeed, ensuring
index-write failures return an error and increment the execution error metric
while post-flush callbacks run only after the complete flush.
In `@pkg/statistics/statistics.go`:
- Around line 73-75: Prevent dry-run MySQL flushes from incrementing
ticdc_sink_write_bytes_total: in TrackDMLEvent/PostFlush accounting, or the
corresponding Writer.Flush path in pkg/sink/mysql/mysql_writer.go (225-253),
gate event.GetSize() recording behind !w.cfg.DryRun. Preserve normal metric
accounting for non-dry-run writes; update the affected sites in
pkg/statistics/statistics.go (73-75) and pkg/sink/mysql/mysql_writer.go
(225-253) as needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bf0789f-b2e1-4bb1-87cd-fd1f28ec7556
📒 Files selected for processing (9)
downstreamadapter/sink/blackhole/sink.godownstreamadapter/sink/cloudstorage/writer.godownstreamadapter/sink/kafka/sink.godownstreamadapter/sink/mysql/sink.godownstreamadapter/sink/pulsar/sink.gopkg/sink/mysql/mysql_writer.gopkg/sink/mysql/mysql_writer_dml_exec.gopkg/statistics/statistics.gopkg/statistics/statistics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sink/mysql/mysql_writer_dml_exec.go
|
/test all |
1 participant
What problem does this PR solve?
Sink statistics were inconsistent across sink implementations:
typelabel split statistics for one changefeed into backend-specific series.pkg/metrics.Issue Number: close #5820
What is changed and how it works?
typelabel and obsolete duplicate sink metrics, and update the built-in Grafana dashboards.Statisticsand its private Prometheus collectors topkg/statistics; exposeNewandInitMetricsas the package entry points.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No performance regression is expected. The change adds constant-size bookkeeping and records metrics from existing completion callbacks.
Do you need to update user documentation, design documentation or monitoring documentation?
The built-in Grafana dashboards are updated. No user or design documentation change is required.
Release note
Summary by CodeRabbit