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

Implement three trace APIs #2205

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

maxconway
Copy link
Contributor

@maxconway maxconway commented Jan 21, 2025

  • trace_block
  • trace_filter
  • trace_transaction

Plus tests and docs

Closes #2117
Closes #2192
Closes #2197

- trace_block
- trace_filter
- trace_transaction

Plus tests and docs
@maxconway maxconway marked this pull request as ready for review January 21, 2025 14:36
Copy link
Contributor

github-actions bot commented Jan 21, 2025

🐰 Bencher Report

Branchmax/feature/trace_apis_batch_1
Testbedself-hosted
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
milliseconds (ms)
(Result Δ%)
Upper Boundary
milliseconds (ms)
(Limit %)
full-blocks-erc20-transfers/full-blocks-erc20-transfers📈 view plot
🚷 view threshold
1,188.10
(-14.48%)
1,832.32
(64.84%)
full-blocks-evm-transfers/full-blocks-evm-transfers📈 view plot
🚷 view threshold
553.68
(-9.56%)
695.38
(79.62%)
full-blocks-zil-transfers/full-blocks-zil-transfers📈 view plot
🚷 view threshold
3,552.00
(-15.88%)
5,385.11
(65.96%)
process-empty/process-empty📈 view plot
🚷 view threshold
8.39
(-7.48%)
10.43
(80.49%)
🐰 View full continuous benchmarking report in Bencher

Comment on lines 274 to 275
.skip(after as usize)
.take(count as usize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it wasteful to only use these parameters after you've already collected all_traces? Should we use them above? Or equivalently, make all_traces an Iterator which you collect the correct number of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good point. I did a procedural approach here and used a loop label here because that was the smaller change from what we already had, but it feels a bit gnarly to me so I'm very open to switching to the iterator approach if you'd prefer?

- Moved pagination earlier in the code to avoid unnecessary traces
- I've taken a procedural approach here with use of a loop label, but I'm open to switching to iterators
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.

Implement trace_transaction Implement trace_block Implement trace_filter RPC method
2 participants