From 4308eb1fa59754f1353d2bd5cf41637c402eaff1 Mon Sep 17 00:00:00 2001 From: Cooper Ry Lees Date: Wed, 22 Apr 2026 13:51:55 +0000 Subject: [PATCH] Also prune response-time histogram "check" call_type on peer removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #169 review: DeletePeerMetrics only removed the "ping" call_type label set, leaving stale "check" samples from CheckAllPods in /metrics after a peer rolls. goldpingerResponseTimePeersHistogram is observed at two sites — Pinger (pinger.go:56, "ping") and CheckAllPods via GetLabeledPeersCallsTimer (client.go:209, "check") — so both must be pruned. Missed on the first pass because I traced the Pinger observation (the hot path) and did not separately check for the "check" label value emitted from the /check_all handler. Tests now seed both call_types before teardown so either gap regresses. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Cooper Ry Lees --- pkg/goldpinger/stats.go | 6 ++++-- pkg/goldpinger/stats_test.go | 28 ++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/pkg/goldpinger/stats.go b/pkg/goldpinger/stats.go index 4085bc8..4c7fc65 100644 --- a/pkg/goldpinger/stats.go +++ b/pkg/goldpinger/stats.go @@ -301,10 +301,12 @@ func SetPeerHopCount(hostIP, podIP string, hopCount int32) { } // DeletePeerMetrics removes stale metric labels for a destroyed peer. -// This covers the HTTP ping response-time histogram which is always active. -// Must be called unconditionally when a peer is removed. +// The response-time histogram is observed with two call_type values: +// "ping" (continuous from Pinger) and "check" (from CheckAllPods), so both +// must be pruned. Must be called unconditionally when a peer is removed. func DeletePeerMetrics(hostIP, podIP string) { goldpingerResponseTimePeersHistogram.DeleteLabelValues(GoldpingerConfig.Hostname, "ping", hostIP, podIP) + goldpingerResponseTimePeersHistogram.DeleteLabelValues(GoldpingerConfig.Hostname, "check", hostIP, podIP) } // DeletePeerUDPMetrics removes stale UDP metric labels for a destroyed peer. diff --git a/pkg/goldpinger/stats_test.go b/pkg/goldpinger/stats_test.go index 2bf359f..7a6890e 100644 --- a/pkg/goldpinger/stats_test.go +++ b/pkg/goldpinger/stats_test.go @@ -27,13 +27,16 @@ func TestDeletePeerMetrics_CleansResponseTimeHistogram(t *testing.T) { GoldpingerConfig.Hostname = "test-instance" defer func() { GoldpingerConfig.Hostname = origHostname }() - // Simulate a ping observation (call_type="ping") + // Simulate observations for both call_type values the histogram uses goldpingerResponseTimePeersHistogram.WithLabelValues( GoldpingerConfig.Hostname, "ping", tt.hostIP, tt.podIP, ).Observe(0.005) + goldpingerResponseTimePeersHistogram.WithLabelValues( + GoldpingerConfig.Hostname, "check", tt.hostIP, tt.podIP, + ).Observe(0.010) - if countMetrics(goldpingerResponseTimePeersHistogram) == 0 { - t.Fatal("response time histogram has no label values before cleanup — test setup is broken") + if n := countMetrics(goldpingerResponseTimePeersHistogram); n != 2 { + t.Fatalf("response time histogram has %d label sets before cleanup, want 2 — test setup is broken", n) } DeletePeerMetrics(tt.hostIP, tt.podIP) @@ -81,25 +84,31 @@ func TestDeletePeerMetrics_LeavesOtherPeersIntact(t *testing.T) { GoldpingerConfig.Hostname = "test-instance" defer func() { GoldpingerConfig.Hostname = origHostname }() - // Peer A + // Peer A — observe both call types goldpingerResponseTimePeersHistogram.WithLabelValues( GoldpingerConfig.Hostname, "ping", tt.peerA[0], tt.peerA[1], ).Observe(0.005) + goldpingerResponseTimePeersHistogram.WithLabelValues( + GoldpingerConfig.Hostname, "check", tt.peerA[0], tt.peerA[1], + ).Observe(0.006) SetPeerLossPct(tt.peerA[0], tt.peerA[1], 0) - // Peer B + // Peer B — observe both call types goldpingerResponseTimePeersHistogram.WithLabelValues( GoldpingerConfig.Hostname, "ping", tt.peerB[0], tt.peerB[1], ).Observe(0.010) + goldpingerResponseTimePeersHistogram.WithLabelValues( + GoldpingerConfig.Hostname, "check", tt.peerB[0], tt.peerB[1], + ).Observe(0.011) SetPeerLossPct(tt.peerB[0], tt.peerB[1], 1.5) // Delete peer A only DeletePeerMetrics(tt.peerA[0], tt.peerA[1]) DeletePeerUDPMetrics(tt.peerA[0], tt.peerA[1]) - // Peer B should survive in both metrics - if countMetrics(goldpingerResponseTimePeersHistogram) == 0 { - t.Error("response time histogram lost all label sets — peer B should still exist") + // Peer B's ping and check histogram entries should both survive + if n := countMetrics(goldpingerResponseTimePeersHistogram); n != 2 { + t.Errorf("response time histogram has %d label set(s), want 2 for peer B (ping+check)", n) } if countMetrics(goldpingerPeersLossPct) == 0 { t.Error("loss pct gauge lost all label sets — peer B should still exist") @@ -109,6 +118,9 @@ func TestDeletePeerMetrics_LeavesOtherPeersIntact(t *testing.T) { goldpingerResponseTimePeersHistogram.DeleteLabelValues( GoldpingerConfig.Hostname, "ping", tt.peerB[0], tt.peerB[1], ) + goldpingerResponseTimePeersHistogram.DeleteLabelValues( + GoldpingerConfig.Hostname, "check", tt.peerB[0], tt.peerB[1], + ) goldpingerPeersLossPct.DeleteLabelValues( GoldpingerConfig.Hostname, tt.peerB[0], tt.peerB[1], )