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

[QUESTION] Should log level be included in HostData? #140

Closed
jordan-rash opened this issue Feb 15, 2023 · 20 comments · Fixed by wasmCloud/wasmcloud-otp#558
Closed

[QUESTION] Should log level be included in HostData? #140

jordan-rash opened this issue Feb 15, 2023 · 20 comments · Fixed by wasmCloud/wasmcloud-otp#558
Assignees

Comments

@jordan-rash
Copy link
Member

jordan-rash commented Feb 15, 2023

For the same reason that StructuredLogging is included...the provider knows what level to log at.

/// True if structured logging is enabled for the host. Providers should use the same setting as the host.
#[serde(default)]
pub structured_logging: bool,

WASMCLOUD_STRUCTURED_LOG_LEVEL <- looking for this setting in HostData

Or am I missing it somewhere?

@connorsmith256
Copy link
Contributor

I had to double-check how this works today, since RUST_LOG isn't found anywhere in the OTP host codebase.

Today, provider processes inherit environment variables from their parent, the host process. So if you start the host with RUST_LOG=debug, every Rust provider instance started on that host will also have that environment variable set when starting, so they will log at the debug level.

I'm guessing you're looking for an equivalent for a Go provider? 😉 I'm not sure if there's a standard env var like there is for Rust, but the same process would work.

However, to answer your question: yes, I do think log level should be explicitly passed to the provider rather than implicitly via a language-specific env var. I've been interested in writing an RFC around provider configuration more generally. I'm particularly interested in allowing runtime changes to logging config for providers/actors!

@jordan-rash
Copy link
Member Author

jordan-rash commented Feb 16, 2023

Yea, I noticed that some things where using RUST_LOG while others were using, well nothing. I think the WASMCLOUD_STRUCTURED_LOG_LEVEL is a good place to start, but maybe without the "STRUCTURED_LOG" part (so it applies to json or text based logs.

I could just have the go sdks look for RUST_LOG env, but i'm afraid when I run for Gopher President in the future, my opposition will find that and my dreams will be shattered.

@jordan-rash
Copy link
Member Author

jordan-rash commented Feb 17, 2023

@connorsmith256 I remember when I was creating the wazero host that the hostdata has this EnvValues item

pub env_values: HostEnvValues,

Which is documented as

https://github.com/wasmCloud/interfaces/blob/cd6580dc7505c7aa25cf3fa066a0c0fb112019b7/core/wasmcloud-core.smithy#L192-L193

I assume that would have things like RUST_LOG as well as WASMCLOUD_STRUCTRED_LOG_LEVEL in it. I can try and confirm that later....so my question now is, what determines which variables get pulled into the host_data struct and what just rides that env_values map?

@connorsmith256
Copy link
Contributor

@autodidaddict
Copy link
Member

No implicit env vars are passed for security reasons. That empty list was there in case we had specific wildcards or things that we wanted to forward

@connorsmith256
Copy link
Contributor

To answer Jordan's original question, how do providers get access to the RUST_LOG env var (assuming they do?)

@jordan-rash
Copy link
Member Author

jordan-rash commented Feb 17, 2023

Not specifically RUST_LOG as much as WASMCLOUD_STRUCTURED_LOG_LEVEL....or more generically something like WASMCLOUD_LOG_LEVEL

@brooksmtownsend
Copy link
Member

So providers can access RUST_LOG directly from the environment today, but we could also consider giving an explicit log level so that it's not implicitly just available.

@connorsmith256
Copy link
Contributor

connorsmith256 commented Feb 22, 2023

@brooksmtownsend Now I'm confused more... I found propagated_env_vars in the OTP host, but RUST_LOG isn't in there. How are variables like RUST_LOG, WASMCLOUD_STRUCTURED_LOG_LEVEL, and WASMCLOUD_STRUCTURED_LOGGING_ENABLED making it to the provider process?

@jordan-rash
Copy link
Member Author

WASMCLOUD_STRUCTURED_LOGGING_ENABLED is in HostData....the others aren't.

I can't read the Elixr code, but whereever its starting a provider, it should be handing it a JSON of the HostData as arg[1]

@connorsmith256
Copy link
Contributor

HostData has a structured_logging field

It appears the host is passing a enable_structured_logging field

HostData is passed via stdin to the process here

As for env vars, it appears only propagated_env_vars are included

So, my confusion:

  • are providers correctly receiving the structured logging field from the host data? If so, how, since the field names don't appear to line up?
  • where are env vars like RUST_LOG getting passed to the provider process?

@jordan-rash
Copy link
Member Author

jordan-rash commented Feb 23, 2023

I can speak for Go Providers, they receive the HostData, from the Host, via standard in here. Then it is parsed into the core.HostData

It is my understanding that the only place a provider provided initialization data, is that stdin process you noted above (for all providers)

So I would assume, in order to add a logging level, we would need to add a LogLevel field to HostData for the providers to ingest when they start

To your second question, I don't think they are being passed. I think current providers are picking the the ENV straight from the host they are started on

@connorsmith256
Copy link
Contributor

@jordan-rash what you said makes sense, but it contradicts my observations, which are when starting a host with RUST_LOG=debug wash up, rust providers started on the host do indeed log at the debug level. I'm just very confused as to how that's working today 😅

@brooksmtownsend @autodidaddict ?

@jordan-rash
Copy link
Member Author

Yea, because, correct me if I wrong, they are all being ran in the session you just started wash in. So by running wash up with that ENV, all child pids, which include the started providers, are running in a session with RUST_LOG=debug set. At that point, does not every invocation of debug!() fire?

@connorsmith256
Copy link
Contributor

Maybe? The Port docs don't mention how child processes inherit environment variables. Under the hood it looks like elixir is calling :erlang.open_port. The docs there say about env:

The environment of the started process is extended using the environment specifications in Env.

I'm not sure whether to infer that "extended" means the parent process is passing all of its env vars to the child process. If that's the case, is it a security concern like @autodidaddict alluded to here?

@brooksmtownsend
Copy link
Member

Yeah, that's correct @jordan-rash. The Rust providers get the env var RUST_LOG from the session you start wash in, so they kind of implicitly figure out their own log level. I think we should hand down log level in the HostData so that regardless of provider language we know where to find it (RUST_LOG means nothing to Go providers, for example.)

I am concerned with passing the full set of environment variables and didn't know we still did that

@jordan-rash
Copy link
Member Author

I am concerned with passing the full set of environment variables and didn't know we still did that

Per @autodidaddict comment, I dont think the host does...the field has always been blank when I checked it.

@jordan-rash
Copy link
Member Author

jordan-rash commented Mar 14, 2023

Not sure why closed this if the 558 PR says

This doesn't fix #140, but it is related

This seems to still be an issue?

@connorsmith256
Copy link
Contributor

I determined the answer to your question is "yes" :)

The issue for tracking that work is here: #145

@jordan-rash
Copy link
Member Author

Thanks Connor :-)

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 a pull request may close this issue.

6 participants