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

Don't trace tasks spawned through the console server #314

Merged
merged 2 commits into from Apr 8, 2022

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Mar 24, 2022

The console subscriber's server generates a lot of async activity,
introspecting or logging that drowns out the activity the program
has outside of instrumentation.

Set a thread-local subscriber that drops events coming from the server thread.

Implementation requires a simple wrapper for NoSubscriber, because the dispatch implementation uses it as a sentinel value (tokio-rs/tracing#2001 explains the history) and logs through the global dispatcher when a thread-local NoSubscriber is set.

@g2p
Copy link
Contributor Author

g2p commented Apr 6, 2022

(Now that tokio-rs/tracing#2001 is in a release, console could also require that release instead of the NoSubscriber wrapper)

@seanmonstar
Copy link
Member

I kinda think the task being shown is useful, since it could help you realize how much the console is affecting your app, or reveal potential bugs in the console server. But I understand wanting to hide it usually. I don't know what the best solution is. Maybe we can dim it, or have a thing at the bottom "2 tasks hidden by default", or some other idea?

@g2p
Copy link
Contributor Author

g2p commented Apr 6, 2022

I could provide a way to make this configurable, I would put it in console_subscriber::Builder, but the default would be to hide so that a console user gets something relevant with less effort and knowledge. A runtime annotation that could be filtered client-side is also much harder than the thread-local filter in this PR, the annotations don't exist right now.

Aside: Having hyper tasks coming from the subscriber definitely made the console unusable for me at release. Currently tasks like this cannot be distinguished if the app also uses hyper, though better context annotations will hopefully be provided at some point.

Figuring out console bugs or overheads is more relevant to a tokio-console developer, someone also more able to find and configure an "enable_self_trace" flag.

@hawkw
Copy link
Member

hawkw commented Apr 7, 2022

I agree that it feels a bit philosophically wrong to totally exclude the console's tasks, because then we are not reporting a completely accurate view of the application's real state. But, pragmatically, if the console tasks make using the console more difficult, I'm willing to consider excluding them --- doing so may also improve performance and/or reduce dropped events. So, I agree with @seanmonstar --- we should allow the user to choose whether the console's tasks are recorded or not.

I think in an ideal world, we would want to configure whether or not the console's tasks are recorded in the console UI, using a general-purpose runtime filtering system as described in #131. However, implementing that is a pretty big project --- it would require UI work as well as potential changes to the wire protocol and console-subscriber crates. So, if the console tasks are currently causing problems for users trying to debug their applications, I think a builder option for enabling recording the console's tasks would be a fine stop-gap solution until the more general-purpose filtering system is implemented.

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 think that, rather than always disabling recording on the console-subscriber thread, we should add a builder option to control whether or not it's disabled, and only set NoSubscriber when that builder option is set. I would be fine with making the console-subscriber thread disabled by default, but I think it ought to be possible to enable recording it.

Also, I commented with a couple smaller suggestions, but neither of those are hard blockers for merging this branch.

console-subscriber/src/builder.rs Outdated Show resolved Hide resolved
console-subscriber/src/builder.rs Outdated Show resolved Hide resolved
@g2p g2p force-pushed the no-self-trace branch 3 times, most recently from 206d196 to 42a8afe Compare April 8, 2022 08:22
@g2p
Copy link
Contributor Author

g2p commented Apr 8, 2022

Addressed comments and made this configurable.

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.

looks good to me! thank you!

console-subscriber/src/builder.rs Outdated Show resolved Hide resolved
console-subscriber/src/builder.rs Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Apr 8, 2022

@g2p mind merging the main branch into this PR so we can get a passing CI build? I'll be happy to merge it afterwards --- it looks like I can't update this branch from main in the GitHub UI, for some reason.

Edit: never mind, I took care of it.

@hawkw hawkw enabled auto-merge (squash) April 8, 2022 17:30
Enabling the console subscriber starts a server that generates a lot of
async activity.  Introspecting or logging that can drown out the
activity the program has outside of instrumentation.

Provide a new builder method, enable_self_trace, to control whether to
also trace events that come with enabling the tokio console.

If self trace is not enabled, set a thread-local subscriber that drops
events coming from the server thread.

Raise the requirement on tracing-core so that setting a per-thread
NoSubscriber is effective.
@g2p
Copy link
Contributor Author

g2p commented Apr 8, 2022

(I just rebased to appease CI. Feel free to merge when that works)

auto-merge was automatically disabled April 8, 2022 18:09

Head branch was pushed to by a user without write access

@hawkw hawkw enabled auto-merge (squash) April 8, 2022 18:16
@hawkw hawkw merged commit 0045e9b into tokio-rs:main Apr 8, 2022
hawkw added a commit that referenced this pull request Apr 11, 2022
<a name="0.1.4"></a>
## 0.1.4  (2022-04-11)

#### Bug Fixes

*  fix memory leak from historical `PollOp`s (#311)
   ([9178ecf](9178ecf), closes [#256](256))

#### Features

* **console-api:**  Update `tonic` to `0.7` (#318) ([83d8a87](83d8a87))
*  don't trace tasks spawned through the console server (#314)
   ([0045e9b](0045e9b))
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.

None yet

3 participants