syncer: add region syncer observability#10914
Conversation
ti-chi-bot
Bot
added
release-note-none
|
[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/XXL
📝 WalkthroughWalkthroughAdds Prometheus instrumentation for region syncer client readiness, full-sync outcomes, history buffer state, downstream lag, and stream events, updates syncer lifecycle code to emit those metrics, adds tests for the new gauges and counters, and exposes the metrics in seven new Grafana panels. ChangesRegion Syncer Prometheus Metrics Instrumentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client as RegionSyncer client
participant Server as RegionSyncServer
participant Buffer as historyBuffer
participant Metrics as metrics.go
Client->>Metrics: setRegionSyncerClientReadyMetrics(false/true)
Server->>Metrics: observeFullSyncMetrics(result, reason, duration)
Server->>Metrics: observeDownstreamLagMetricsLocked(sendIndex)
Buffer->>Metrics: observeHistoryBufferMetrics(length, capacity, maxCapacity)
Server->>Metrics: incStreamEventMetrics(event)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![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
🧹 Nitpick comments (1)
pkg/syncer/server_test.go (1)
265-353: ⚡ Quick winUse bounded contexts for potentially blocking test calls.
Line 279, Line 292, Line 334, and Line 349 use
context.Background()on paths that can block; if behavior regresses, these tests can hang instead of failing fast. Prefercontext.WithTimeoutper call site.Suggested patch
func TestFullSyncMetrics(t *testing.T) { re := require.New(t) + testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() @@ - re.NoError(syncFullRegionsForTest(context.Background(), successSyncer, successSyncStream, 10)) + re.NoError(syncFullRegionsForTest(testCtx, successSyncer, successSyncStream, 10)) @@ done := make(chan error, 1) go func() { - done <- syncFullRegionsForTest(context.Background(), failureSyncer, failureSyncStream, startIndex) + done <- syncFullRegionsForTest(testCtx, failureSyncer, failureSyncStream, startIndex) }() @@ func TestDownstreamLagAndStreamEventMetrics(t *testing.T) { re := require.New(t) + testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() @@ - re.NoError(syncer.sendDownstream(context.Background(), downstream, syncStream, false)) + re.NoError(syncer.sendDownstream(testCtx, downstream, syncStream, false)) @@ - re.Error(timeoutSyncer.sendDownstream(context.Background(), "pd-timeout-follower", timeoutSyncStream, false)) + re.Error(timeoutSyncer.sendDownstream(testCtx, "pd-timeout-follower", timeoutSyncStream, false))As per coding guidelines, "Use context-aware timeouts and backoff for retries".
🤖 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/syncer/server_test.go` around lines 265 - 353, The test functions TestFullSyncMetrics and TestDownstreamLagAndStreamEventMetrics use context.Background() on potentially blocking operations like syncFullRegionsForTest and sendDownstream calls, which can cause tests to hang indefinitely if there's a regression instead of failing fast. Replace each context.Background() call with context.WithTimeout to add appropriate timeouts at each call site, ensuring tests fail quickly rather than hang when blocking behavior regresses.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.
Inline comments:
In `@pkg/syncer/server.go`:
- Around line 86-88: The `downstream` and `downstreamLagGauge` fields lack
proper synchronization protection, causing data races between write operations
(lines 136-167) and read operations in send paths (lines 148-157). Add a
dedicated RWMutex field to the struct alongside these metric fields, then
protect all mutations and reads of `downstream` and `downstreamLagGauge` with
appropriate lock/RLock calls to ensure consistent access and prevent nil-gauge
panics during unbind/shutdown scenarios.
---
Nitpick comments:
In `@pkg/syncer/server_test.go`:
- Around line 265-353: The test functions TestFullSyncMetrics and
TestDownstreamLagAndStreamEventMetrics use context.Background() on potentially
blocking operations like syncFullRegionsForTest and sendDownstream calls, which
can cause tests to hang indefinitely if there's a regression instead of failing
fast. Replace each context.Background() call with context.WithTimeout to add
appropriate timeouts at each call site, ensuring tests fail quickly rather than
hang when blocking behavior regresses.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e427aad5-7dc7-42ca-85b7-980b8a0e5a0b
📒 Files selected for processing (7)
metrics/grafana/pd.jsonpkg/syncer/client.gopkg/syncer/history_buffer.gopkg/syncer/history_buffer_test.gopkg/syncer/metrics.gopkg/syncer/server.gopkg/syncer/server_test.go
| downstream string | ||
| downstreamLagGauge prometheus.Gauge | ||
| } |
There was a problem hiding this comment.
Synchronize downstream metric fields with a single lock domain.
Lines 136-167 mutate downstream/downstreamLagGauge without synchronization, while Lines 148-157 read them in send paths. This lock mismatch can race during unbind/shutdown and lead to nil-gauge panics or inconsistent lag reporting.
Suggested fix (use a dedicated mutex for metric fields)
type regionSyncStream struct {
@@
+ metricsMu syncutil.Mutex
downstream string
downstreamLagGauge prometheus.Gauge
}
@@
func (s *regionSyncStream) setDownstreamMetrics(name string, leaderNextIndex uint64) {
+ s.metricsMu.Lock()
s.downstream = name
s.downstreamLagGauge = regionSyncerDownstreamLagGauge.WithLabelValues(name)
- s.observeDownstreamLagMetrics(leaderNextIndex)
+ s.metricsMu.Unlock()
+ s.observeDownstreamLagMetrics(leaderNextIndex)
}
@@
func (s *regionSyncStream) observeDownstreamLagMetricsLocked(leaderNextIndex uint64) {
- if s.downstreamLagGauge == nil {
+ s.metricsMu.Lock()
+ gauge := s.downstreamLagGauge
+ s.metricsMu.Unlock()
+ if gauge == nil {
return
}
sendIndex := s.getSendIndexLocked()
if sendIndex >= leaderNextIndex {
- s.downstreamLagGauge.Set(0)
+ gauge.Set(0)
return
}
- s.downstreamLagGauge.Set(float64(leaderNextIndex - sendIndex))
+ gauge.Set(float64(leaderNextIndex - sendIndex))
}
@@
func (s *regionSyncStream) deleteDownstreamMetrics() {
- if s.downstream == "" {
- return
- }
- regionSyncerDownstreamLagGauge.DeleteLabelValues(s.downstream)
- s.downstream = ""
- s.downstreamLagGauge = nil
+ s.metricsMu.Lock()
+ downstream := s.downstream
+ s.downstream = ""
+ s.downstreamLagGauge = nil
+ s.metricsMu.Unlock()
+ if downstream != "" {
+ regionSyncerDownstreamLagGauge.DeleteLabelValues(downstream)
+ }
}As per coding guidelines, "Guard shared state with mutex/RWMutex; keep lock ordering consistent".
Also applies to: 136-167
🤖 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/syncer/server.go` around lines 86 - 88, The `downstream` and
`downstreamLagGauge` fields lack proper synchronization protection, causing data
races between write operations (lines 136-167) and read operations in send paths
(lines 148-157). Add a dedicated RWMutex field to the struct alongside these
metric fields, then protect all mutations and reads of `downstream` and
`downstreamLagGauge` with appropriate lock/RLock calls to ensure consistent
access and prevent nil-gauge panics during unbind/shutdown scenarios.
Source: Coding guidelines
| oldStream.deleteDownstreamMetrics() | ||
| incStreamEventMetrics(streamEventUnbind) | ||
| } | ||
| syncStream.setDownstreamMetrics(name, leaderNextIndex) |
There was a problem hiding this comment.
This introduces a lock-order inversion: binding holds s.mu and then takes sendMu, while sendDownstream takes sendMu before s.mu.RLock. This is why the deadlock-tagged race tests fail. Please avoid taking sendMu while holding s.mu, or move this observation after releasing s.mu.
Signed-off-by: okjiang <819421878@qq.com>
okJiang
force-pushed
the
codex/region-syncer-observability
branch
from
2d0351d to
6a17cc9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/syncer/metrics.go (1)
207-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: string-concatenation map key is fragile.
fullSyncMetricKeyjoinsresultandreasonwith"/". It's safe today since none of the constants contain/, but a struct key{result, reason}would be more type-safe and avoid any future delimiter collision.🤖 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/syncer/metrics.go` around lines 207 - 209, The current fullSyncMetricKey helper builds a composite key by concatenating result and reason with a slash, which is fragile and can collide if either value ever contains that delimiter. Update the metrics keying in fullSyncMetricKey and its callers to use a type-safe composite struct or equivalent map key instead of string concatenation, keeping the result and reason fields separate and adjusting any lookup/storage logic that currently depends on the joined string.
🤖 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/syncer/metrics.go`:
- Around line 207-209: The current fullSyncMetricKey helper builds a composite
key by concatenating result and reason with a slash, which is fragile and can
collide if either value ever contains that delimiter. Update the metrics keying
in fullSyncMetricKey and its callers to use a type-safe composite struct or
equivalent map key instead of string concatenation, keeping the result and
reason fields separate and adjusting any lookup/storage logic that currently
depends on the joined string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ddec7c6-51c4-4ba8-b288-b0d95ed3901c
📒 Files selected for processing (7)
metrics/grafana/pd.jsonpkg/syncer/client.gopkg/syncer/history_buffer.gopkg/syncer/history_buffer_test.gopkg/syncer/metrics.gopkg/syncer/server.gopkg/syncer/server_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/syncer/history_buffer.go
- pkg/syncer/client.go
- pkg/syncer/history_buffer_test.go
- metrics/grafana/pd.json
- pkg/syncer/server_test.go
- pkg/syncer/server.go
|
@okJiang: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: ref #10719
What is changed and how does it work?
Check List
Tests
Manual test:
node -e "JSON.parse(require('fs').readFileSync('metrics/grafana/pd.json','utf8')); console.log('pd.json ok')"Release note
Summary by CodeRabbit