Skip to content

Batcher Metrics#5167

Merged
mergify[bot] merged 11 commits intodevelopfrom
seb/batcher-metrics
Mar 20, 2023
Merged

Batcher Metrics#5167
mergify[bot] merged 11 commits intodevelopfrom
seb/batcher-metrics

Conversation

@sebastianst
Copy link
Member

@sebastianst sebastianst commented Mar 15, 2023

Description

This PR adds metrics to the batcher. The following metrics are recorded:

  • channel
    • opened
    • closed
    • fully submitted
    • timed out
  • L2 blocks loaded into queue
  • L2 blocks added to channel
  • batch transaction
    • submitted
    • success
    • failed

Some metrics are implemented as simple events (like tx success), other contain many data dimensions, like RecordL2BlocksAdded.

Also added info logging of different stats when channels are closed. This has also long been overdue.

Open Tasks

Some open tasks were not done yet as to limit this PR's size.

  • CLI-3642 - Some reusable components were taken from the op-node metrics, namely RefMetrics and EventMetrics, and copied into package op-service/metrics. The duplicated code should be removed in a follow-up by making the op-node use these.
  • CLI-3641 - Record channel IDs and transaction IDs similar to how block hashes are recorded, to allow for a visual analysis of changing channels/transactions.
  • CLI-3640 - Record reasons for channel closing. A dependency cycle has to be resolved for that.

Metadata

@changeset-bot
Copy link

changeset-bot bot commented Mar 15, 2023

⚠️ No Changeset found

Latest commit: 62003a9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Mar 15, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 62003a9
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/641832f05c6b8e00080dc899

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is exciting

@trianglesphere
Copy link
Contributor

@sebastianst needs a go lint on l2block_utils.go

@trianglesphere
Copy link
Contributor

Also, all TODO's need linear tickets to pass semgrep.

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2023

Hey @sebastianst! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 16, 2023
@sebastianst sebastianst force-pushed the seb/batcher-metrics branch from 66bab91 to 8529542 Compare March 16, 2023 22:32
@mergify mergify bot removed the conflict label Mar 16, 2023
@sebastianst sebastianst force-pushed the seb/batcher-metrics branch from 8529542 to 2dbcdc2 Compare March 16, 2023 22:56
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #5167 (62003a9) into develop (259868c) will decrease coverage by 4.34%.
The diff coverage is 21.13%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5167      +/-   ##
===========================================
- Coverage    41.91%   37.58%   -4.34%     
===========================================
  Files          354      216     -138     
  Lines        21977    18192    -3785     
  Branches       776        0     -776     
===========================================
- Hits          9212     6837    -2375     
+ Misses       12062    10687    -1375     
+ Partials       703      668      -35     
Flag Coverage Δ
bedrock-go-tests 37.58% <21.13%> (-0.40%) ⬇️
contracts-bedrock-tests ?
contracts-tests ?
core-utils-tests ?
dtl-tests ?
fault-detector-tests ?
sdk-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-batcher/batcher/batch_submitter.go 0.00% <0.00%> (ø)
op-batcher/batcher/config.go 0.00% <ø> (ø)
op-batcher/batcher/driver.go 0.00% <0.00%> (ø)
op-batcher/metrics/metrics.go 0.00% <0.00%> (ø)
op-node/rollup/derive/l2block_util.go 0.00% <0.00%> (ø)
op-service/metrics/event.go 0.00% <0.00%> (ø)
op-service/metrics/ref_metrics.go 0.00% <0.00%> (ø)
op-batcher/metrics/noop.go 38.46% <38.46%> (ø)
op-node/rollup/derive/channel_out.go 77.84% <66.66%> (ø)
op-batcher/batcher/channel_manager.go 79.82% <83.54%> (+2.66%) ⬆️
... and 1 more

... and 144 files with indirect coverage changes

@tynes
Copy link
Contributor

tynes commented Mar 16, 2023

This is looking good

@sebastianst sebastianst force-pushed the seb/batcher-metrics branch 2 times, most recently from 9e30a15 to 17f13aa Compare March 17, 2023 17:15
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor nits

This is similar to PayloadToBlockRef but takes an interface instead that
e.g. a types.Block fulfills. This way, we can directly extract L2BlockRefs
from type.Blocks that have a valid L1 Info Deposit tx as first tx.

A block could also be converted into an ExecutionPayload, but this would make an
inefficient roundtrip, marshalling and unmarshalling all transactions.
The caller of BlockToBatch is sometimes interested in the L1BlockInfo
of the block, so instead of re-extracting it again, we just return it
here.
RefMetrics are a reusable metrics module to enable block reference metrics.
By removing some code duplication and a more efficient implementation
of the MaxRLPBytes test.
@sebastianst sebastianst force-pushed the seb/batcher-metrics branch from 61eb165 to 0803ea2 Compare March 20, 2023 09:50
@sebastianst sebastianst force-pushed the seb/batcher-metrics branch from 0803ea2 to 62003a9 Compare March 20, 2023 10:18
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🔥

@sebastianst
Copy link
Member Author

Output at http://localhost:7301/ after letting the batcher running for some time: batcher-metrics-prometheus.raw.txt

@mergify mergify bot merged commit 1258b25 into develop Mar 20, 2023
@mergify mergify bot deleted the seb/batcher-metrics branch March 20, 2023 14:15
@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Mar 20, 2023

This PR is next in line to be merged, and will be merged as soon as checks pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants