Skip to content

coordinator: clean stale owner checkpoint metrics - #5491

Merged
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
asddongmen:fix-owner-checkpoint-lag-metrics
Jun 24, 2026
Merged

coordinator: clean stale owner checkpoint metrics#5491
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
asddongmen:fix-owner-checkpoint-lag-metrics

Conversation

@asddongmen

@asddongmen asddongmen commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: close #5490

Finished or removed changefeeds can leave stale ticdc_owner_checkpoint_ts_lag series visible. Finished changefeeds remain in the coordinator changefeed DB as stopped metadata, but the owner metrics loop only skipped StateStopped and kept setting checkpoint lag for StateFinished. Old owner instances could also keep checkpoint labels after ownership stops.

What is changed and how it works?

This PR deletes owner checkpoint timestamp and lag labels together for stopped, finished, or removed changefeeds instead of recreating them during the metrics loop. It also resets owner checkpoint labels when the coordinator metrics loop exits, so an instance that loses ownership stops exposing stale checkpoint series. The stop/remove path now deletes both checkpoint_ts and checkpoint_ts_lag.

Check List

Tests

  • Unit test

Questions

Will it cause performance regression or break compatibility?

No. This only changes cleanup behavior for owner metrics labels.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

Fix stale owner checkpoint lag metrics for finished or removed changefeeds.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed metric cleanup for stopped changefeeds to prevent stale metrics from persisting in monitoring systems.
    • Improved periodic reset of changefeed metrics to ensure monitoring accuracy.
  • Tests

    • Added tests to verify metrics are properly cleaned up when changefeeds reach terminal states.

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed labels Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d4c021c8-e2fa-4e5d-8c58-de40499d455d

📥 Commits

Reviewing files that changed from the base of the PR and between 0eec971 and afa82cc.

📒 Files selected for processing (6)
  • coordinator/changefeed/changefeed_db.go
  • coordinator/changefeed/changefeed_db_test.go
  • coordinator/controller.go
  • coordinator/controller_test.go
  • pkg/metrics/changefeed.go
  • pkg/metrics/changefeed_test.go

📝 Walkthrough

Walkthrough

Adds two exported metric helpers (DeleteChangefeedCheckpointMetrics, ResetOwnerChangefeedMetrics) to the metrics package. Wires ResetOwnerChangefeedMetrics into Controller.collectMetrics via defer, refactors per-changefeed checkpoint gauge updates into a new updateChangefeedCheckpointMetrics helper that deletes metrics for terminal states, and updates StopByChangefeedID to use the new delete helper. Tests cover all three call sites.

Changes

Checkpoint metric cleanup for terminal changefeed states

Layer / File(s) Summary
New metric helper functions
pkg/metrics/changefeed.go, pkg/metrics/changefeed_test.go
Adds DeleteChangefeedCheckpointMetrics to delete checkpoint ts and lag gauge samples for a specific keyspace/changefeed, and ResetOwnerChangefeedMetrics to reset all five owner-scoped changefeed gauges. A new test file verifies ResetOwnerChangefeedMetrics zeroes all five gauges.
collectMetrics refactor and updateChangefeedCheckpointMetrics helper
coordinator/controller.go, coordinator/controller_test.go
collectMetrics defers ResetOwnerChangefeedMetrics at the start of each run. Per-changefeed checkpoint gauge logic is replaced by a new updateChangefeedCheckpointMetrics helper that deletes metrics for Stopped/Finished/Removed states and sets gauges for active states. A new test verifies both gauge cleanup and population.
StopByChangefeedID metric fix
coordinator/changefeed/changefeed_db.go, coordinator/changefeed/changefeed_db_test.go
Replaces single-gauge label deletion with DeleteChangefeedCheckpointMetrics. A new test verifies both checkpoint gauges are cleared after StopByChangefeedID.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • pingcap/ticdc#4499: Introduced ChangefeedErrorInfoGauge, which is now included in the ResetOwnerChangefeedMetrics reset call added in this PR.

Suggested reviewers

  • 3AceShowHand
  • lidezhu

Poem

🐇 Hop hop, the gauges were stuck in a rut,
Finished changefeeds still showing — oh, what!
With a Reset() and Delete() so neat,
Stale labels retreat from the metric fleet.
The rabbit says: clean dashboards can't be beat! 📊

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'coordinator: clean stale owner checkpoint metrics' directly and clearly summarizes the main change—removing stale checkpoint metrics.
Description check ✅ Passed The PR description provides a well-structured explanation following the template with linked issue, changes, test coverage, and release notes.
Linked Issues check ✅ Passed The PR fully addresses issue #5490 by deleting checkpoint metrics for terminal states, resetting owner metrics on loop exit, and adding comprehensive unit tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing stale owner checkpoint metrics through metric deletion and reset operations without introducing unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 24, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces helper functions to clean up and reset changefeed checkpoint metrics when a changefeed is stopped, finished, or removed, or when the coordinator's metric collection exits. It also adds corresponding unit tests to verify this behavior. The reviewer suggests expanding the reset logic to cover other owner-subsystem metrics (such as status, error, and downstream info gauges) to prevent stale metrics when a coordinator instance loses ownership.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/metrics/changefeed.go Outdated
@wk989898

Copy link
Copy Markdown
Collaborator

If the changefeed is deleted now, will the metrics from a certain period of time before be deleted? Please conduct a test to verify.

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 24, 2026
@asddongmen

Copy link
Copy Markdown
Collaborator Author

If the changefeed is deleted now, will the metrics from a certain period of time before be deleted? Please conduct a test to verify.

No, it will not.

image

@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongyunyan, wk989898

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [hongyunyan,wk989898]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-24 03:32:45.857970052 +0000 UTC m=+91249.521195565: ☑️ agreed by hongyunyan.
  • 2026-06-24 03:38:23.82900669 +0000 UTC m=+91587.492232193: ☑️ agreed by wk989898.

@ti-chi-bot
ti-chi-bot Bot merged commit 5d5121d into pingcap:master Jun 24, 2026
25 checks passed
@asddongmen

Copy link
Copy Markdown
Collaborator Author

/cherry-pick release-8.5

@ti-chi-bot

Copy link
Copy Markdown
Member

@asddongmen: new pull request created to branch release-8.5: #5766.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-8.5

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 ti-community-infra/tichi repository.

@asddongmen

Copy link
Copy Markdown
Collaborator Author

/cherry-pick release-8.5-20260728-v8.5.7

@ti-chi-bot

Copy link
Copy Markdown
Member

@asddongmen: new pull request created to branch release-8.5-20260728-v8.5.7: #5793.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-8.5-20260728-v8.5.7

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 ti-community-infra/tichi repository.

asddongmen added a commit that referenced this pull request Jul 28, 2026
* This is an automated cherry-pick of #5491

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

* coordinator: resolve v8.5.7 cherry-pick conflicts

* common: update TiDB TableInfo shared schema guard (#5652) (#5741)

close #5740

---------

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Co-authored-by: dongmen <20351731+asddongmen@users.noreply.github.com>
Co-authored-by: dongmen <414110582@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Owner checkpoint lag metric remains after changefeed finishes

4 participants