-
Notifications
You must be signed in to change notification settings - Fork 50
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 rpc logging #75
Conversation
pub(crate) async fn query_broadcast_tx( | ||
&self, | ||
method: &methods::broadcast_tx_commit::RpcBroadcastTxCommitRequest, | ||
) -> MethodCallResult< |
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.
I don't really like how I had to create a separate function for broadcast txs, but it seems like Rust does not support specialization, so I do not know if there is a better way... Any suggestions are welcome
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.
yeah there's not much you can do here besides this. The alternative is creating some traits to specialize over, but that's not gonna be great either
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.
looking good so far and adds a lot of value to how we can debug errors in workspaces. Only a couple things I noticed we can make simpler
pub(crate) async fn query_broadcast_tx( | ||
&self, | ||
method: &methods::broadcast_tx_commit::RpcBroadcastTxCommitRequest, | ||
) -> MethodCallResult< |
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.
yeah there's not much you can do here besides this. The alternative is creating some traits to specialize over, but that's not gonna be great either
workspaces/src/rpc/client.rs
Outdated
if tracing::level_enabled!(tracing::Level::DEBUG) { | ||
tracing::error!( | ||
target: "workspaces", | ||
"Calling RPC method {:?} resulted in error {:?}", | ||
method, | ||
error | ||
); | ||
} else { | ||
tracing::error!( | ||
target: "workspaces", | ||
"Submitting transaction with actions {:?} resulted in error {:?}", | ||
method.signed_transaction.transaction.actions, | ||
error | ||
); | ||
} |
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.
hmm, I'm debating this, but for errors, we should be as verbose as possible anyways. There could be cases where errors are hard to reproduce
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.
You are right. Since errors should not happen very often and they could be hard to reproduce I think it makes sense to print them verbosely all the time.
This PR adds logging to RPC client. I made the logs dynamic depending on the logging level enabled. Usually
INFO
would be enough for most users as it prints one-liners with submitted actions and the resulting status. But if a user wants more information (e.g. gas profile, logs etc), they can enableDEBUG
. Try it out by running tests withRUST_LOG=workspaces=info
.