From acb3d7d102a0ad397b6b88ffc00dd882483c47e4 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Sat, 26 Oct 2024 17:47:39 -0400 Subject: [PATCH] Add `Deregister` to `metrics.MultiGatherer` interface (#3498) --- api/metrics/label_gatherer.go | 14 ++-- api/metrics/label_gatherer_test.go | 100 +++++++++++++++++++++++++---- api/metrics/multi_gatherer.go | 32 +++++++-- api/metrics/prefix_gatherer.go | 12 ++-- utils/slice.go | 18 ++++++ utils/slice_test.go | 50 +++++++++++++++ 6 files changed, 197 insertions(+), 29 deletions(-) create mode 100644 utils/slice.go create mode 100644 utils/slice_test.go diff --git a/api/metrics/label_gatherer.go b/api/metrics/label_gatherer.go index 3b8951a75b77..0a4488dd1fd4 100644 --- a/api/metrics/label_gatherer.go +++ b/api/metrics/label_gatherer.go @@ -45,12 +45,14 @@ func (g *labelGatherer) Register(labelValue string, gatherer prometheus.Gatherer ) } - g.names = append(g.names, labelValue) - g.gatherers = append(g.gatherers, &labeledGatherer{ - labelName: g.labelName, - labelValue: labelValue, - gatherer: gatherer, - }) + g.register( + labelValue, + &labeledGatherer{ + labelName: g.labelName, + labelValue: labelValue, + gatherer: gatherer, + }, + ) return nil } diff --git a/api/metrics/label_gatherer_test.go b/api/metrics/label_gatherer_test.go index d5f30fd6529b..59873aa56173 100644 --- a/api/metrics/label_gatherer_test.go +++ b/api/metrics/label_gatherer_test.go @@ -138,9 +138,13 @@ func TestLabelGatherer_Gather(t *testing.T) { } } -func TestLabelGatherer_Register(t *testing.T) { +func TestLabelGatherer_Registration(t *testing.T) { + const ( + firstName = "first" + secondName = "second" + ) firstLabeledGatherer := &labeledGatherer{ - labelValue: "first", + labelValue: firstName, gatherer: &testGatherer{}, } firstLabelGatherer := func() *labelGatherer { @@ -154,25 +158,37 @@ func TestLabelGatherer_Register(t *testing.T) { } } secondLabeledGatherer := &labeledGatherer{ - labelValue: "second", + labelValue: secondName, gatherer: &testGatherer{ mfs: []*dto.MetricFamily{{}}, }, } - secondLabelGatherer := &labelGatherer{ + secondLabelGatherer := func() *labelGatherer { + return &labelGatherer{ + multiGatherer: multiGatherer{ + names: []string{ + firstLabeledGatherer.labelValue, + secondLabeledGatherer.labelValue, + }, + gatherers: prometheus.Gatherers{ + firstLabeledGatherer, + secondLabeledGatherer, + }, + }, + } + } + onlySecondLabeledGatherer := &labelGatherer{ multiGatherer: multiGatherer{ names: []string{ - firstLabeledGatherer.labelValue, secondLabeledGatherer.labelValue, }, gatherers: prometheus.Gatherers{ - firstLabeledGatherer, secondLabeledGatherer, }, }, } - tests := []struct { + registerTests := []struct { name string labelGatherer *labelGatherer labelValue string @@ -183,7 +199,7 @@ func TestLabelGatherer_Register(t *testing.T) { { name: "first registration", labelGatherer: &labelGatherer{}, - labelValue: "first", + labelValue: firstName, gatherer: firstLabeledGatherer.gatherer, expectedErr: nil, expectedLabelGatherer: firstLabelGatherer(), @@ -191,21 +207,21 @@ func TestLabelGatherer_Register(t *testing.T) { { name: "second registration", labelGatherer: firstLabelGatherer(), - labelValue: "second", + labelValue: secondName, gatherer: secondLabeledGatherer.gatherer, expectedErr: nil, - expectedLabelGatherer: secondLabelGatherer, + expectedLabelGatherer: secondLabelGatherer(), }, { name: "conflicts with previous registration", labelGatherer: firstLabelGatherer(), - labelValue: "first", + labelValue: firstName, gatherer: secondLabeledGatherer.gatherer, expectedErr: errDuplicateGatherer, expectedLabelGatherer: firstLabelGatherer(), }, } - for _, test := range tests { + for _, test := range registerTests { t.Run(test.name, func(t *testing.T) { require := require.New(t) @@ -214,4 +230,64 @@ func TestLabelGatherer_Register(t *testing.T) { require.Equal(test.expectedLabelGatherer, test.labelGatherer) }) } + + deregisterTests := []struct { + name string + labelGatherer *labelGatherer + labelValue string + expectedRemoved bool + expectedLabelGatherer *labelGatherer + }{ + { + name: "remove from nothing", + labelGatherer: &labelGatherer{}, + labelValue: firstName, + expectedRemoved: false, + expectedLabelGatherer: &labelGatherer{}, + }, + { + name: "remove unknown name", + labelGatherer: firstLabelGatherer(), + labelValue: secondName, + expectedRemoved: false, + expectedLabelGatherer: firstLabelGatherer(), + }, + { + name: "remove first name", + labelGatherer: firstLabelGatherer(), + labelValue: firstName, + expectedRemoved: true, + expectedLabelGatherer: &labelGatherer{ + multiGatherer: multiGatherer{ + // We must populate with empty slices rather than nil slices + // to pass the equality check. + names: []string{}, + gatherers: prometheus.Gatherers{}, + }, + }, + }, + { + name: "remove second name", + labelGatherer: secondLabelGatherer(), + labelValue: secondName, + expectedRemoved: true, + expectedLabelGatherer: firstLabelGatherer(), + }, + { + name: "remove only first name", + labelGatherer: secondLabelGatherer(), + labelValue: firstName, + expectedRemoved: true, + expectedLabelGatherer: onlySecondLabeledGatherer, + }, + } + for _, test := range deregisterTests { + t.Run(test.name, func(t *testing.T) { + require := require.New(t) + + removed := test.labelGatherer.Deregister(test.labelValue) + require.Equal(test.expectedRemoved, removed) + require.Equal(test.expectedLabelGatherer, test.labelGatherer) + }) + } } diff --git a/api/metrics/multi_gatherer.go b/api/metrics/multi_gatherer.go index b2fede55643c..d5e7464a87ab 100644 --- a/api/metrics/multi_gatherer.go +++ b/api/metrics/multi_gatherer.go @@ -5,10 +5,13 @@ package metrics import ( "fmt" + "slices" "sync" "github.com/prometheus/client_golang/prometheus" + "github.com/ava-labs/avalanchego/utils" + dto "github.com/prometheus/client_model/go" ) @@ -20,13 +23,11 @@ type MultiGatherer interface { // Register adds the outputs of [gatherer] to the results of future calls to // Gather with the provided [name] added to the metrics. Register(name string, gatherer prometheus.Gatherer) error -} -// Deprecated: Use NewPrefixGatherer instead. -// -// TODO: Remove once coreth is updated. -func NewMultiGatherer() MultiGatherer { - return NewPrefixGatherer() + // Deregister removes the outputs of a gatherer with [name] from the results + // of future calls to Gather. Returns true if a gatherer with [name] was + // found. + Deregister(name string) bool } type multiGatherer struct { @@ -42,6 +43,25 @@ func (g *multiGatherer) Gather() ([]*dto.MetricFamily, error) { return g.gatherers.Gather() } +func (g *multiGatherer) register(name string, gatherer prometheus.Gatherer) { + g.names = append(g.names, name) + g.gatherers = append(g.gatherers, gatherer) +} + +func (g *multiGatherer) Deregister(name string) bool { + g.lock.Lock() + defer g.lock.Unlock() + + index := slices.Index(g.names, name) + if index == -1 { + return false + } + + g.names = utils.DeleteIndex(g.names, index) + g.gatherers = utils.DeleteIndex(g.gatherers, index) + return true +} + func MakeAndRegister(gatherer MultiGatherer, name string) (*prometheus.Registry, error) { reg := prometheus.NewRegistry() if err := gatherer.Register(name, reg); err != nil { diff --git a/api/metrics/prefix_gatherer.go b/api/metrics/prefix_gatherer.go index fae7adb26e84..02e4858346eb 100644 --- a/api/metrics/prefix_gatherer.go +++ b/api/metrics/prefix_gatherer.go @@ -45,11 +45,13 @@ func (g *prefixGatherer) Register(prefix string, gatherer prometheus.Gatherer) e } } - g.names = append(g.names, prefix) - g.gatherers = append(g.gatherers, &prefixedGatherer{ - prefix: prefix, - gatherer: gatherer, - }) + g.register( + prefix, + &prefixedGatherer{ + prefix: prefix, + gatherer: gatherer, + }, + ) return nil } diff --git a/utils/slice.go b/utils/slice.go new file mode 100644 index 000000000000..abe7a57c725a --- /dev/null +++ b/utils/slice.go @@ -0,0 +1,18 @@ +// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package utils + +// DeleteIndex moves the last element in the slice to index [i] and shrinks the +// size of the slice by 1. +// +// This is an O(1) operation that allows the removal of an element from a slice +// when the order of the slice is not important. +// +// If [i] is out of bounds, this function will panic. +func DeleteIndex[S ~[]E, E any](s S, i int) S { + newSize := len(s) - 1 + s[i] = s[newSize] + s[newSize] = Zero[E]() + return s[:newSize] +} diff --git a/utils/slice_test.go b/utils/slice_test.go new file mode 100644 index 000000000000..cecc0c3ab5d1 --- /dev/null +++ b/utils/slice_test.go @@ -0,0 +1,50 @@ +// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package utils + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDeleteIndex(t *testing.T) { + tests := []struct { + name string + s []int + i int + expected []int + }{ + { + name: "delete only element", + s: []int{0}, + i: 0, + expected: []int{}, + }, + { + name: "delete first element", + s: []int{0, 1}, + i: 0, + expected: []int{1}, + }, + { + name: "delete middle element", + s: []int{0, 1, 2, 3}, + i: 1, + expected: []int{0, 3, 2}, + }, + { + name: "delete last element", + s: []int{0, 1, 2, 3}, + i: 3, + expected: []int{0, 1, 2}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := DeleteIndex(test.s, test.i) + require.Equal(t, test.expected, s) + }) + } +}