Refactor watch event dispatch metric stage breakdown

Project: kubernetes/kubernetes | Language: Go | Concepts: refactoring, observability, api-design | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141176

The problem

The code that tracks and emits metrics for the various stages of watch event dispatch in the Kubernetes apiserver was hard to extend. Each stage was defined as a field or enum, so adding new metrics or stages required changes in multiple places, making the code brittle and error-prone.

Why it's non-trivial

A naive fix might just add more fields or cases for each new metric stage, but this approach doesn't scale. It leads to repeated code changes across structs, method signatures, and metric emission logic. This increases the risk of bugs, makes it harder to maintain, and discourages adding new observability points.

The approach

The PR replaces the hardcoded enum and struct fields with a more extensible, table-driven approach:

1. Replace enum and fields with timeline array

Old approach (hardcoded fields and enum):

-type DispatchStage int
-
-const (
-	StageTotal DispatchStage = iota
-	StageStorageToCache
-	StageWatcherToClientHandler
-	numDispatchStages
-)
-
-var dispatchStageName = [numDispatchStages]string{
-	StageTotal:                  "total",
-	StageStorageToCache:         "storage_to_cache",
-	StageWatcherToClientHandler: "watcher_to_client_handler",
-}

New approach (timeline array and points):

+type DispatchPoint int
+
+const (
+	PointStorageDecoded DispatchPoint = iota
+	PointCacheReceived
+	PointEventBuilt
+	PointSentToClient
+	numDispatchPoints
+)
+
+type DispatchTimeline [numDispatchPoints]time.Time
+
+func (tl *DispatchTimeline) MarkAt(p DispatchPoint, t time.Time) { tl[p] = t }

What this does:
Defines a fixed array of timestamps (DispatchTimeline) indexed by logical points in the event lifecycle. This replaces multiple fields and allows for easy extension.

2. Table-driven stage definitions

Instead of hardcoding the metrics stages, the PR introduces a single table that maps label names to timeline intervals:

+var dispatchStages = []struct {
+	label    string
+	from, to DispatchPoint
+}{
+	{"storage_to_cache", PointStorageDecoded, PointCacheReceived},
+	{"watcher_to_client_handler", PointEventBuilt, PointSentToClient},
+	{"total", PointStorageDecoded, PointSentToClient},
+}

What this does:
Now, to add a new stage, you only add one line to this table. No need to touch struct fields or enums.

3. Metrics emission is now generic and loop-based

Old approach (per-stage logic):

-func (d *WatcherMetricsObservers) ObserveStage(stage DispatchStage, duration time.Duration) {
-	if stage < 0 || stage >= numDispatchStages {
-		return
-	}
-	observe(d.stageDurations[stage], duration)
-}

New approach (loop over stages and emit if both endpoints are set):

+func (d *WatcherMetricsObservers) ObserveTimeline(tl *DispatchTimeline) {
+	for i, stage := range dispatchStages {
+		from, to := tl[stage.from], tl[stage.to]
+		if from.IsZero() || to.IsZero() {
+			continue
+		}
+		observe(d.stageDurations[i], to.Sub(from))
+	}
+}

What this does:
This generic loop means the emission logic doesn't need to change when new stages are added.

4. Update event construction and marking points

Instead of setting individual fields, the code now marks points in the timeline:

-		RecordTime:      recordTime,
-		CacheReceived:   cacheReceived,
+		RecordTime:      recordTime,
+	}
+	wcEvent.timeline.MarkAt(metrics.PointStorageDecoded, recordTime)
+	wcEvent.timeline.MarkAt(metrics.PointCacheReceived, cacheReceived)

And in the watcher:

-	if !event.CacheReceived.IsZero() {
-		c.watcherMetrics.ObserveStage(metrics.StageStorageToCache, event.CacheReceived.Sub(event.RecordTime))
-	}
-	c.watcherMetrics.ObserveStage(metrics.StageWatcherToClientHandler, sentAt.Sub(builtAt))
-	c.watcherMetrics.ObserveStage(metrics.StageTotal, sentAt.Sub(event.RecordTime))
+	// The pre-fanout points are marked in processEvent; complete the per-watcher
+	// tail here then emit all stages.
+	tl := event.timeline
+	tl.MarkAt(metrics.PointEventBuilt, builtAt)
+	tl.MarkAt(metrics.PointSentToClient, sentAt)
+	c.watcherMetrics.ObserveTimeline(&tl)

What this does:
All metric points are handled via the timeline, making the code less repetitive and more robust.

5. Test updates

Tests now use the new timeline structure instead of individual time fields:

-	if !w.add(&watchCacheEvent{Object: makePod(5), ResourceVersion: 5, RecordTime: fakeClock.Now().Add(-2 * time.Second), CacheReceived: fakeClock.Now().Add(-1 * time.Second)}, time.NewTimer(1*time.Second)) {
+	if !w.add(&watchCacheEvent{
+		Object:          makePod(5),
+		ResourceVersion: 5,
+		RecordTime:      fakeClock.Now().Add(-2 * time.Second),
+		timeline: metrics.DispatchTimeline{
+			metrics.PointStorageDecoded: fakeClock.Now().Add(-2 * time.Second),
+			metrics.PointCacheReceived:  fakeClock.Now().Add(-1 * time.Second),
+		},
+	}, time.NewTimer(1*time.Second)) {

What this does:
Ensures the new approach is covered by tests and that test setup is future-proof.

From the review thread

[review] serathius: Not following this being a refactor. It adds stages.

[review] richabanker: It doesnt... If you look at dispatchStages - the stages are the same as what we have prior to this change

Takeaways

• Prefer table-driven or data-driven designs for extensibility. When adding new cases or metrics is a single-line change in a table, the code is easier to maintain and less error-prone.
• Avoid scattering related logic across multiple structs and enums. Centralizing lifecycle points and stage definitions reduces duplication and the risk of inconsistency.
• Generic, loop-based metric emission is more robust than per-case handling. This makes it easy to add, remove, or change metrics without touching the core emission logic.