metrics(dm): ignore binlog gap for non-running tasks - #12766
Conversation
Join the master-to-syncer binlog gap alert with worker task state so paused or stopped tasks do not raise misleading alerts.
ti-chi-bot
Bot
added
first-time-contributor
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe binlog gap alert now requires a running DM worker task. A Go test loads the YAML rule and validates its expression, label expression, and duration. ChangesDM worker alert behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Run the alert rule regression check as part of the normal DM Go unit tests without downloading an external promtool binary.
![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 `@dm/metrics/alertmanager/dm_worker_rules_test.go`:
- Around line 44-50: Extend TestBinlogFileGapAlertRuleRequiresRunningTask beyond
metadata checks by evaluating dm_worker.rules.yml with representative running
and paused task samples. Assert that the alert fires for running tasks and does
not fire for paused tasks, while preserving the existing expression, label, and
duration assertions; alternatively, restore the prior promtool coverage.
🪄 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: f2e426df-7cff-4741-9df4-527956731a71
📒 Files selected for processing (1)
dm/metrics/alertmanager/dm_worker_rules_test.go
ti-chi-bot
Bot
added
size/XL
|
/retest-required |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 2
- Inline comments: 2
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (1)
- Test-only PromQL coverage changes repository-wide production dependencies (
dm/metrics/alertmanager/dm_worker_rules_test.go:21; go.mod:11; go.mod:56; go.mod:82; pkg/util/memory.go:34; cdc/sink/dmlsink/mq/dmlproducer/pulsar_dml_producer.go:99; cdc/sink/ddlsink/mq/ddlproducer/pulsar_ddl_producer.go:121; cdc/sink/dmlsink/txn/mysql/mysql.go:158)
🟡 [Minor] (1)
- One-second wall-clock query deadline makes the rule test load-sensitive (
dm/metrics/alertmanager/dm_worker_rules_test.go:55)
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/prometheus/prometheus/promql" |
There was a problem hiding this comment.
⚠️ [Major] Test-only PromQL coverage changes repository-wide production dependencies
Why
The new alert-rule test imports the full Prometheus PromQL engine from the root module, which also upgrades dependencies used by production TiFlow code outside DM. Go's module selection is repository-wide rather than test-only, so production binaries will compile against automemlimit v0.5.0 instead of v0.2.4 and golang-lru v0.6.0 instead of v0.5.1. Those releases change cgroup v1/hybrid/no-limit detection and move LRU eviction callbacks outside the cache lock, making a monitoring-only fix alter unrelated runtime behavior.
Scope
dm/metrics/alertmanager/dm_worker_rules_test.go:21; go.mod:11; go.mod:56; go.mod:82; pkg/util/memory.go:34; cdc/sink/dmlsink/mq/dmlproducer/pulsar_dml_producer.go:99; cdc/sink/ddlsink/mq/ddlproducer/pulsar_ddl_producer.go:121; cdc/sink/dmlsink/txn/mysql/mysql.go:158
Risk if unchanged
DM and TiCDC binaries can regress in memory-limit selection on cgroup v1 or hybrid hosts and in eviction or close concurrency for Pulsar producers, prepared statements, and other sink caches. These behavior changes would ship as part of a two-line alert fix even though the new validation exercises only PromQL, expanding the compatibility surface and making failures difficult to attribute.
Evidence
The only new Prometheus import is the test import on this line, while the root go.mod changes 27 requirement lines, adds roughly 20 other module entries, and go.sum changes 384 lines. Production code calls memlimit.FromCgroup from pkg/util/memory.go, whose result controls the redo applier memory cap, and several cdc/sink packages use the upgraded LRU library. No cgroup-mode, redo memory-limit, Pulsar eviction, prepared-statement eviction, or concurrency test is changed in this PR.
Change request
Please keep the production dependency graph unchanged by running the rule test through isolated Prometheus tooling, such as the existing promtool rule-test format or a dedicated test module. If the runtime dependency upgrades are intentional, split them into a dedicated change and add focused coverage for cgroup v1, v2, hybrid, and no-limit detection as well as the affected LRU eviction and sink-cache concurrency paths.
There was a problem hiding this comment.
I’ll remove PromQL evaluation and keep lightweight YAML assertions. I won’t add promtool tests because downloading promtool adds over a minute to test execution.
|
|
||
| engine := promql.NewEngine(promql.EngineOpts{ | ||
| MaxSamples: 10000, | ||
| Timeout: time.Second, |
There was a problem hiding this comment.
🟡 [Minor] One-second wall-clock query deadline makes the rule test load-sensitive
Why
The tested alert expression has no timing semantics, but the engine is configured with a one-second real-time deadline. PromQL applies this setting through context.WithTimeout during query execution, so a constrained or race-enabled CI worker can fail the test because of host speed rather than an incorrect rule.
Scope
dm/metrics/alertmanager/dm_worker_rules_test.go:55
Risk if unchanged
The new regression test can fail intermittently under CPU contention, reducing confidence in the alert-rule signal and creating non-reproducible CI failures.
Evidence
EngineOpts.Timeout is set to time.Second; the upstream PromQL test setup in the selected v0.50.1 dependency uses 10-100 second safety timeouts for comparable engine-backed tests. The fixture itself is deterministic and does not require a one-second behavioral boundary.
Change request
Please use a generous safety timeout consistent with the upstream PromQL tests so this case fails on expression behavior, not machine scheduling speed.
There was a problem hiding this comment.
The PromQL evaluation and one-second timeout will be removed, so this issue no longer applies.
There was a problem hiding this comment.
this is not the right place to write tests, and the test seems not that necessary
Keep the alert rule test lightweight and avoid changing production dependencies for test-only PromQL evaluation.
|
[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
size/M
| } `yaml:"groups"` | ||
| } | ||
|
|
||
| func TestBinlogFileGapAlertRuleRequiresRunningTask(t *testing.T) { |
There was a problem hiding this comment.
I don't think this test test anything except that the yaml contains xxx
2 participants
What problem does this PR solve?
Issue Number: ref #12765
DM_binlog_file_gap_between_master_syncerevaluates paused and stopped tasks. These tasks can continue exposing master and syncer binlog metrics and trigger misleading critical alerts even though replication is intentionally not running.What is changed and how it works?
dm_worker_task_state == 2, so the alert only evaluates Running tasks.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. This change only affects alert evaluation and does not change DM runtime behavior. Paused and stopped tasks will no longer trigger this alert.
Do you need to update user documentation, design documentation or monitoring documentation?
No. The alert rule and its tests are updated directly.
Release note
Summary by CodeRabbit
Bug Fixes
Tests