-
Notifications
You must be signed in to change notification settings - Fork 37
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
🧹 Tidy up logging and error output #37
Conversation
variant for unavailable features.
lib/src/execute.rs
Outdated
Level::INFO, | ||
"Request completed, using {} bytes of wasm heap", | ||
info!( | ||
"request completed using {} bytes of WebAssembly heap", |
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.
Maybe a future improvement and out of scope of this PR, however, it would be nice if the n bytes
could be a more human-friendly representation I.e. 1 Kb, 1Mb, 1Gb etc. Such as bytesize.
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.
Sounds great, I'll try to sneak that one in as well!
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.
Alright, I added a commit to make the heap size human-friendly, thank you!
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.
These changes look good to me. I'm glad we have a better error now to let people know we don't support something on local testing yet!
This PR addresses a few minor UI nits (reported on Slack), as well as a more significant issue: rather than panicking when an unimplemented hostcall is used, we instead yield an error back to the guest. The error reporting infrastructure will print a friendly message, though in most cases the SDK will subsequently panic.