Add initial metrics to Churn server #11
Conversation
Signed-off-by: Wellington Barbosa <[email protected]>
| Name: "requests", | ||
| Help: "the number of requests", | ||
| }, | ||
| []string{"status", "reason", "method"}, |
There was a problem hiding this comment.
Why do you need a "reason" to "success" request?
There was a problem hiding this comment.
Do you want 2 METRICS a part?
There was a problem hiding this comment.
One is sufficient, I am mostly looking at why NumFailedRequests is needed, since NumRequests already has a status label that can indicate if it's success or failure.
There was a problem hiding this comment.
I overlooked the metric "NumFailedRequests" in the code, and it shouldn't be there.
| // IncrementSuccessfulRequestNum increments the number of successful requests | ||
| func (g *Metrics) IncrementSuccessfulRequestNum(method string) { | ||
| g.NumRequests.With(prometheus.Labels{ | ||
| "status": "success", |
There was a problem hiding this comment.
Same as "reason", if this is always "success", why it needs to be a label?
And how do you tell the total requests that the churner received easily if there are two metrics (one for success and one for failed)?
There was a problem hiding this comment.
What do you mean?
We can filter, isn't it?
eigenda_churner_ requests({status ="success"})
eigenda_churner_ requests({status ="failed", reason="XPTO"})
There was a problem hiding this comment.
Then there is no need to have NumFailedRequests
churner/server.go
Outdated
| s.logger.Info("Received request: ", "QuorumIds", req.GetQuorumIds()) | ||
|
|
||
| now := time.Now() | ||
| // check that we are after the previous approval's expiry | ||
| if now.Unix() < latestExpiry { | ||
| s.metrics.IncrementFailedRequestNum("Churn", "Expiry: previous approval hasn't expired") |
There was a problem hiding this comment.
The "reason" part should be made concise as an Enum, rather than a sentence.
| @@ -192,6 +195,7 @@ func (c *churner) getOperatorsToChurn(ctx context.Context, quorumIDs []uint8, op | |||
| // verify the lowest stake against the registering operator's stake | |||
| // make sure that: lowestStake * churnBIPsOfOperatorStake < operatorToRegisterStake * bipMultiplier | |||
| if new(big.Int).Mul(lowestStake, churnBIPsOfOperatorStake).Cmp(new(big.Int).Mul(operatorToRegisterStake, bipMultiplier)) >= 0 { | |||
| c.metrics.IncrementFailedRequestNum("getOperatorsToChurn", "Insufficient stake: operator doesn't have enough stake") | |||
| return nil, errors.New("registering operator has less than churnBIPsOfOperatorStake") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
There is another case which fails to pass the stake check below
Signed-off-by: Wellington Barbosa <[email protected]>
| // IncrementSuccessfulRequestNum increments the number of successful requests | ||
| func (g *Metrics) IncrementSuccessfulRequestNum(method string) { | ||
| g.NumRequests.With(prometheus.Labels{ | ||
| "status": "success", |
There was a problem hiding this comment.
Then there is no need to have NumFailedRequests
Signed-off-by: Wellington Barbosa <[email protected]>
churner/metrics.go
Outdated
| FailReasonRateLimitExceeded FailReason = "rate_limit_exceeded" // Rate limited: per operator rate limiting | ||
| FailReasonInsufficientStakeToRegister FailReason = "insufficient_stake_to_register" // Operator doesn't have enough stake to be registered | ||
| FailReasonInsufficientStakeToChurn FailReason = "insufficient_stake_to_churn" // Operator doesn't have enough stake to be churned | ||
| FailReasonQuorumIdOutOfRange FailReason = "quorum_id_out_of_range" // Quorum ID out of range: quorum is not in the range of [0, 255] |
There was a problem hiding this comment.
nit: 255 -> QuorumCount, which is a value gotten from the chain and used to the validation
Why are these changes needed?
Add couple of metrics to Churn server
Related issue number
Checks