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

allow customizing log prefixes for wasmtime serve command #9821

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

jzhn
Copy link
Contributor

@jzhn jzhn commented Dec 14, 2024

What

As discussed in feature request: #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

  • Added test case to cover new flags
  • Existing test case covers the existing behaviour when flags are absent.

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

/// Configure whether logging is enabled.
pub logging: Option<bool>,
/// Configure whether logs are emitted to files
pub log_to_files: Option<bool>,

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.

@jzhn jzhn requested a review from a team as a code owner December 14, 2024 01:44
@jzhn jzhn requested review from alexcrichton and removed request for a team December 14, 2024 01:44
Signed-off-by: Joseph Zhang <jzhang@absolute.com>
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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?

@jzhn
Copy link
Contributor Author

jzhn commented Dec 16, 2024

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 :)

@alexcrichton
Copy link
Member

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.

@jzhn jzhn requested a review from alexcrichton December 16, 2024 19:05
@alexcrichton alexcrichton added this pull request to the merge queue Dec 16, 2024
Merged via the queue into bytecodealliance:main with commit d3f05ee Dec 16, 2024
42 checks passed
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.

2 participants