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

log: add tokio-console support #41

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

hawkw
Copy link
Collaborator

@hawkw hawkw commented Apr 19, 2022

This branch adds initial support for setting up the Tokio console to
track tasks in a Kubert controller. In order to do so, I added a new
LogArgs struct in the log module to represent a logging
configuration that can be parsed using clap, including a flag for
enabling tokio-console support. When this flag is present, and the
requisite kubert feature and RUSTFLAGS="--cfg tokio_unstable" are
enabled, the Tokio console server is enabled.

Additionally, the new LogArgs struct handles parsing the LogFormat
and tracing_subscriber::EnvFilter arguments. The LogArgs type uses a
custom clap::FromArgs implementation so that the default value for the
filter can include the name of the binary itself.

Finally, I added support for spawning named tasks using the unstable
tokio::task::Builder API, when the tokio_unstable cfg is enabled.
These task names will show up in the console when the cfg is enabled.

Comment on lines +13 to +18
pub struct LogArgs {
/// The log format to use.
pub log_format: LogFormat,

/// The filter that determines what tracing spans and events are enabled.
pub log_level: LogFilter,
Copy link
Owner

@olix0r olix0r Apr 20, 2022

Choose a reason for hiding this comment

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

Unfortunately, using this eliminates the ability to set defaults or configure logging via the environment. I avoided providing a helper so that applications can provide their own variables, defaults, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that a good middle ground was to set a default that enabled the command name at the info level, which is why there's a manual FromArgs impl. We could also similarly set the env var based on the command name (but uppercased).

If we'd prefer not to do that, we could also pull just the filter out of the struct and have the struct just parse the log format and whether the console should be enabled.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the same issues exist with the log format (with regard to env variables, etc).

Is there any coupling between the tokio-console setting and the other log configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the same issues exist with the log format (with regard to env variables, etc).

Is there any coupling between the tokio-console setting and the other log configuration?

No, it's just also a required argument to the function for actually constructing the tracing subscriber, and I thought it was nice to generate a CLI argument for it as well. We could just as well add a LogBuilder struct or something instead of LogArgs, so that the user can define the CLI args themselves.

Copy link
Collaborator Author

@hawkw hawkw Apr 20, 2022

Choose a reason for hiding this comment

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

...I just had a crazy thought for how we could do the env vars actually

Edit: oh, nevermind, that's not going to work:

error: `&'static str` is forbidden as the type of a const generic parameter
  --> kubert/src/log.rs:13:34
   |
13 | pub struct LogArgs<const PREFIX: &'static str = "KUBERT"> {
   |                                  ^^^^^^^^^^^^
   |
   = note: the only supported types are integers, `bool` and `char`

sad!

Copy link
Owner

Choose a reason for hiding this comment

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

I think we'll want to add our own macros/main at some point in the future, but for now i'd be inclined to add this new flag as a one-off.

Is the tokio console server configurable at all? especially what port it runs on?

hawkw added 6 commits April 22, 2022 10:44
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
(we don't have any kind of kube auth token on CI)

Signed-off-by: Eliza Weisman <[email protected]>
@olix0r olix0r marked this pull request as draft December 7, 2024 22:26
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