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(console): surface dropped events if there are any #316

Merged
merged 6 commits into from
Apr 12, 2022

Conversation

bobrik
Copy link
Contributor

@bobrik bobrik commented Mar 31, 2022

During normal operation it won't show anything:

connection: http://127.0.0.1:6669/ (CONNECTED)

But if there are any drops detected, it will show a cumulative amount:

connection: http://127.0.0.1:6669/ (CONNECTED) dropped events: 2507

image

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, this PR looks like a great start!

One thing I notice is that we're currently only showing the count of dropped async ops, and not dropped tasks or resources. We should probably fix that.

Also, some ideas for the future:

  • I had initially wondered if it might be nice to show a count of dropped events as a warning in the warnings UI box, but I think showing it here is fine for now.
  • I also wonder if we want to consider showing the number of dropped events over a time window, like 43 events dropped over the last 20 seconds (3489 dropped events total) or something?

But, those can both be addressed in follow-ups --- for now, I think it's important to just actually start showing this data, as it's not currently exposed at all...

tokio-console/src/main.rs Outdated Show resolved Hide resolved
During normal operation it won't show anything:

```
connection: http://127.0.0.1:6669/ (CONNECTED)
```

But if there are any drops detected, it will show a cumulative amount:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped events: 2507
```
@bobrik bobrik force-pushed the ivan/dropped-ui branch from f0beef3 to 2b20440 Compare April 5, 2022 02:57
@hawkw
Copy link
Member

hawkw commented Apr 8, 2022

@bobrik FYI, you generally don't need to force push new changes to your PRs. We merge pull requests by squashing, so individual work-in-progress commits to a branch are fine. See https://github.com/tokio-rs/console/blob/main/CONTRIBUTING.md#commit-squashing for details.

tokio-console/src/main.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Apr 12, 2022

@bobrik quick ping --- are you still planning to work on this? I'd like to get it merged! If you don't have the time to finish this, I'd be happy to wrap it up myself, just let me know!

bobrik added 2 commits April 12, 2022 14:23
Before:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped events: 2507
```

After:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped async_ops_state: 29172 dropped tasks_state: 72328 dropped resources_state: 240995304
```
The numbers are much more reasonable this way.

Before:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped async_ops_state: 29172 dropped tasks_state: 72328 dropped resources_state: 240995304
```

After:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped async_ops_state: 76 dropped tasks_state: 3388 dropped resources_state: 28956
```
@bobrik
Copy link
Contributor Author

bobrik commented Apr 12, 2022

Apologies for the delay. I pushed separate counters and a bugfix for overcounting of resources_state drops.

@bobrik bobrik requested a review from hawkw April 12, 2022 21:35
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

I had a couple minor suggestions, but overall, this looks good to me. Thanks for working on it!

tokio-console/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 130 to 150
let dropped_async_ops_state = state.async_ops_state().dropped_events();
if dropped_async_ops_state > 0 {
header_text.0.push(Span::styled(
format!(" dropped async_ops_state: {}", dropped_async_ops_state),
view.styles.fg(Color::Red),
));
}
let dropped_tasks_state = state.tasks_state().dropped_events();
if dropped_tasks_state > 0 {
header_text.0.push(Span::styled(
format!(" dropped tasks_state: {}", dropped_tasks_state),
view.styles.fg(Color::Red),
));
}
let dropped_resources_state = state.resources_state().dropped_events();
if dropped_resources_state > 0 {
header_text.0.push(Span::styled(
format!(" dropped resources_state: {}", dropped_resources_state),
view.styles.fg(Color::Red),
));
}
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: we could probably save a little horizontal real estate by formatting the text like

dropped XXXX tasks, YYYY resources, ZZZZ async ops

or something?

instead of repeating the word "dropped" three times, we could check if any counter is non-zero, print a single "dropped", and then print the actual counts. not a blocker, though, as the current code should fit on most terminals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, updated.

bobrik and others added 2 commits April 12, 2022 14:59
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Before:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped async_ops_state: 76 dropped tasks_state: 3388 dropped resources_state: 28956
```

After:

```
connection: http://127.0.0.1:6669/ (CONNECTED) dropped: 48 async_ops, 2073 tasks, 17852 resources
```
@hawkw hawkw enabled auto-merge (squash) April 12, 2022 22:10
@hawkw hawkw disabled auto-merge April 12, 2022 22:10
@hawkw hawkw enabled auto-merge (squash) April 12, 2022 22:10
@hawkw hawkw merged commit 16df5d3 into tokio-rs:main Apr 12, 2022
hawkw added a commit that referenced this pull request Apr 13, 2022
<a name="0.1.4"></a>
## 0.1.4 (2022-04-13)

#### Features

*  add autogenerated example config file to docs (#327) ([79da280](79da280))
*  add `gen-config` subcommand to generate a config file (#324) ([e034f8d](e034f8d))
*  surface dropped event count if there are any (#316) ([16df5d3](16df5d3))
*  read configuration options from a config file (#320) ([defe346](defe346), closes [#310](310))
hawkw added a commit that referenced this pull request Apr 13, 2022
<a name="0.1.4"></a>
## 0.1.4 (2022-04-13)

#### Features

*  add autogenerated example config file to docs (#327) ([79da280](79da280))
*  add `gen-config` subcommand to generate a config file (#324) ([e034f8d](e034f8d))
*  surface dropped event count if there are any (#316) ([16df5d3](16df5d3))
*  read configuration options from a config file (#320) ([defe346](defe346), closes [#310](310))
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