Stream: git-wasmtime

Topic: wasmtime / PR #9821 allow customizing log prefixes for wa...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 14 2024 at 01:44):

jzhn opened PR #9821 from jzhn:main to bytecodealliance:main:

What

As discussed in feature request: https://github.com/bytecodealliance/wasmtime/issues/9799, wasmtime serve command always prefixes stdout and stderr logs of wasi-http handler with hardcoded format. This prevents user to implement consistent logging formats like json logging.

This PR adds two new optional flags, --logging-prefix-stdout and --logging-prefix-stderr, which allows users to customize the prefix, and possibly removing the prefix by providing empty strings. When the new flags are not provided, existing hehaviour is preserved (covered by existing test cases).

Test

Alternatives

I know adding flags to CLI creates long term promise, so I'm trying to be careful here and have considered a few alternatives.

boolean flags vs string

we could simply provide new boolean flags like --no-logging-prefix, that works well for the original purpose of allowing users to disable log prefixes. However, in the future, we might want to allow users to specify log prefixes with templates, as the { req_id } could be useful. Using string flags preserves the possibility of such feature.

Debug options

The existing DebugOptions already contains some customization about logging
https://github.com/bytecodealliance/wasmtime/blob/da93f647974518e111561b3b451ef8c4a576bf20/crates/cli-flags/src/lib.rs#L241-L244

While we could add the flags there, I feel the new flags in this PR are specific to wasmtime serve command. So adding them to the common flags might confuse people for other commands.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 14 2024 at 01:44):

jzhn requested wasmtime-core-reviewers for a review on PR #9821.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 14 2024 at 01:44):

jzhn requested alexcrichton for a review on PR #9821.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 14 2024 at 01:45):

jzhn edited PR #9821:

What

As discussed in feature request: https://github.com/bytecodealliance/wasmtime/issues/9799, wasmtime serve command always prefixes stdout and stderr logs of wasi-http handler with hardcoded format. This prevents user to implement consistent logging formats like json logging.

This PR adds two new optional flags, --logging-prefix-stdout and --logging-prefix-stderr, which allows users to customize the prefix, and possibly removing the prefix by providing empty strings. When the new flags are not provided, existing hehaviour is preserved (covered by existing test cases).

Test

Alternatives

I know adding flags to CLI creates long term promise, so I'm trying to be careful here and have considered a few alternatives.

boolean flags vs string

we could simply provide new boolean flags like --no-logging-prefix, that works well for the original purpose of allowing users to disable log prefixes. However, in the future, we might want to allow users to specify log prefixes with templates, as the { req_id } could be useful. Using string flags preserves the possibility of such feature.

Debug options

The existing DebugOptions already contains some customization about logging
https://github.com/bytecodealliance/wasmtime/blob/da93f647974518e111561b3b451ef8c4a576bf20/crates/cli-flags/src/lib.rs#L241-L244

While we could add the flags there, I feel the new flags in this PR are specific to wasmtime serve command. So adding them to the common flags might confuse people for other commands.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 14 2024 at 01:49):

jzhn updated PR #9821.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 15:47):

alexcrichton submitted PR review:

Thanks! I think the documentation may want to be updated though because using {req_id} could be confusing as the prefix here doesn't actually support interpolation of req_id or other variables. If there's no need to have acustom prefixes it might also be reasonable to have a single flag to disable prefixes perhaps?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 18:01):

jzhn commented on PR #9821:

Thanks! I think the documentation may want to be updated though because using {req_id} could be confusing as the prefix here doesn't actually support interpolation of req_id or other variables. If there's no need to have acustom prefixes it might also be reasonable to have a single flag to disable prefixes perhaps?

Thanks for the review. I see that you are suggesting two directions, one to keep the custom prefix flags but update the documentation to remove mention of {req_id} to avoid confusion, the other to replace those custom prefix flags and use a simple boolean flag like --no-logging-prefix to disable the prefix.

I'm fine with either approach, so please just let me know which is your preference and I'll update accordingly. thanks :)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 18:05):

alexcrichton commented on PR #9821:

Ah yes sorry, I also don't have much of a preference. Since your goal is to just disable the prefix why don't we add that for now and consider adding customizable ones later.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 18:47):

jzhn updated PR #9821.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 18:52):

jzhn edited PR #9821:

What

As discussed in feature request: https://github.com/bytecodealliance/wasmtime/issues/9799, wasmtime serve command always prefixes stdout and stderr logs of wasi-http handler with hardcoded format. This prevents user to implement consistent logging formats like json logging.

This PR adds an optional flags, --no-logging-prefix, to stop adding log prefixes. When the new flags are not provided, existing hehaviour is preserved.

Test

Alternatives

I know adding flags to CLI creates long term promise, so I'm trying to be careful here and have considered a few alternatives.

boolean flags vs string

(original implementation in this PR) To allow better flexibility and future proof, we could add two string flags, --logging-prefix-stdout and --logging-prefix-stderr, which allows users to customize the prefix, and possibly removing the prefix by providing empty strings. This enables future possibilities like allow users to specify log prefixes with templates, as the { req_id } could be useful. __this is replaced with simple boolean flag of --no-logging-prefix to avoid over engineering.__

Debug options

The existing DebugOptions already contains some customization about logging
https://github.com/bytecodealliance/wasmtime/blob/da93f647974518e111561b3b451ef8c4a576bf20/crates/cli-flags/src/lib.rs#L241-L244

While we could add the flags there, I feel the new flags in this PR are specific to wasmtime serve command. So adding them to the common flags might confuse people for other commands.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 19:05):

jzhn requested alexcrichton for a review on PR #9821.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 21:31):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 16 2024 at 21:50):

alexcrichton merged PR #9821.

Last updated: Dec 23 2024 at 12:05 UTC