metrics: hardcode lazy metric construction codepath #31962
Conversation
motivation is to remove the dependency on reflect.Call Throughout the codebase, GetOrRegister() is mostly used with lazy construction while Register() is strictly used with eagerly constructed metric objects. So it makes sense to isolate the lazy construction to GetOrRegister() and make caller decide on which function makes sense for them.
omerfirmak
force-pushed
the
no-reflect-metric
branch
from
a1d0466 to
0484562
Compare
metrics/registry.go
Outdated
| @@ -338,8 +327,11 @@ func Get(name string) interface{} { | |||
|
|
|||
| // GetOrRegister gets an existing metric or creates and registers a new one. Threadsafe | |||
| // alternative to calling Get and Register on failure. | |||
| func GetOrRegister(name string, i interface{}) interface{} { | |||
| return DefaultRegistry.GetOrRegister(name, i) | |||
| func GetOrRegister[T any](name string, ctor func() T, r Registry) interface{} { | |||
There was a problem hiding this comment.
I would prefer to have Registry as the first argument here
There was a problem hiding this comment.
That was my first choice too, but then I decided to follow the existing signature of metric specific counterparts like GetOrRegisterCounter et al. Which might makes sense because r is an "optional" argument which you can just pass a nil in. It looks slightly better for a nil to be at the end rather than at the beginning of args list imo. I don't mind changing it tho.
There was a problem hiding this comment.
GetOrRegister should do the type assertion internally and return T instead of interface{}.
There was a problem hiding this comment.
Also this does not need to be exported.
There was a problem hiding this comment.
GetOrRegister should do the type assertion internally and return T instead of interface{}.
Would've been nice but that is not possible, because the already registered metric can be of a different type. TestRegistryGetOrRegister relies on that behaviour. We could do that if you think we don't need to support that case.
Also this does not need to be exported.
It was already exported, do you want to make it private?
There was a problem hiding this comment.
We could do that if you think we don't need to support that case.
I feel like it doesn't make sense to support that, went ahead and done the change.
There was a problem hiding this comment.
Yes, when I wrote "this does not need to be exported", I meant, please make the new function GetOrRegister private, since it is an internal function to be called by the GetOrRegisterXXX functions.
There was a problem hiding this comment.
I know what you meant and I don't mind making it private. But this is not a new function, I am just modifying an existing one which was already public. It doesn't make sense to make it private because this is part of a family of functions that are all public. see https://github.com/ethereum/go-ethereum/pull/31962/files#diff-6bb88bbbfabec07942822873c99fb049bb38511750afd4179d6b99f0ec0fb955R338-R359
There was a problem hiding this comment.
restored the old public function to keep the interface intact and made this one private in 83c5e78
Co-authored-by: Marius van der Wijden <[email protected]>
motivation is to remove the dependency on reflect.Call
Throughout the codebase,
GetOrRegister()is mostly used with lazy construction whileRegister()is strictly used with eagerly constructed metric objects. So it makes sense to isolate the lazy construction toGetOrRegister()and make caller decide on which function makes sense for them.