feat: more precise mempool metrics #1158
Conversation
mempool/metrics.go
Outdated
| FailedPrecheck = "precheck" | ||
| FailedAdding = "adding" | ||
| FailedRecheck = "recheck" | ||
|
|
||
| EvictedTxIncomingFullMempool = "full-removed-incoming" | ||
| EvictedTxExistingFullMempool = "full-removed-existing" | ||
| EvictedTxExpiredBlocks = "expired-ttl-blocks" | ||
| EvictedTxExpiredTime = "expired-ttl-time" |
There was a problem hiding this comment.
by adding labels, we can more precisely track why txs are being rejected, failing, or being evicted
mempool/v1/mempool.go
Outdated
| @@ -469,7 +481,10 @@ func (txmp *TxMempool) addNewTransaction(wtx *WrappedTx, checkTxRes *abci.Respon | |||
| "post_check_err", err, | |||
| ) | |||
|
|
|||
| txmp.metrics.FailedTxs.Add(1) | |||
| txmp.metrics.FailedTxs.With(mempool.FailedAdding).Add(1) | |||
| if err != nil { | |||
There was a problem hiding this comment.
This should also trigger of abci code is not zero. In this case we should print the raw logs and code as the reason
|
ran into an issue while testing this manually, which were fixed by the last two commits. apologies for the mistakes. this is confirmed to be working now tho |
There was a problem hiding this comment.
So I was also thinking about this a little more and it might be also good to distinguish whether this came from another peer or from an RPC endpoint (i.e. directly from a client who does not have complete state). We can do this by checking the TxInfo in the CheckTx method if there is a sender ID or not (can't recall the exact names)
| @@ -72,6 +72,7 @@ func PrometheusMetrics(namespace string, labelsAndValues ...string) *Metrics { | |||
| for i := 0; i < len(labelsAndValues); i += 2 { | |||
| labels = append(labels, labelsAndValues[i]) | |||
| } | |||
| typedCounterLabels := append(append(make([]string, 0, len(labels)+1), labels...), TypeLabel) | |||
There was a problem hiding this comment.
why not just append the TypeLabel?
There was a problem hiding this comment.
we probably could since I don't think we're writing to it later, but I've ran into too many difficult to find slice bugs in my life.
Since we're not overwriting an existing var, my fingers literally won't let me use append using an existing slice.
nice! good idea. the senderID should be 0, correct? |
| checkTxRes.Code, | ||
| checkTxRes.Codespace, | ||
| checkTxRes.Log, | ||
| wtx.HasPeer(0), // this checks if the peer id is local |
There was a problem hiding this comment.
I'm not 100% sure this will work as intended, but if there is no peer id, then it looks like the value will be zero. I did not check tho. cc @cmwaters
There was a problem hiding this comment.
This looks logically sound to me. It's a new transaction so there can't be any other peers but the sender
There was a problem hiding this comment.
I wonder if we should also be tracing this for recheckTx
| checkTxRes.Code, | ||
| checkTxRes.Codespace, | ||
| checkTxRes.Log, | ||
| wtx.HasPeer(0), // this checks if the peer id is local |
There was a problem hiding this comment.
This looks logically sound to me. It's a new transaction so there can't be any other peers but the sender
| checkTxRes.Code, | ||
| checkTxRes.Codespace, | ||
| checkTxRes.Log, | ||
| wtx.HasPeer(0), // this checks if the peer id is local |
There was a problem hiding this comment.
I wonder if we should also be tracing this for recheckTx
This PR introduces more precise mempool metrics to provide a clearer view in why txs are getting evicted or failing
## Description same as #1158 but targeted at main
Description
This PR introduces more precise mempool metrics to provide a clearer view in why txs are getting evicted or failing