Skip to content

Commit 3df65f8

Browse files
hoffmaenhoffmaen
committed
Address review comments
1 parent a33c109 commit 3df65f8

7 files changed

Lines changed: 70 additions & 64 deletions

File tree

‎src/code.cloudfoundry.org/gorouter/metrics/compositereporter.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ type MetricReporter interface {
4848
CaptureNATSDroppedMessages(messages int)
4949
UnmuzzleRouteRegistrationLatency()
5050
CaptureEndpointsPerPool(endpoints int, route string, lbAlgo string)
51-
DeleteEndpointsPerPool(route, lbAlgo string)
51+
UncaptureEndpointsPerPool(route, lbAlgo string)
5252
}
5353

5454
type ComponentTagged interface {
@@ -238,9 +238,9 @@ func (m MultiMetricReporter) CaptureEndpointsPerPool(endpoints int, route string
238238
}
239239
}
240240

241-
func (m MultiMetricReporter) DeleteEndpointsPerPool(route, lbAlgo string) {
241+
func (m MultiMetricReporter) UncaptureEndpointsPerPool(route, lbAlgo string) {
242242
for _, r := range m {
243-
r.DeleteEndpointsPerPool(route, lbAlgo)
243+
r.UncaptureEndpointsPerPool(route, lbAlgo)
244244
}
245245
}
246246

@@ -276,6 +276,6 @@ func (c *CompositeReporter) CaptureEndpointsPerPool(endpoints int, route string,
276276
c.MetricReporter.CaptureEndpointsPerPool(endpoints, route, lbAlgo)
277277
}
278278

279-
func (c *CompositeReporter) DeleteEndpointsPerPool(route, lbAlgo string) {
280-
c.MetricReporter.DeleteEndpointsPerPool(route, lbAlgo)
279+
func (c *CompositeReporter) UncaptureEndpointsPerPool(route, lbAlgo string) {
280+
c.MetricReporter.UncaptureEndpointsPerPool(route, lbAlgo)
281281
}

‎src/code.cloudfoundry.org/gorouter/metrics/fakes/fake_metricreporter.go‎

Lines changed: 22 additions & 22 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/code.cloudfoundry.org/gorouter/metrics/metricsreporter.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ func (m *Metrics) CaptureEndpointsPerPool(endpoints int, route string, lbAlgo st
206206
}
207207

208208
// Empty implementation here is to fulfil interface
209-
func (m *Metrics) DeleteEndpointsPerPool(route string, lbAlgo string) {
209+
func (m *Metrics) UncaptureEndpointsPerPool(route string, lbAlgo string) {
210210
}
211211

212212
func getResponseCounterName(statusCode int) string {

‎src/code.cloudfoundry.org/gorouter/metrics_prometheus/metrics.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ func (metrics *Metrics) CaptureEndpointsPerPool(count int, route string, loadBal
227227
metrics.EndpointsPerPool.Set(float64(count), []string{route, loadBalancingAlgo})
228228
}
229229

230-
func (metrics *Metrics) DeleteEndpointsPerPool(route string, lbAlgo string) {
230+
func (metrics *Metrics) UncaptureEndpointsPerPool(route string, lbAlgo string) {
231231
metrics.EndpointsPerPool.Delete([]string{route, lbAlgo})
232232
}
233233

‎src/code.cloudfoundry.org/gorouter/metrics_prometheus/metrics_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,14 +487,14 @@ var _ = Describe("Metrics", func() {
487487
m.CaptureEndpointsPerPool(5, "routeA", "round_robin")
488488
Expect(getMetrics(r.Port())).To(ContainSubstring("endpoints_per_pool{lb_algorithm=\"round_robin\",route=\"routeA\"} 5"))
489489

490-
m.DeleteEndpointsPerPool("routeA", "round_robin")
490+
m.UncaptureEndpointsPerPool("routeA", "round_robin")
491491
Expect(getMetrics(r.Port())).NotTo(ContainSubstring("endpoints_per_pool{lb_algorithm=\"round_robin\",route=\"routeA\"}"))
492492
})
493493

494494
It("does nothing when deleting a non-existent label combination", func() {
495495
m.CaptureEndpointsPerPool(5, "routeA", "round_robin")
496496

497-
m.DeleteEndpointsPerPool("routeX", "round_robin")
497+
m.UncaptureEndpointsPerPool("routeX", "round_robin")
498498
Expect(getMetrics(r.Port())).To(ContainSubstring("endpoints_per_pool{lb_algorithm=\"round_robin\",route=\"routeA\"} 5"))
499499
})
500500
})

‎src/code.cloudfoundry.org/gorouter/registry/registry.go‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ func (r *RouteRegistry) pruneStaleDroplets() {
451451

452452
if lbAlgo == config.LOAD_BALANCE_HB {
453453
if t.Pool == nil || t.Pool.NumEndpoints() == 0 {
454-
r.reporter.DeleteEndpointsPerPool(uri, config.LOAD_BALANCE_HB)
454+
r.reporter.UncaptureEndpointsPerPool(uri, config.LOAD_BALANCE_HB)
455455
} else {
456456
r.reporter.CaptureEndpointsPerPool(t.Pool.NumEndpoints(), uri, config.LOAD_BALANCE_HB)
457457
}
@@ -474,20 +474,26 @@ func (r *RouteRegistry) freshenRoutes() {
474474
})
475475
}
476476

477+
// reportEndpointsPerPool reports the endpoints_per_pool metric for hash-based (HB) route pools.
478+
// For non-HB endpoints, it unconditionally deletes stale metric entries.
477479
func (r *RouteRegistry) reportEndpointsPerPool(uri route.Uri, endpoint *route.Endpoint) {
478-
if endpoint.LoadBalancingAlgorithm == config.LOAD_BALANCE_HB {
479-
pool := r.byURI.Find(uri.RouteKey())
480-
if pool == nil || pool.NumEndpoints() == 0 {
481-
r.reporter.DeleteEndpointsPerPool(string(uri), config.LOAD_BALANCE_HB)
482-
return
483-
}
484-
r.reporter.CaptureEndpointsPerPool(pool.NumEndpoints(), string(uri), config.LOAD_BALANCE_HB)
485-
} else {
480+
if endpoint.LoadBalancingAlgorithm != config.LOAD_BALANCE_HB {
486481
// Delete stale entries for routes that have switched from HB to another load balancing algorithm.
487482
// This is called unconditionally because the pool may still report its algorithm as HB
488483
// even after the HB setting was removed (setPoolLoadBalancingAlgorithm ignores empty values).
489-
r.reporter.DeleteEndpointsPerPool(string(uri), config.LOAD_BALANCE_HB)
484+
r.reporter.UncaptureEndpointsPerPool(string(uri), config.LOAD_BALANCE_HB)
485+
return
486+
}
487+
488+
r.RLock()
489+
pool := r.byURI.Find(uri.RouteKey())
490+
r.RUnlock()
491+
492+
if pool == nil || pool.NumEndpoints() == 0 {
493+
r.reporter.UncaptureEndpointsPerPool(string(uri), config.LOAD_BALANCE_HB)
494+
return
490495
}
496+
r.reporter.CaptureEndpointsPerPool(pool.NumEndpoints(), string(uri), config.LOAD_BALANCE_HB)
491497
}
492498

493499
func splitHostAndContextPath(uri route.Uri) (string, string) {

‎src/code.cloudfoundry.org/gorouter/registry/registry_test.go‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -599,11 +599,11 @@ var _ = Describe("RouteRegistry", func() {
599599
r.Register("hb.example.com", hbEndpoint)
600600

601601
captureCount := reporter.CaptureEndpointsPerPoolCallCount()
602-
deleteCount := reporter.DeleteEndpointsPerPoolCallCount()
602+
deleteCount := reporter.UncaptureEndpointsPerPoolCallCount()
603603

604604
// The metric should be captured but NOT deleted for an active HB route
605605
Expect(captureCount).To(Equal(1))
606-
Expect(deleteCount).To(Equal(0), "DeleteEndpointsPerPool should not be called when registering an HB endpoint with a live pool")
606+
Expect(deleteCount).To(Equal(0), "UncaptureEndpointsPerPool should not be called when registering an HB endpoint with a live pool")
607607
})
608608

609609
It("does not report the metric for non-HB endpoints", func() {
@@ -631,9 +631,9 @@ var _ = Describe("RouteRegistry", func() {
631631

632632
r.Unregister("hb.example.com", hbEndpoint)
633633
// After unregistering the last endpoint, the metric should be deleted
634-
Expect(reporter.DeleteEndpointsPerPoolCallCount()).To(BeNumerically(">=", 1))
635-
lastCall := reporter.DeleteEndpointsPerPoolCallCount() - 1
636-
uri, algo := reporter.DeleteEndpointsPerPoolArgsForCall(lastCall)
634+
Expect(reporter.UncaptureEndpointsPerPoolCallCount()).To(BeNumerically(">=", 1))
635+
lastCall := reporter.UncaptureEndpointsPerPoolCallCount() - 1
636+
uri, algo := reporter.UncaptureEndpointsPerPoolArgsForCall(lastCall)
637637
Expect(uri).To(Equal("hb.example.com"))
638638
Expect(algo).To(Equal(config.LOAD_BALANCE_HB))
639639
})
@@ -682,20 +682,20 @@ var _ = Describe("RouteRegistry", func() {
682682
LoadBalancingAlgorithm: config.LOAD_BALANCE_RR,
683683
})
684684

685-
deleteBefore := reporter.DeleteEndpointsPerPoolCallCount()
685+
deleteBefore := reporter.UncaptureEndpointsPerPoolCallCount()
686686
r.Register("switching.example.com", rrEndpoint)
687687

688688
// Should have called delete to clean up the stale HB metric
689-
Expect(reporter.DeleteEndpointsPerPoolCallCount()).To(BeNumerically(">", deleteBefore))
689+
Expect(reporter.UncaptureEndpointsPerPoolCallCount()).To(BeNumerically(">", deleteBefore))
690690
found := false
691-
for i := deleteBefore; i < reporter.DeleteEndpointsPerPoolCallCount(); i++ {
692-
uri, algo := reporter.DeleteEndpointsPerPoolArgsForCall(i)
691+
for i := deleteBefore; i < reporter.UncaptureEndpointsPerPoolCallCount(); i++ {
692+
uri, algo := reporter.UncaptureEndpointsPerPoolArgsForCall(i)
693693
if uri == "switching.example.com" && algo == config.LOAD_BALANCE_HB {
694694
found = true
695695
break
696696
}
697697
}
698-
Expect(found).To(BeTrue(), "expected DeleteEndpointsPerPool to be called for the switched route")
698+
Expect(found).To(BeTrue(), "expected UncaptureEndpointsPerPool to be called for the switched route")
699699
})
700700

701701
It("deletes the HB metric when the HB algorithm is removed and endpoint registers with empty algorithm", func() {
@@ -717,19 +717,19 @@ var _ = Describe("RouteRegistry", func() {
717717
LoadBalancingAlgorithm: "",
718718
})
719719

720-
deleteBefore := reporter.DeleteEndpointsPerPoolCallCount()
720+
deleteBefore := reporter.UncaptureEndpointsPerPoolCallCount()
721721
r.Register("removed-hb.example.com", noAlgoEndpoint)
722722

723-
Expect(reporter.DeleteEndpointsPerPoolCallCount()).To(BeNumerically(">", deleteBefore))
723+
Expect(reporter.UncaptureEndpointsPerPoolCallCount()).To(BeNumerically(">", deleteBefore))
724724
found := false
725-
for i := deleteBefore; i < reporter.DeleteEndpointsPerPoolCallCount(); i++ {
726-
uri, algo := reporter.DeleteEndpointsPerPoolArgsForCall(i)
725+
for i := deleteBefore; i < reporter.UncaptureEndpointsPerPoolCallCount(); i++ {
726+
uri, algo := reporter.UncaptureEndpointsPerPoolArgsForCall(i)
727727
if uri == "removed-hb.example.com" && algo == config.LOAD_BALANCE_HB {
728728
found = true
729729
break
730730
}
731731
}
732-
Expect(found).To(BeTrue(), "expected DeleteEndpointsPerPool to be called when HB algorithm is removed")
732+
Expect(found).To(BeTrue(), "expected UncaptureEndpointsPerPool to be called when HB algorithm is removed")
733733
})
734734

735735
It("does not change the metric count when an existing HB endpoint is updated", func() {
@@ -763,10 +763,10 @@ var _ = Describe("RouteRegistry", func() {
763763

764764
It("deletes the metric when HB endpoints are pruned", func() {
765765
hbEndpoint := route.NewEndpoint(&route.EndpointOpts{
766-
Host: "192.168.1.1",
767-
Port: 8080,
768-
PrivateInstanceId: "id-hb-1",
769-
LoadBalancingAlgorithm: config.LOAD_BALANCE_HB,
766+
Host: "192.168.1.1",
767+
Port: 8080,
768+
PrivateInstanceId: "id-hb-1",
769+
LoadBalancingAlgorithm: config.LOAD_BALANCE_HB,
770770
StaleThresholdInSeconds: int(configObj.DropletStaleThreshold.Seconds()),
771771
})
772772

@@ -778,18 +778,18 @@ var _ = Describe("RouteRegistry", func() {
778778

779779
// Wait for the endpoint to go stale and be pruned
780780
Eventually(func() int {
781-
return reporter.DeleteEndpointsPerPoolCallCount()
781+
return reporter.UncaptureEndpointsPerPoolCallCount()
782782
}, configObj.PruneStaleDropletsInterval+configObj.DropletStaleThreshold+100*time.Millisecond).Should(BeNumerically(">=", 1))
783783

784784
found := false
785-
for i := 0; i < reporter.DeleteEndpointsPerPoolCallCount(); i++ {
786-
uri, algo := reporter.DeleteEndpointsPerPoolArgsForCall(i)
785+
for i := 0; i < reporter.UncaptureEndpointsPerPoolCallCount(); i++ {
786+
uri, algo := reporter.UncaptureEndpointsPerPoolArgsForCall(i)
787787
if uri == "hb-prune.example.com" && algo == config.LOAD_BALANCE_HB {
788788
found = true
789789
break
790790
}
791791
}
792-
Expect(found).To(BeTrue(), "expected DeleteEndpointsPerPool to be called when HB endpoints are pruned")
792+
Expect(found).To(BeTrue(), "expected UncaptureEndpointsPerPool to be called when HB endpoints are pruned")
793793
})
794794

795795
It("updates the metric when some HB endpoints are pruned but others remain", func() {

0 commit comments

Comments
 (0)