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

feat(katana): add executor metrics #1791

Merged
merged 7 commits into from
Apr 12, 2024
Merged

feat(katana): add executor metrics #1791

merged 7 commits into from
Apr 12, 2024

Conversation

kariy
Copy link
Member

@kariy kariy commented Apr 8, 2024

ref #1369

Add metrics on katana executor; tracking the total L1 gas and Cairo steps used.

There were two approaches that i thought of;

  1. record the metrics on every tx execution, or
  2. on every block

Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.

Thought im not exactly sure which one is the ideal one.

Doing (1) might be less performant bcs we have to acquire the lock to the metrics recorder more frequently (ie every tx), as opposed to only updating the metrics once every block.

another thing to note, currently doing (1) would require all executor implementations to define the metrics in their own implmentations, meaning have to duplicate code. If do (2) can just define it under block_producer scope and be executor agnostic.

EDIT: doing (2). metrics are collected upon completion of block production


some changes are made to gather the value after block production:

  • simplify params on backend::do_mine_block, now only accept two args; BlockEnv and ExecutionOutput
  • add a new type ExecutionStats under katana-executor, this is where executor would store the gas and steps value

@kariy kariy mentioned this pull request Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.61%. Comparing base (39ee46b) to head (da35e37).

Files Patch % Lines
crates/katana/core/src/service/block_producer.rs 87.50% 1 Missing ⚠️
crates/katana/primitives/src/receipt.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
+ Coverage   68.56%   68.61%   +0.05%     
==========================================
  Files         307      308       +1     
  Lines       33865    33900      +35     
==========================================
+ Hits        23219    23261      +42     
+ Misses      10646    10639       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glihm
Copy link
Collaborator

glihm commented Apr 9, 2024

Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.

The (1) approach is definitely the more reactive one. Since Katana should usually have low block-time and potentially a looooot of transactions, shouldn't be better performance wise to go with (2)?

For sure, a 20s block time would totally drop the refresh rate. :P But it still a very long block time for our use-cases I guess.

@glihm glihm added the katana This issue is related to Katana label Apr 9, 2024
@kariy
Copy link
Member Author

kariy commented Apr 10, 2024

Since Katana should usually have low block-time and potentially a looooot of transactions, shouldn't be better performance wise to go with (2)?

Good point!

I asked a Voyager guy and it seems like they evaluate the TPS and SPS below based on previous block. Which is how it'd be like if we go with (2)

image

@kariy
Copy link
Member Author

kariy commented Apr 10, 2024

Just realised that we wouldn't be able to see the updates in realtime anyway if the grafana refresh rate is set to more than the node's block time. By default the lowest refresh rate is 5s and i assume setting it too low could generally harm the node's performance.

Doing (2) on instant mining would pretty much appear similar to (1) so this decision mostly depends on whether or not we want to receive instant updates during the pending block period in interval mining.

EDIT: will change to (2)

@kariy kariy force-pushed the executor-metrics branch 2 times, most recently from b144326 to f1d95f0 Compare April 11, 2024 18:22
@kariy
Copy link
Member Author

kariy commented Apr 11, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kariy and the rest of your teammates on Graphite Graphite

@kariy
Copy link
Member Author

kariy commented Apr 12, 2024

Merge activity

  • Apr 12, 5:04 AM EDT: @kariy started a stack merge that includes this pull request via Graphite.
  • Apr 12, 5:04 AM EDT: @kariy merged this pull request with Graphite.

@kariy kariy merged commit 4ec3321 into main Apr 12, 2024
12 checks passed
@kariy kariy deleted the executor-metrics branch April 12, 2024 09:04
kariy added a commit that referenced this pull request Apr 12, 2024
ref #1791 #1369

<img width="1420" alt="Screenshot 2024-04-12 at 2 36 56 AM" src="https://github.com/dojoengine/dojo/assets/26515232/b97b49df-c5fc-429a-9cb0-bf66138a00b6">

showing total gas and steps using a simple line charts, tracking its growth over time
matzayonc pushed a commit that referenced this pull request Apr 12, 2024
ref #1369 

Add metrics on katana executor; tracking the total L1 **gas** and Cairo **steps** used.

There were two approaches that i thought of; 
1. record the metrics on every tx execution, or
2. on every block

~Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.~

Thought im not exactly sure which one is the ideal one.

Doing (1) might be less performant bcs we have to acquire the lock to the metrics recorder more frequently (ie every tx), as opposed to only updating the metrics once every block.

another thing to note, currently doing (1) would require all executor implementations to define the metrics in their own implmentations, meaning have to duplicate code. If do (2) can just define it under `block_producer` scope and be executor agnostic.

EDIT: doing (2). metrics are collected upon completion of block production

---

some changes are made to gather the value after block production:
- simplify params on `backend::do_mine_block`, now only accept two args; `BlockEnv` and `ExecutionOutput`
- add a new type `ExecutionStats` under `katana-executor`, this is where executor would store the gas and steps value
matzayonc pushed a commit that referenced this pull request Apr 12, 2024
ref #1791 #1369

<img width="1420" alt="Screenshot 2024-04-12 at 2 36 56 AM" src="https://github.com/dojoengine/dojo/assets/26515232/b97b49df-c5fc-429a-9cb0-bf66138a00b6">

showing total gas and steps using a simple line charts, tracking its growth over time
matzayonc pushed a commit that referenced this pull request Apr 14, 2024
ref #1369 

Add metrics on katana executor; tracking the total L1 **gas** and Cairo **steps** used.

There were two approaches that i thought of; 
1. record the metrics on every tx execution, or
2. on every block

~Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.~

Thought im not exactly sure which one is the ideal one.

Doing (1) might be less performant bcs we have to acquire the lock to the metrics recorder more frequently (ie every tx), as opposed to only updating the metrics once every block.

another thing to note, currently doing (1) would require all executor implementations to define the metrics in their own implmentations, meaning have to duplicate code. If do (2) can just define it under `block_producer` scope and be executor agnostic.

EDIT: doing (2). metrics are collected upon completion of block production

---

some changes are made to gather the value after block production:
- simplify params on `backend::do_mine_block`, now only accept two args; `BlockEnv` and `ExecutionOutput`
- add a new type `ExecutionStats` under `katana-executor`, this is where executor would store the gas and steps value
matzayonc pushed a commit that referenced this pull request Apr 14, 2024
ref #1791 #1369

<img width="1420" alt="Screenshot 2024-04-12 at 2 36 56 AM" src="https://github.com/dojoengine/dojo/assets/26515232/b97b49df-c5fc-429a-9cb0-bf66138a00b6">

showing total gas and steps using a simple line charts, tracking its growth over time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
katana This issue is related to Katana
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants