github/goldpinger
1
0
Fork 0
mirror of https://github.com/bloomberg/goldpinger.git synced 2026-08-23 21:36:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

Also prune response-time histogram "check" call_type on peer removal

Browse Source 
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) <noreply@anthropic.com>
Signed-off-by: Cooper Ry Lees <me@cooperlees.com>
This commit is contained in:
Cooper Ry Lees
2026-04-22 13:54:01 +00:00
co-authored by Claude Opus 4.7
parent e08e21f15c
commit 4308eb1fa5
2 changed files with 24 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/goldpinger/stats.go
+4 -2
View File
@@ -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.
pkg/goldpinger/stats_test.go
+20 -8
View File
@@ -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],
)