-
Notifications
You must be signed in to change notification settings - Fork 581
feat: measuring node round trips #19291
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
Conversation
8db5612 to
e972bd8
Compare
mverzilli
left a comment
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.
Approving as I don't see anything that could break, and I ultimately trust your judgment on whether this adds value as-is.
IMO there's problems in how we're measuring things here, and the names we're using to collect those stats can hide those problems. But maybe we find some big issues using this, fix them, archive the whole topic so 🤷♂️
yarn-project/pxe/src/contract_function_simulator/proxied_node.ts
Outdated
Show resolved
Hide resolved
3ef744e to
ad87c0f
Compare
3c91e76 to
d2bf40d
Compare
a158e93 to
5a8ef00
Compare
Flakey Tests🤖 says: This CI run detected 4 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
mverzilli
left a comment
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.
Cool! Nothing blocking, the only one I would pay some love to is the comment about not needing to use http to communicate with node in this context, which surprised me
In this PR I implement round trip tracking and I use this info as a basis for a roundtrip regression test. This check was placed in `yarn-project/end-to-end/src/bench/client_flows/amm.test.ts`. This test was a good candidate for this because we already use it for benchmarking and it doesn't communicate with node over `http` (instead we have a direct access to the instance) - this is great as it makes the dest more deterministic and hence the roundtrip counts should not be brittle (when communicating over network I can imagine that latency or something could result in different counts e.g. when syncing PXE or something - not super sure if this is still the case though but it was the case in the past when we were polling node for new blocks) In that test I also implemented a debugging functionality that allows for debugging the regression by nicely printing out round trip trace. To track the round trips I updated `ProxiedNode` by having an `inFlightCount` counter tracking num current RPC calls. This allows us to group parallel rpc calls into the same round trip. Fixes https://linear.app/aztec-labs/issue/F-230/improve-benchmarks The linked issue seems to have a wider scope than just the round trip measurements so not sure if we want to fully close it with this PR.
5a8ef00 to
33dc463
Compare
|
The |
In this PR I rename `ProxiedNode` as `BenchmarkedNode` because `ProxiedNode` name was just bad (originally discussed [here](#19291 (comment))) and I rename `build_nullifier_read_request_hints.ts` to reflect the name of the class it contains.
In this PR I rename `ProxiedNode` as `BenchmarkedNode` because `ProxiedNode` name was just bad (originally discussed [here](#19291 (comment))) and I rename `build_nullifier_read_request_hints.ts` to reflect the name of the class it contains.

In this PR I implement round trip tracking and I use this info as a basis for a roundtrip regression test. This check was placed in
yarn-project/end-to-end/src/bench/client_flows/amm.test.ts. This test was a good candidate for this because we already use it for benchmarking and it doesn't communicate with node overhttp(instead we have a direct access to the instance) - this is great as it makes the dest more deterministic and hence the roundtrip counts should not be brittle (when communicating over network I can imagine that latency or something could result in different counts e.g. when syncing PXE or something - not super sure if this is still the case though but it was the case in the past when we were polling node for new blocks)In that test I also implemented a debugging functionality that allows for debugging the regression by nicely printing out round trip trace.
To track the round trips I updated
ProxiedNodeby having aninFlightCountcounter tracking num current RPC calls. This allows us to group parallel rpc calls into the same round trip.Fixes https://linear.app/aztec-labs/issue/F-230/improve-benchmarks
The linked issue seems to have a wider scope than just the round trip measurements so not sure if we want to fully close it with this PR.