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: add cache sub commands and allow for proper transaction data logging #1173

Merged
merged 32 commits into from
May 2, 2024

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Jan 18, 2024

fixes #1106
Adds the following cache subcommands:

  • ls - list cached transactions and simulations
  • info - get info about the location of the data (default XDG_DATA_HOME standard)
  • read - out put raw data of transaction

This PR also caches referenced contracts dramatically speeding up using invoke and --help

@janewang janewang added the cli Related to Soroban CLI label Jan 18, 2024
@willemneal
Copy link
Member Author

Currently this requires: stellar/stellar-rpc#90

@willemneal willemneal force-pushed the feat/data_logging branch 4 times, most recently from fb6b4f5 to b7a66d5 Compare March 11, 2024 21:38
@willemneal willemneal marked this pull request as ready for review March 11, 2024 21:40
@janewang
Copy link
Contributor

To be able to print, as well as save to a local file

@willemneal willemneal marked this pull request as draft March 12, 2024 20:20
@willemneal willemneal force-pushed the feat/data_logging branch 3 times, most recently from fbdf760 to 1478548 Compare April 2, 2024 19:52
@willemneal willemneal marked this pull request as ready for review April 2, 2024 19:52
@willemneal willemneal changed the title feat: add proper transaction data logging feat: add cache sub commands and allow for proper transaction data logging Apr 2, 2024
@janewang janewang requested review from leighmcculloch and Shaptic and removed request for leighmcculloch April 2, 2024 20:05
@willemneal willemneal force-pushed the feat/data_logging branch 2 times, most recently from 9462ba6 to 83d7b93 Compare April 8, 2024 15:26
@willemneal willemneal enabled auto-merge (squash) April 8, 2024 16:59
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Few comments inline.

If I understand correctly this PR serves a couple purposes:

  1. Caching contract specs so they don't need to be downloaded repeatedly. 💯 👍🏻

  2. Caching transactions and simulations to facilitate tracking fees and resource consumption in the CLI (soroban-cli: automatically save resource cost of transactions #1106). My understanding of soroban-cli: automatically save resource cost of transactions #1106 was a bulk way of collecting statistics-like data, data that folks could feel comfortable sharing, but caching full transactions doesn't feel exactly aligned with that, as it's storing individual transactions and making individual transactions retrievable from the cache, and folks may not want to share individual transactions with all the arguments, auths, signatures, etc, not to mention their size will be unnecessarily large. @namankumar I think we might need a user story drawn up for your issue to make sure that what we're building here fits the user workflow. What format and what data exactly were you hoping to get with soroban-cli: automatically save resource cost of transactions #1106? I think storing a tx resource log that stores tx id, resource details, fee details would probably suffice?

cmd/soroban-cli/src/commands/cache/clean.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/cache/read.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/cache/read.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/data.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/data.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/data.rs Show resolved Hide resolved
cmd/soroban-cli/Cargo.toml Outdated Show resolved Hide resolved
@namankumar
Copy link

While the intent of #1106 was, like Leigh said, to reduce developer effort in profiling their transactions and sharing the aggregate data, I wonder whether having a full cache of tx is actually a net benefit? It can allow a developer to review their historical tx as they fine tune the code for limits and fees.

To be clear, I still only need the aggregate data and not the full tx; but now that the cacheing code is written, perhaps we can give it shape to make a useful feature out of it.

@janewang thoughts? Just an idea

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Couple minor things I commented inline that can be fixed up when merging.

A bigger issue is that the commands don't tell a clear story.

The cache stores contract specs, but the cache ls and cache read commands don't display contract specs that have been cached.

The cache stores responses to requests to rpc, and they're visible in the cache ls and cache read commands, but they aren't used as a cache, they're used as a log, and the cache ls and cache read commands are accessing that log.

In terms of what should be in the "cache", if we're true to what a cache is and is used for, it should just be the contract specs, and the request/response log should be a separate thing.

I'm inclined to merge just the contract spec caching parts of this PR, simplify the commands to only the clean command. Then we aren't merging commands that we don't really have a goal for that would then be a breaking change later to remove or fixup. We can always circle back to reviving this logic out of the PR at a later date if needed.

@janewang Does that work for you?

cmd/soroban-cli/src/commands/cache/read.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/cache/read.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/cache/info.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/data.rs Outdated Show resolved Hide resolved
leighmcculloch

This comment was marked as duplicate.

@namankumar
Copy link

Couple minor things I commented inline that can be fixed up when merging.

A bigger issue is that the commands don't tell a clear story.

The cache stores contract specs, but the cache ls and cache read commands don't display contract specs that have been cached.

The cache stores responses to requests to rpc, and they're visible in the cache ls and cache read commands, but they aren't used as a cache, they're used as a log, and the cache ls and cache read commands are accessing that log.

In terms of what should be in the "cache", if we're true to what a cache is and is used for, it should just be the contract specs, and the request/response log should be a separate thing.

I'm inclined to merge just the contract spec caching parts of this PR, simplify the commands to only the clean command. Then we aren't merging commands that we don't really have a goal for that would then be a breaking change later to remove or fixup. We can always circle back to reviving this logic out of the PR at a later date if needed.

@janewang Does that work for you?

Thanks for flagging. I'm filling in for @janewang here.

Could we simply add a new command "log"?
"Cache" can operate on contract specs
"Log" can operate on the log of RPC calls

@leighmcculloch
Copy link
Member

The term log on its own is overloaded. But yes we can move the actions log / request log items into their own command relatively simply.

@namankumar
Copy link

Actually a better story is making both subcommands of "cache", which opens space for future development as well.

cache specs
cache simulation
cache tx-log (or something)

@leighmcculloch
Copy link
Member

Actually a better story is making both subcommands of "cache", which opens space for future development as well.

cache specs

cache simulation

cache tx-log (or something)

I like it. Id change it to:

cache clean // cleans everything
cache info // as it is now dir for everything
cache commandlog ls
cache commandlog read

Basically move the commands that only work for txs into their own. We don't need any fns for spec at the moment. And clean can work for all.

I'm torn on the commandlog vs say requestlog.

@namankumar
Copy link

Sounds great!
Yeah commandlog is off, I'd make it something that describes the item being worked on more precisely, like preflight, simulation, tx profile, tx

@leighmcculloch leighmcculloch disabled auto-merge May 2, 2024 05:18
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I made a few minor changes (couple bug fixes, removed some incomplete functionality around output types, and changed the output type to be the full cached data instead of a subset of it).

In the end after testing this in a variety of situations I'm unclear on how useful the caching of the contract specs are actually going to be until we speed up the rpc. While it means the WASM doesn't have to be repeatedly downloaded, we still check which wasm the contract uses, and the RPC is pretty slow at providing that response, so the difference in user experience is negligible. But that's something we can improve on. If the RPC can get faster, then the whole thing will be faster. [Edit: I realised I'm testing from a far distance from the RPC I was testing with, so it's going to be delayed because of that so probably not a good test.]

@namankumar The actionslog command I left as is rather than split it up as you suggested only because this commands future is dubious at best and if we want to make a thing of it we should come up with a good story for it. I marked the command as experimental with a help comment so that folks know this is not a stable feature and will change.

@leighmcculloch leighmcculloch enabled auto-merge (squash) May 2, 2024 06:50
@leighmcculloch leighmcculloch merged commit 9fb2016 into stellar:main May 2, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to Soroban CLI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

soroban-cli: automatically save resource cost of transactions
4 participants