-
Notifications
You must be signed in to change notification settings - Fork 200
[Overload Resilience] Metrics for prioritized transactions #8030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Overload Resilience] Metrics for prioritized transactions #8030
Conversation
…n. Added implemention for metrics and integrated them in block building process
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Main piece of feedback is to consolidate the new metric with the existing ClusterBlockProposed
collectionSize: promauto.NewHistogramVec(prometheus.HistogramOpts{ | ||
Namespace: namespaceCollection, | ||
Subsystem: subsystemProposal, | ||
Buckets: []float64{1, 2, 5, 10, 20}, | ||
Name: "collection_size", | ||
Help: "number of transactions included in the block", | ||
}, []string{LabelChain}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have a metric defined that is equivalent to this, but it isn't being used anywhere (proposals
). Can we use that instead of adding a new metric?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that I report the facft that proposal has been created, the other metric, the old one, states about block being "proposed". That is the reason I have introduced a new metric, since proposing technically happens when we broadcast it over the network I didn't want to have confusing naming. And actually reporting those values at the point where we broadcast it requires us to carry additional information across modules(number of priority transactions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that, but the existing one is not being used. My main point is, let's just keep one of these nearly identical metrics (whether the existing one, or your new one) and update the documentation to match the context in which you are using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the old one and replaced it with new one, I think the naming is better and represents the purpose better.
7ad5f51
return nil, fmt.Errorf("could not build cluster block: %w", err) | ||
} | ||
|
||
b.metrics.ClusterBlockCreated(block, priorityTransactionsCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried making a panel that visualizes this yet? Most of our metrics are uniform across all nodes (each node reports approximately the same metric values, and we can average across them). For this one we will need to sum across all nodes (and this will exclude partner-run and Dapper-run nodes) to get a full picture, since each node is only reporting on blocks it is building.
It will definitely give us some useful information, we just need to be careful when interpreting it.
… with ClusterBlockCreated
Co-authored-by: Jordan Schalm <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear and clean. Nice work.
#7931
Context
This PR adds reporting of metrics related to the block construction process. Specifically it reports actual collection size and number of prioritized transactions for each created cluster block. The metric is reported during block building process, by each leader.
This will be used to add a panel which shows ratio of prioritized transactions to all transactions.