now fetching operatorId dynamically in economicCollector #54
Conversation
samlaf
force-pushed
the
samlaf/fix-economic-collector
branch
from
b2bde68 to
7d096fb
Compare
| for quorumIdx, quorumNum := range quorumNums { | ||
| // TODO: this is stupid.. when AVSs scale to have 5K operators we'll be running through a bunch of operators | ||
| // we should instead just call registryCoordinator.getQuorumBitmapIndicesByOperatorIdsAtBlockNumber | ||
| // and stakeRegistry.getStakeForOperatorIdForQuorumAtBlockNumber directly |
There was a problem hiding this comment.
Why don't we do these two per-operator fetches?
There was a problem hiding this comment.
Because I was lazy at the time. Fixed: 858ae16
samlaf
force-pushed
the
samlaf/fix-economic-collector
branch
from
7d096fb to
973343f
Compare
changed to cache: 5b942b6 |
| @@ -110,6 +108,17 @@ func (ec *Collector) Describe(ch chan<- *prometheus.Desc) { | |||
| // ch <- ec.delegatedShares | |||
| } | |||
|
|
|||
| func (ec *Collector) cacheOperatorIdIfNotCached() error { | |||
There was a problem hiding this comment.
cacheOperatorIdIfNotCached - it's a weird name. don't cache already mean if some key is not present it would cache it? just cacheOperatorId should be a good name for this?
There was a problem hiding this comment.
Not in this case because we only cache the very first time this is called. If it's already cached we don't update the cache.
There was a problem hiding this comment.
maybe maybeCacheOperator? or cacheOperatorIfNeeded? feel free to find another name, but I don't think cacheOperatorId works
There was a problem hiding this comment.
Renamed to initOperatorId as we discussed on slack: fa42170
There was a problem hiding this comment.
IMO just something like getOperatorID() will be good. The cache is internal detail/optimization that the caller doesn't need to care about.
| @@ -110,6 +108,17 @@ func (ec *Collector) Describe(ch chan<- *prometheus.Desc) { | |||
| // ch <- ec.delegatedShares | |||
| } | |||
|
|
|||
| func (ec *Collector) cacheOperatorIdIfNotCached() error { | |||
There was a problem hiding this comment.
IMO just something like getOperatorID() will be good. The cache is internal detail/optimization that the caller doesn't need to care about.
Motivation
eigenDA team was running into errors because the economicCollector was fetching operatorId from the eigenda registry coordinator in its constructor, which is initialized before the node starts. When the operator is registered at node start, it only registers in the start() function, which happens after, so the economicCollector never gets updated with the operatorId and hence keeps erroring on the collect call.
Solution
This PR moves fetching the operatorId into the collector. This adds 1 round trip latency to the node on every collect (every ~15sec), but at least this will fix this problem. Another solution might be to cache the operatorId after one of the collect calls gets the operatorId, but this could break if we later allow the operatorId to change.
Open questions