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

BREAKING: pass log_level in HostData #610

Merged
merged 5 commits into from
Apr 14, 2023

Conversation

connorsmith256
Copy link
Contributor

@connorsmith256 connorsmith256 commented Apr 10, 2023

Feature or Problem

This passes log_level to providers. See wasmCloud/weld#148 for context

Related Issues

wasmCloud/weld#148

Release Information

v0.63.0, since this is technically a breaking change. See below.

Consumer Impact

See the testing section of wasmCloud/weld#148. Notice that after this change, RUST_LOG is ignored.

Note: I find it odd that the source field is called structured_log_level, and that Elixir logging is controlled via mix environment without structured logging enabled. I propose we make this a breaking change: rename this field to just log_level, proxy it through to providers, and also respect it when structured logging is not enabled

Update: we decided the following:

  • WASMCLOUD_STRUCTURED_LOG_LEVEL is now deprecated, but will continue to be parsed
  • WASMCLOUD_LOG_LEVEL now controls logging for structured and unstructured logs
    • this value is proxied to providers
    • If not set, it defaults to the deprecated env var, or to the mix config level (debug) if neither is set

Testing

Manual Verification

See testing section of wasmCloud/weld#148

Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
@connorsmith256
Copy link
Contributor Author

After talking with @thomastaylor312, he agreed that renaming to log_level for elixir logs for consistency makes sense. Does anyone disagree? He also proposed that we remain backwards-compatible for a time™️ and continue to parse the old env var name, as well as the new name (WASMCLOUD_LOG_LEVEL)

@brooksmtownsend
Copy link
Member

@connorsmith256 I like the idea of using log_level and the backwards compatible logic, just output a warning if people use the old one so that they get a notice to update

The new config value applies to structured and unstructured logging.
To maintain backwards-compatibility, the old environment variable is
mapped to the new field

Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
@connorsmith256 connorsmith256 marked this pull request as ready for review April 14, 2023 18:16
@connorsmith256 connorsmith256 changed the title pass log_level in HostData BREAKING: pass log_level in HostData Apr 14, 2023
Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
@connorsmith256 connorsmith256 merged commit a5d0107 into wasmCloud:main Apr 14, 2023
11 checks passed
@connorsmith256 connorsmith256 deleted the feat/provider-log-level branch April 14, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants