Skip to content
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

Add some metrics for L1 processing #29

Merged
merged 10 commits into from
Oct 20, 2023
Merged

Add some metrics for L1 processing #29

merged 10 commits into from
Oct 20, 2023

Conversation

tuommaki
Copy link
Collaborator

@tuommaki tuommaki commented Oct 19, 2023

Prints total progress and number of processed blocks:
image

Prints total progress and number of processed blocks.
@tuommaki tuommaki requested a review from zeapoz October 19, 2023 12:15
@tuommaki tuommaki self-assigned this Oct 19, 2023
@tuommaki tuommaki marked this pull request as ready for review October 19, 2023 13:21
Copy link
Member

@zeapoz zeapoz left a comment

Choose a reason for hiding this comment

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

LGTM, some suggestions.

src/l1_fetcher.rs Outdated Show resolved Hide resolved
src/l1_fetcher.rs Outdated Show resolved Hide resolved
let perc = (cur * 100) / total;
format!("{perc}%")
}
None => "∞".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Currently this won't print any percentages if we don't supply an end_block, right? Could we use the latest (currently polled) L1 block number that exists in that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this won't print any percentages if we don't supply an end_block, right?

If disable_polling == false, the execution continues forever and then this won't print any percentages.

Could we use the latest (currently polled) L1 block number that exists in that case?

With extremely long stretch, yeah, maybe we could, but it would be very far from correct.

I struggled quite a bit to provide any useful metrics from L1 blocks because of following behaviour:

  • We don't literally process L1 blocks. We filter event logs with certain criteria and each filter batch spans over multiple L1 blocks.
  • We see events from L1 blocks here & there. Similar story with transactions.

Because of that, I don't really consider the L1 block number too useful, but that being said, the information is already there (CUR L1 & L2 block numbers). I don't think it's useful to have it again either.

One alternative could be to display just "PROCESSING", instead of "PROGRESS [ n %]" when polling is enabled?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to be able to see how many blocks (and for a future PR; roughly be able to estimate how long the reconstruction will take) are left to be processed and filtered. Given that most, if not all, users would be reconstructing to the latest L1 block, that information seems quite useful. So, even if it doesn't accurately reflect the state of L2, I still think we should include it, just as a general progress bar (it's nice too see say 26%, instead of block 1583).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I changed it so that last_l1_block is now always set. When polling goes past it, the percentages should cap at 100%.

src/l1_fetcher.rs Show resolved Hide resolved
Copy link
Member

@zeapoz zeapoz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

latest_l2_block_nbr: u64,

first_l1_block: Option<u64>,
last_l1_block: Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be an Option anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not. I'll clean it out 🙂

@tuommaki tuommaki merged commit 1918eb1 into main Oct 20, 2023
4 checks passed
@tuommaki tuommaki deleted the add-l1-metrics branch October 20, 2023 09:40
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.

2 participants