Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Deregister to metrics.MultiGatherer interface #3498

Merged
merged 2 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions api/metrics/label_gatherer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
100 changes: 88 additions & 12 deletions api/metrics/label_gatherer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -183,29 +199,29 @@ func TestLabelGatherer_Register(t *testing.T) {
{
name: "first registration",
labelGatherer: &labelGatherer{},
labelValue: "first",
labelValue: firstName,
gatherer: firstLabeledGatherer.gatherer,
expectedErr: nil,
expectedLabelGatherer: firstLabelGatherer(),
},
{
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)

Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No action required) Maybe use a name other than require to avoid shadowing the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this shadowing pretty much everywhere... I feel like the shadowing here is almost beneficial, as there is no reason to ever use any of the functions in the package after this point.


removed := test.labelGatherer.Deregister(test.labelValue)
require.Equal(test.expectedRemoved, removed)
require.Equal(test.expectedLabelGatherer, test.labelGatherer)
StephenButtolph marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
32 changes: 26 additions & 6 deletions api/metrics/multi_gatherer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
12 changes: 7 additions & 5 deletions api/metrics/prefix_gatherer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
18 changes: 18 additions & 0 deletions utils/slice.go
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is O(1) and not O(n) like the regular slices.Delete but it is less code and less complexity to just use the aforementioned builtin slices.Delete over adding a new method.
Deregister isn't called yet in the code, and if called - it will be probably be called seldomly, so I'm not sure if it's worth adding it from a code maintenance perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely isn't performance critical... But I also feel like I've re-implemented this same code like 5 times... So I feel like it is a reasonable helper to add

// 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]
}
50 changes: 50 additions & 0 deletions utils/slice_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
Loading