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

feat: add flags and configurations for warnings #493

Merged
merged 11 commits into from
Jan 19, 2024
12 changes: 6 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,25 @@ Options:

[env: RUST_LOG=]

-W, --warn <WARNINGS>...
Enable lint warnings.

This is a comma-separated list of warnings to enable.

Each warning is specified by its name, which is one of:

* `self-wakes` -- Warns when a task wakes itself more than a
certain percentage of its total wakeups. Default percentage is
50%.

* `lost-waker` -- Warns when a task is dropped without being
woken.

* `never-yielded` -- Warns when a task has never yielded.

[default: self-wakes lost-waker never-yielded]
[possible values: self-wakes, lost-waker, never-yielded]

--log-dir <LOG_DIRECTORY>
Path to a directory to write the console's internal logs to.

Expand Down
4 changes: 2 additions & 2 deletions tokio-console/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@ toml = "0.5"
dirs = "5"

[dev-dependencies]
# Use 1.64.0 compatible version of `trycmd@0.13.4`.
trycmd = "=0.13.4"
# Use 1.64.0 compatible version of `trycmd@0.13.6`.
trycmd = "=0.13.6"
5 changes: 5 additions & 0 deletions tokio-console/console.example.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
default_target_addr = 'http://127.0.0.1:6669/'
log = 'off'
warnings = [
'self-wakes',
'lost-waker',
'never-yielded',
]
log_directory = '/tmp/tokio-console/logs'
retention = '6s'

Expand Down
69 changes: 69 additions & 0 deletions tokio-console/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::state::tasks::Task;
use crate::view::Palette;
use crate::warnings;
use clap::builder::{PossibleValuesParser, TypedValueParser};
use clap::{ArgAction, ArgGroup, CommandFactory, Parser as Clap, Subcommand, ValueHint};
use clap_complete::Shell;
Expand Down Expand Up @@ -48,6 +50,23 @@ pub struct Config {
#[clap(long = "log", env = "RUST_LOG")]
log_filter: Option<LogFilter>,

/// Enable lint warnings.
///
/// This is a comma-separated list of warnings to enable.
///
/// Each warning is specified by its name, which is one of:
///
/// * `self-wakes` -- Warns when a task wakes itself more than a certain percentage of its total wakeups.
Rustin170506 marked this conversation as resolved.
Show resolved Hide resolved
/// Default percentage is 50%.
///
/// * `lost-waker` -- Warns when a task is dropped without being woken.
///
/// * `never-yielded` -- Warns when a task has never yielded.
///
#[clap(long = "warn", short = 'W', value_delimiter = ',', num_args = 1..)]
Copy link
Member

Choose a reason for hiding this comment

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

if i understand correctly, the use of num_args = 1.. mean that there's currently no way to disable all warnings from the CLI currently? since passing --warn with no warning names is invalid, and by default we enable all warnins.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we discussed on Discord, we will add the --Allow flag to disable warnings. @hds proposed adding the ALL value to --Allow to disable all warnings.

#[clap(default_values_t = KnownWarnings::default_enabled_warnings())]
pub(crate) warnings: Vec<KnownWarnings>,

/// Path to a directory to write the console's internal logs to.
///
/// [default: /tmp/tokio-console/logs]
Expand Down Expand Up @@ -98,6 +117,45 @@ pub struct Config {
pub subcmd: Option<OptionalCmd>,
}

/// Known warnings that can be enabled or disabled.
#[derive(clap::ValueEnum, Clone, Debug, Deserialize, Serialize, PartialEq, Eq, PartialOrd, Ord)]
#[serde(rename_all = "kebab-case")]
pub(crate) enum KnownWarnings {
SelfWakes,
LostWaker,
NeverYielded,
}

impl From<&KnownWarnings> for warnings::Linter<Task> {
fn from(warning: &KnownWarnings) -> Self {
match warning {
KnownWarnings::SelfWakes => warnings::Linter::new(warnings::SelfWakePercent::default()),
KnownWarnings::LostWaker => warnings::Linter::new(warnings::LostWaker),
KnownWarnings::NeverYielded => warnings::Linter::new(warnings::NeverYielded::default()),
}
}
}

impl fmt::Display for KnownWarnings {
Rustin170506 marked this conversation as resolved.
Show resolved Hide resolved
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
KnownWarnings::SelfWakes => write!(f, "self-wakes"),
KnownWarnings::LostWaker => write!(f, "lost-waker"),
KnownWarnings::NeverYielded => write!(f, "never-yielded"),
}
}
}

impl KnownWarnings {
fn default_enabled_warnings() -> Vec<Self> {
vec![
KnownWarnings::SelfWakes,
KnownWarnings::LostWaker,
KnownWarnings::NeverYielded,
]
}
}

#[derive(Debug, Subcommand, PartialEq, Eq)]
pub enum OptionalCmd {
/// Generate a `console.toml` config file with the default configuration
Expand Down Expand Up @@ -240,6 +298,7 @@ impl FromStr for LogFilter {
struct ConfigFile {
default_target_addr: Option<String>,
log: Option<String>,
warnings: Vec<KnownWarnings>,
log_directory: Option<PathBuf>,
retention: Option<RetainFor>,
charset: Option<CharsetConfig>,
Expand Down Expand Up @@ -428,6 +487,13 @@ impl Config {
log_directory: other.log_directory.or(self.log_directory),
target_addr: other.target_addr.or(self.target_addr),
log_filter: other.log_filter.or(self.log_filter),
warnings: {
let mut warns: Vec<KnownWarnings> = other.warnings;
warns.extend(self.warnings);
warns.sort_unstable();
warns.dedup();
warns
Comment on lines +491 to +495
Copy link
Member

Choose a reason for hiding this comment

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

Potentially, we might want to store the list of known warnings as a BTreeSet rather than as a Vec? That way, the properties that the list is sorted and contains no duplicates will be ensured at all times, rather than just when we ensure they are true.

On the other hand, I'm not sure whether that changes how it's deserialized from TOML...so, just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other hand, I'm not sure whether that changes how it's deserialized from TOML...so, just a thought.

default_values_t seems to only support the vec as the default value.

},
retain_for: other.retain_for.or(self.retain_for),
view_options: self.view_options.merge_with(other.view_options),
subcmd: other.subcmd.or(self.subcmd),
Expand All @@ -442,6 +508,7 @@ impl Default for Config {
log_filter: Some(LogFilter(
filter::Targets::new().with_default(filter::LevelFilter::OFF),
)),
warnings: KnownWarnings::default_enabled_warnings(),
log_directory: Some(default_log_directory()),
retain_for: Some(RetainFor::default()),
view_options: ViewOptions::default(),
Expand Down Expand Up @@ -677,6 +744,7 @@ impl From<Config> for ConfigFile {
default_target_addr: config.target_addr.map(|addr| addr.to_string()),
log: config.log_filter.map(|filter| filter.to_string()),
log_directory: config.log_directory,
warnings: config.warnings,
retention: config.retain_for,
charset: Some(CharsetConfig {
lang: config.view_options.lang,
Expand All @@ -699,6 +767,7 @@ impl TryFrom<ConfigFile> for Config {
Ok(Config {
target_addr: value.target_addr()?,
log_filter: value.log_filter()?,
warnings: value.warnings.clone(),
log_directory: value.log_directory.take(),
retain_for: value.retain_for(),
view_options: ViewOptions {
Expand Down
8 changes: 1 addition & 7 deletions tokio-console/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,7 @@ async fn main() -> color_eyre::Result<()> {
let (details_tx, mut details_rx) = mpsc::channel::<TaskDetails>(2);

let mut state = State::default()
// TODO(eliza): allow configuring the list of linters via the
// CLI/possibly a config file?
.with_task_linters(vec![
warnings::Linter::new(warnings::SelfWakePercent::default()),
warnings::Linter::new(warnings::LostWaker),
warnings::Linter::new(warnings::NeverYielded::default()),
])
.with_task_linters(args.warnings.iter().map(|lint| lint.into()))
.with_retain_for(retain_for);
let mut input = Box::pin(input::EventStream::new().try_filter(|event| {
future::ready(!matches!(
Expand Down
19 changes: 19 additions & 0 deletions tokio-console/tests/cli-ui.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,25 @@ Options:

[env: RUST_LOG=]

-W, --warn <WARNINGS>...
Enable lint warnings.

This is a comma-separated list of warnings to enable.

Each warning is specified by its name, which is one of:

* `self-wakes` -- Warns when a task wakes itself more than a
certain percentage of its total wakeups. Default percentage is
50%.

* `lost-waker` -- Warns when a task is dropped without being
woken.

* `never-yielded` -- Warns when a task has never yielded.

[default: self-wakes lost-waker never-yielded]
[possible values: self-wakes, lost-waker, never-yielded]

--log-dir <LOG_DIRECTORY>
Path to a directory to write the console's internal logs to.

Expand Down