Add Deregister to metrics.MultiGatherer interface
#3498
Conversation
StephenButtolph
added
the
monitoring
|
|
||
| package utils | ||
|
|
||
| // DeleteIndex moves the last element in the slice to index [i] and shrinks the |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| } | ||
| for _, test := range deregisterTests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| require := require.New(t) |
There was a problem hiding this comment.
(No action required) Maybe use a name other than require to avoid shadowing the package?
There was a problem hiding this comment.
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.
Why this should be merged
Metrics are registered throughout avalanchego, deregistering all of them individually would require extensive additional code flows during the shutdown process. However, there are only a handful of metrics namespaces. This PR allows easily deregistering an entire namespace of metrics.
How this works
Adds
Deregisterto themetrics.MultiGathererinterface.How this was tested
Need to be documented in RELEASES.md?
No.