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

Lower Hostcall error log level to DEBUG #314

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

awilliams-fastly
Copy link
Contributor

@awilliams-fastly awilliams-fastly commented Sep 25, 2023

The Fastly CLI executes Viceroy with a -v argument, which corresponds to INFO log level. This change drops the log level of hostcall errors to DEBUG, so that they're not displayed to users running apps via fastly compute serve.

As an example, BufferLengthError is a sentinel error used to indicate to the caller that the passed in buffer was too small. The caller can then try again with an appropriately sized buffer.

`BufferLengthError` is a sentinel error used to indicate to the caller that the passed in buffer was [too small](https://github.com/fastly/Viceroy/blob/f62eb94c470f39df1d25cba3ffb028a3289bbf15/lib/src/wiggle_abi/secret_store_impl.rs#L110-L113). The caller can then try again with an appropriately sized buffer.

The Fastly CLI executes Viceroy with a [`-v`](https://github.com/fastly/cli/blob/24761dcccd81d16e3873c1fa65e03fd83486442f/pkg/commands/compute/serve.go#L534) argument, which corresponds to `INFO` log level. This change drops the log level of `BufferLengthError` errors to `DEBUG`, so that they're not displayed to users running apps via `fastly compute serve`.
@joeshaw
Copy link
Member

joeshaw commented Sep 25, 2023

My 2¢: I think it would be better to lower all the "hostcall yielded" error messages to DEBUG, as you'll see them for various other hostcalls when, for example, running the Go SDK's integration tests.

The fact that a hostcall returned an error to an SDK isn't indicative of a bug in the runtime, or the hostcall. In the case with BufferLengthError it's a negotiation between the runtime and the SDK of the appropriate buffer size, an implementation detail. But other times it may be caused by unimplemented behavior in Viceroy. Perhaps most commonly it's caused by user error, passing in bad data. Those errors should be passed through and reflected in errors by the SDK in the guest code.

@awilliams-fastly awilliams-fastly changed the title Lower BufferLengthError's log level to DEBUG Lower Hostcall error log level to DEBUG Sep 25, 2023
@draco2003
Copy link

+1 for this fix.

Getting a similar error when code references the FetchEvent.client.tlsProtocol and run locally
image

Copy link
Member

@joeshaw joeshaw left a comment

Choose a reason for hiding this comment

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

👍

@JakeChampion JakeChampion merged commit 8fba2c6 into main Oct 6, 2023
7 checks passed
@JakeChampion JakeChampion deleted the awilliams/silence-buf-len-err branch October 6, 2023 09:48
cmckendry pushed a commit to 1stdibs/Viceroy that referenced this pull request Feb 8, 2024
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.

4 participants