-
Notifications
You must be signed in to change notification settings - Fork 310
syncer(dm): initialize binlog metrics from checkpoint #12767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
02a72ff
8e089ff
6cefba8
3998b80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // Copyright 2026 PingCAP, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package syncer | ||
|
|
||
| import ( | ||
| "math" | ||
| "testing" | ||
|
|
||
| "github.com/go-mysql-org/go-mysql/mysql" | ||
| "github.com/pingcap/tiflow/dm/pkg/binlog" | ||
| "github.com/pingcap/tiflow/dm/pkg/gtid" | ||
| "github.com/pingcap/tiflow/dm/syncer/metrics" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
| "github.com/prometheus/client_golang/prometheus/testutil" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestInitSyncerBinlogMetrics(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Minor] Checkpoint-to-metric lifecycle wiring is not coveredWhy Scope Risk if unchanged Evidence Change request
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense, I will fix it. |
||
| gtidSet, err := gtid.ParserGTID( | ||
| mysql.MySQLFlavor, | ||
| "3ccc475b-2343-11e7-be21-6c0b84d59f30:1-3", | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| checkpoint binlog.Location | ||
| expectedFile float64 | ||
| expectedPos float64 | ||
| }{ | ||
| { | ||
| name: "file position checkpoint", | ||
| checkpoint: binlog.NewLocation(mysql.Position{ | ||
| Name: "binary-log.346652", | ||
| Pos: 560567, | ||
| }, nil), | ||
| expectedFile: 346652, | ||
| expectedPos: 560567, | ||
| }, | ||
| { | ||
| name: "missing position", | ||
| checkpoint: binlog.MustZeroLocation(mysql.MySQLFlavor), | ||
| expectedFile: math.NaN(), | ||
| expectedPos: float64(binlog.MinPosition.Pos), | ||
| }, | ||
| { | ||
| name: "GTID-only checkpoint", | ||
| checkpoint: binlog.NewLocation( | ||
| mysql.Position{}, | ||
| gtidSet, | ||
| ), | ||
| expectedFile: math.NaN(), | ||
| expectedPos: 0, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| fileGauge := prometheus.NewGauge(prometheus.GaugeOpts{Name: "syncer_binlog_file"}) | ||
| posGauge := prometheus.NewGauge(prometheus.GaugeOpts{Name: "syncer_binlog_pos"}) | ||
| s := &Syncer{ | ||
| metricsProxies: &metrics.Proxies{ | ||
| Metrics: &metrics.Metrics{ | ||
| BinlogSyncerFileGauge: fileGauge, | ||
| BinlogSyncerPosGauge: posGauge, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| s.initSyncerBinlogMetrics(tc.checkpoint) | ||
|
|
||
| actualFile := testutil.ToFloat64(fileGauge) | ||
| if math.IsNaN(tc.expectedFile) { | ||
| require.True(t, math.IsNaN(actualFile)) | ||
| } else { | ||
| require.Equal(t, tc.expectedFile, actualFile) | ||
| } | ||
| require.Equal(t, tc.expectedPos, testutil.ToFloat64(posGauge)) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 [Minor] Initializer name contradicts the helper's refresh behavior
Why
The helper is not limited to initialization:
Rundeliberately calls it again after the effective global checkpoint may change. Naming this repeated gauge updateinitSyncerBinlogMetricsobscures that it is safe and intended to overwrite existing metric values.Scope
dm/syncer/syncer.go:554
Risk if unchanged
Future callers may treat the helper as a one-time setup operation, miss required refreshes after checkpoint changes, or add initialization-only work that is unsafe on the second call.
Evidence
The new comment at
dm/syncer/syncer.go:1857explicitly says to "Refresh" the metrics, but line 1860 invokesinitSyncerBinlogMetrics; the same method is first called fromInitat line 548, and the test nameTestInitSyncerBinlogMetricsreinforces the one-time interpretation.Change request
Prefer
setSyncerBinlogMetricsorupdateSyncerBinlogMetrics; the current name is confusing. Rename the helper and its test so both initialization and later refresh call sites describe the same repeated update semantics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, I will fix it.