-
Notifications
You must be signed in to change notification settings - Fork 475
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
Adds additional metrics to the batch-posting path #2369
Conversation
…stia, eigenda, anytrust, etc)
We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2 |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
arbnode/batch_poster.go
Outdated
@@ -66,6 +66,14 @@ var ( | |||
blobGasLimitGauge = metrics.NewRegisteredGauge("arb/batchposter/blobgas/limit", nil) | |||
suggestedTipCapGauge = metrics.NewRegisteredGauge("arb/batchposter/suggestedtipcap", nil) | |||
|
|||
batchPosterBacklogGauge = metrics.NewRegisteredGauge("arb/batchposter/backlog", nil) |
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.
We already have metrics for the number of messages and bytes in the backlog. Calling this the "estimated batch backlog" or similar might be useful though. The main difference in this metric is it's divided by the estimated batch size.
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.
Can update the metric name!
@@ -66,6 +66,14 @@ var ( | |||
blobGasLimitGauge = metrics.NewRegisteredGauge("arb/batchposter/blobgas/limit", nil) | |||
suggestedTipCapGauge = metrics.NewRegisteredGauge("arb/batchposter/suggestedtipcap", nil) | |||
|
|||
batchPosterBacklogGauge = metrics.NewRegisteredGauge("arb/batchposter/backlog", nil) | |||
|
|||
batchPosterDALastSuccessfulActionGauge = metrics.NewRegisteredGauge("arb/batchPoster/action/da_last_success", nil) |
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 adding a last successful batch posting metric would also make sense to complement this
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.
It's a little tricky to add last success, because sending it to the dataposter successfully != the batch posting successfully as its asynchronous.
Updated as per comments, let me know if any additional feedback @PlasmaPower ! |
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.
LGTM
This PR adds additional metrics which tracks the batch posting backlog (and allows operators to set alerts on this growing), as well as DA posting success/failure/last_success metrics.
Goal here is to add more observability to this path, beyond the gap between finalized/unsafe heads. It's non-trivial to add additional metrics for the batch-posting path which track successes to the parent-chain, due to the data-poster path being asynchronous (vs waiting for receipts like the staker).
The backlog metric enables us to get a sense for how big this backlog is, and use the same metric across heterogenous environments (low throughput vs high throughput)