feat: add flags and configurations for warnings #493

0xPoe · 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

0xPoe

🔢 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.

0xPoe

🔢 Self-check

0xPoe · 2023-12-07T01:12:39Z

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

Signed-off-by: hi-rustin <[email protected]>

0xPoe · 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 <[email protected]>

0xPoe · 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.

0xPoe · 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 <[email protected]>

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))

0xPoe 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

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

0xPoe commented Nov 29, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

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

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

hawkw requested changes Nov 29, 2023

View reviewed changes

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

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

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

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

0xPoe commented Nov 30, 2023

View reviewed changes

0xPoe added 8 commits January 9, 2024 23:57

feat: add args and configurations for linters

773d1f3

Signed-off-by: hi-rustin <[email protected]>

test: fix broken cases

d20ebbd

Signed-off-by: hi-rustin <[email protected]>

fix: add default values

06c08dc

Signed-off-by: hi-rustin <[email protected]>

feat: use delimiter

c0fcd18

Signed-off-by: hi-rustin <[email protected]>

refactor: rename to SelfWakes

4375b6b

Signed-off-by: hi-rustin <[email protected]>

refactor: rename to warn

a9723d4

Signed-off-by: hi-rustin <[email protected]>

refactor: rename to warnings

ed610e6

Signed-off-by: hi-rustin <[email protected]>

docs: add default percentage

750c4f6

Signed-off-by: hi-rustin <[email protected]>

0xPoe 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 <[email protected]>

hawkw approved these changes Jan 18, 2024

View reviewed changes

fix: add default_enabled_warnings

35eb524

Signed-off-by: hi-rustin <[email protected]>

hawkw approved these changes Jan 19, 2024

View reviewed changes

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

0xPoe 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

Uh oh!

feat: add flags and configurations for warnings #493

feat: add flags and configurations for warnings #493

Uh oh!

Conversation

0xPoe commented Nov 25, 2023

Uh oh!

hds left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

0xPoe Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

0xPoe Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

0xPoe commented Dec 7, 2023

Uh oh!

0xPoe commented Jan 9, 2024

Uh oh!

hds left a comment

Choose a reason for hiding this comment

Uh oh!

0xPoe commented Jan 17, 2024

Uh oh!

0xPoe commented Jan 17, 2024

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

0xPoe Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw commented Jan 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants