feat: add flags and configurations for warnings #493

Rustin170506 · 2023-11-25T11:24:12Z

I addressed the TODO item.

hds

Thanks for working on this, it's a great feature to have!

For me the default lints aren't working, and I think we need to consider how the command line arguments should be.

tokio-console/src/config.rs

hawkw · 2023-11-28T21:19:20Z

tokio-console/src/config.rs

+    /// * `self-wake-percent` -- Warns when a task wakes itself more than a certain percentage of its total wakeups.
+    ///
+    /// * `lost-waker` -- Warns when a task is dropped without being woken.
+    ///
+    /// * `never-yielded` -- Warns when a task has never yielded.


it would be really nice if the list of warnings that can be enabled could be generated programmatically, somehow. i'm not sure if there's an easy way to do this, but we could potentially look into a macro or something.

i'm fine with that being something we add later, though; it doesn't have to be part of this PR.

Good idea. I will try to support it in my next PR.

tokio-console/src/config.rs

Rustin170506

🔢 Self-check

README.md

tokio-console/src/config.rs

hawkw

overall, this looks pretty close! i had some fairly minor suggestions.

tokio-console/src/config.rs

hawkw · 2023-11-29T17:34:27Z

tokio-console/src/config.rs

+    ///
+    /// * `never-yielded` -- Warns when a task has never yielded.
+    ///
+    #[clap(long = "warn", short = 'W', value_delimiter = ',', num_args = 1..)]


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.

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.

Rustin170506

🔢 Self-check

Rustin170506 · 2023-12-07T01:12:39Z

@hds @hawkw Friendly ping~
Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 · 2024-01-09T16:13:06Z

I've rebased the PR. @hds Could you please take a look when you have time? Then we can move forward to add the following --Allow flag.

hds

This looks good now from my side. @hawkw, would you mind having a look as well?

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 · 2024-01-17T13:54:41Z

The failed test caused by this issue: assert-rs/snapbox#121

I've bumped the trycmd version to fix it in this commit.

Rustin170506 · 2024-01-17T14:00:14Z

It passed now.

hawkw

Overall, this looks good to me! I had a couple fairly minor suggestions you may want to address, but nothing that would block merging IMO.

tokio-console/src/config.rs

hawkw · 2024-01-18T19:41:45Z

tokio-console/src/config.rs

+                let mut warns: Vec<KnownWarnings> = other.warnings;
+                warns.extend(self.warnings);
+                warns.sort_unstable();
+                warns.dedup();
+                warns


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.

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.

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

hawkw

looks great, thank you!

hawkw · 2024-01-19T17:41:40Z

I restarted the failed CI job, since the failure looks flaky (possibly a racy test --- cc @hds): https://github.com/tokio-rs/console/actions/runs/7585395360/job/20661161429?pr=493#step:7:64

## Description This pull request adds a new flag to `tokio-console` that allows warning lints to be selectively disabled. Additionally, you can use the value `all` to disable all warnings. For example, you can use it like this: ```sh cargo run -- --warn "self-wakes,lost-waker,never-yielded" --allow "self-wakes,never-yielded" ``` Which will warn on all three of the current lints, but then allow the `self-wakes` and `never-yielded` lints. The end result will be that only the `lost-wakers` lint will be "active". This PR follows from [comments in #493]. ## Explanation of Changes Added an `AllowedWarnings` enum to the command line interface to represent the allowed warnings. `All` for disable all warnings. `Explicit` explicitly indicates which warning can be ignored. [comments in #493]: #493 (comment)

…scriber-v0.3.0 (#557) ## 🤖 New release * `tokio-console`: 0.1.10 -> 0.1.11 * `console-api`: 0.6.0 -> 0.7.0 * `console-subscriber`: 0.2.0 -> 0.3.0 ## `tokio-console` ## tokio-console-v0.1.11 - (2024-06-10) ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Add flags and configurations for warnings ([#493](#493)) ([174883f](174883f)) - Add `--allow` flag ([#513](#513)) ([8da7037](8da7037)) ### Documented - Add note about running on Windows ([#510](#510)) ([a0d20fd](a0d20fd)) ### Fixed - Ignore key release events ([#468](#468)) ([715713a](715713a)) - Accept only `file://`, `http://`, `https://` URI ([#486](#486)) ([031bddd](031bddd)) - Fix column sorting in resources tab ([#491](#491)) ([96c65bd](96c65bd)) - Only trigger lints on async tasks ([#517](#517)) ([4593222](4593222)) - Remove duplicate controls from async ops view ([#519](#519)) ([f28ba4a](f28ba4a)) - Add pretty format for 'last woken' time ([#529](#529)) ([ea11ad8](ea11ad8)) ## `console-api` ## console-api-v0.7.0 - (2024-06-10) ### <a id = "0.7.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Documented - Fix typo in proto ([#472](#472)) ([2dd3559](2dd3559)) ### Updated - [**breaking**](#0.7.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c)) ## `console-subscriber` ## console-subscriber-v0.3.0 - (2024-06-10) ### <a id = "0.3.0-breaking"></a>Breaking Changes - **Bump tonic to 0.11 ([#547](#547 ([ef6816c](https://github.com/tokio-rs/console/commit/ef6816caa0fe84171105b513425506f25d3082af))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.10.x. ### Added - Replace target column with kind column in tasks view ([#478](#478)) ([903d9fa](903d9fa)) - Reduce retention period to fit in max message size ([#503](#503)) ([bd3dd71](bd3dd71)) - Support grpc-web and add `grpc-web` feature ([#498](#498)) ([4150253](4150253)) ### Documented - Add a grpc-web app example ([#526](#526)) ([4af30f2](4af30f2)) - Fix typo in doc comment ([#543](#543)) ([ff22678](ff22678)) ### Fixed - Don't save poll_ops if no-one is receiving them ([#501](#501)) ([1656c79](1656c79)) - Ignore metadata that is not a span or event ([#554](#554)) ([852a977](852a977)) ### Updated - [**breaking**](#0.3.0-breaking) Bump tonic to 0.11 ([#547](#547)) ([ef6816c](ef6816c))

Rustin170506 requested a review from a team as a code owner November 25, 2023 11:24

hds requested changes Nov 28, 2023

View reviewed changes

tokio-console/src/config.rs Outdated Show resolved Hide resolved

tokio-console/src/config.rs Outdated Show resolved Hide resolved

tokio-console/src/config.rs Outdated Show resolved Hide resolved

hawkw reviewed Nov 28, 2023

View reviewed changes

Rustin170506 force-pushed the rustin-patch-linters branch 5 times, most recently from d87d160 to 87559ea Compare November 29, 2023 14:14

Rustin170506 commented Nov 29, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

tokio-console/src/config.rs Show resolved Hide resolved

Rustin170506 requested review from hds and hawkw November 29, 2023 14:24

hawkw requested changes Nov 29, 2023

View reviewed changes

Rustin170506 force-pushed the rustin-patch-linters branch from 4c76370 to 9aae20f Compare November 30, 2023 12:25

Rustin170506 requested a review from hawkw November 30, 2023 12:34

Rustin170506 changed the title ~~feat: add args and configurations for linters~~ feat: add args and configurations for warnings Nov 30, 2023

Rustin170506 changed the title ~~feat: add args and configurations for warnings~~ feat: add flags and configurations for warnings Nov 30, 2023

Rustin170506 commented Nov 30, 2023

View reviewed changes

Rustin170506 added 8 commits January 9, 2024 23:57

feat: add args and configurations for linters

773d1f3

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

test: fix broken cases

d20ebbd

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

fix: add default values

06c08dc

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

feat: use delimiter

c0fcd18

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

refactor: rename to SelfWakes

4375b6b

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

refactor: rename to warn

a9723d4

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

refactor: rename to warnings

ed610e6

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

docs: add default percentage

750c4f6

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 force-pushed the rustin-patch-linters branch from 9aae20f to 750c4f6 Compare January 9, 2024 15:57

hds approved these changes Jan 9, 2024

View reviewed changes

trigger test

e5c39e0

fix: bump trycmd version to fix the stuck issue

a359250

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

hawkw approved these changes Jan 18, 2024

View reviewed changes

fix: add default_enabled_warnings

35eb524

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

hawkw approved these changes Jan 19, 2024

View reviewed changes

hawkw merged commit 174883f into tokio-rs:main Jan 19, 2024
11 checks passed

Rustin170506 mentioned this pull request Jan 29, 2024

feat: add --allow flag #513

Merged

github-actions bot mentioned this pull request Jun 7, 2024

chore: release tokio-console-v0.1.11, console-api-v0.7.0, console-subscriber-v0.3.0 #557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flags and configurations for warnings #493

feat: add flags and configurations for warnings #493

Rustin170506 commented Nov 25, 2023

hds left a comment

hawkw Nov 28, 2023

Rustin170506 Nov 29, 2023

Rustin170506 left a comment

hawkw left a comment

hawkw Nov 29, 2023

Rustin170506 Nov 30, 2023

Rustin170506 left a comment

Rustin170506 commented Dec 7, 2023

Rustin170506 commented Jan 9, 2024

hds left a comment

Rustin170506 commented Jan 17, 2024

Rustin170506 commented Jan 17, 2024

hawkw left a comment

hawkw Jan 18, 2024

Rustin170506 Jan 19, 2024

hawkw left a comment

hawkw commented Jan 19, 2024

feat: add flags and configurations for warnings #493

feat: add flags and configurations for warnings #493

Conversation

Rustin170506 commented Nov 25, 2023

hds left a comment

Choose a reason for hiding this comment

hawkw Nov 28, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 29, 2023

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Nov 29, 2023

Choose a reason for hiding this comment

Rustin170506 Nov 30, 2023

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 commented Dec 7, 2023

Rustin170506 commented Jan 9, 2024

hds left a comment

Choose a reason for hiding this comment

Rustin170506 commented Jan 17, 2024

Rustin170506 commented Jan 17, 2024

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jan 18, 2024

Choose a reason for hiding this comment

Rustin170506 Jan 19, 2024

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Jan 19, 2024