Skip to content

op-proposer: add L1 fee metrics#5268

Closed
shrimalmadhur wants to merge 1 commit intoethereum-optimism:developfrom
shrimalmadhur:madhur/emit-gas-fee
Closed

op-proposer: add L1 fee metrics#5268
shrimalmadhur wants to merge 1 commit intoethereum-optimism:developfrom
shrimalmadhur:madhur/emit-gas-fee

Conversation

@shrimalmadhur
Copy link
Contributor

Description

  • We want to add metrics to emit L1 gas fee for proposer and batcher

Tests

Please describe any tests you've added. If you've added no tests, or left important behavior untested, please explain why not.

Invariants

For changes to critical code paths, please list and describe the invariants or key security properties of your new or changed code.

Additional context

Add any other context about the problem you're solving.

Metadata

  • Fixes #[Link to Issue]

TODOs

@changeset-bot
Copy link

changeset-bot bot commented Mar 27, 2023

⚠️ No Changeset found

Latest commit: 46bf607

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 27, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 46bf607
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/6421d0047cd3ab0007794abb

m.RecordL2Ref(BlockProposed, l2ref)
}

func (m *Metrics) RecordL1GasFee(receipt *types.Receipt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastianst I also want to add this metrics to batcher so I am wondering if I should add that is in op-service metrics.go file (ref_metrics.go) and re-use it.

Or ref_metrics is only for block ref data and I need to explicitly add this metrics on both places?

Copy link
Member

Choose a reason for hiding this comment

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

Gas fee tracking shouldn't be part of the RefMetrics, that's purely for reference data.

We just did a refactor in the op-batcher and op-proposer to move more transaction handling into the op-service/txmgr. So this metric (together with a few other metrics) should probably be added to the txmgr so that it's available to both, the batcher and proposer. It is on our radar to add metrics there. We can sync on that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice. just saw the refactored code on txmgr. I am happy to help with this. Let me know if you have some guidance for me about what ways you are expecting. I can wait till both op-batcher and op-proposer go via the new txmgr codebase and then add metrics. Let me know :)

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great, I'll write up an issue for the txmgr metrics and let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sebastianst

Copy link
Member

Choose a reason for hiding this comment

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

@shrimalmadhur here's the issue #5291 hope it has enough context & info. feel free to ask questions in that issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @shrimalmadhur I've got one more set of large change to the internals of the txmgr here (#5286, still WIP), but is shouldn't change the constructor/initialization flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sebastianst ! I will start working on it. I will close this PR and start a different one just to make things cleaner.

@trianglesphere 👍

add noop metrics

fix the method
@shrimalmadhur
Copy link
Contributor Author

Closing this PR to start working on #5291 in separate PR.

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.

3 participants