containerd / rust-extensions

Rust crates to extend containerd
https://containerd.io
Apache License 2.0
172 stars 66 forks source link

Use RUST_LOG env to configure logging if present #247

Closed jsturtevant closed 6 months ago

jsturtevant commented 6 months ago

We identified a perf impact when writing logs. In this case it would be helpful to only run with logs set at a "warning" or "error" level. We attempted to turn most logs off via containerd with containerd -l fatal but it still dumped logs.

After some investigation it appears containerd only sets a flag not a level (https://github.com/containerd/containerd/blob/7467d81987e1a54dd2a89e052ae9429a4c921e9e/core/runtime/v2/binary.go#L66-L68) and our code will default to info https://github.com/containerd/rust-extensions/blob/cc445f54c0fa8e9b35376550874422c932ad1566/crates/shim/src/logger.rs#L135-L139

changes

This reads the environment variable RUST_LOG and will use value from there if set. It can be set to any values in the log crate. Otherwise the behavior is the same. If the -debug flag is passed it will choose the highest of the two values.

This also introduces ability to configure the default setting through the shim Config object:

run::<Shim>(&id, Some(Config{default_log_level: "error", ..Default::default()}));

If all the configuration fails it will fallback to logging to to info level.

mxpv commented 6 months ago

We identified a perf impact when writing logs.

Would be curious to learn more about costs of logging to pipe/fifo.

jsturtevant commented 6 months ago

Would be curious to learn more about costs of logging to pipe/fifo.

@radu-matei @lann could you share some of your findings?

lann commented 6 months ago

I suspect that the performance issues have more to do with mutex contention in FifoLogger, but I haven't done any analysis to confirm.

mxpv commented 6 months ago

mutex contention

I wonder if going this route would be more viable instead of introducing a custom env var? parking_lot's mutex or maintaining a message queue.

jsturtevant commented 6 months ago

I think it should be both. There is an expectation from users that since they are using env crate they can adjust the log level. In the current set up, it is impossible to get trace logs from the shim or other components since we only go to the debug level.

jsturtevant commented 6 months ago

@mxpv I've logged #249 for further investigation on the logging issue.

Of the feedback in the PR above above which changes would you like to see before merging this?

mxpv commented 6 months ago

Let's have this one in. We can follow up with further improvements.