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

add support for tokio-console behind a hidden flag and feature #9665

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Dec 17, 2021

Motivation

  • This PR adds a feature that has not yet been specified.

the tokio-console was release recently, in beta mode: https://tokio.rs/blog/2021-12-announcing-tokio-console, and in my experience it can be very helpful debugging problems and finding misbehaving tasks. Here is what it looks like in this commit:
image

and it has already found misbehaving futures that are not yielding:
image

This pr also refactors our use of tracing. We give up the flexibility of EnvFilter to use a Targets per-layer filter, so that events that are not for human consumption are always passed along (to things like the console-subscriber, and our own metrics subscriber): https://docs.rs/tracing-subscriber/0.3.3/tracing_subscriber/struct.EnvFilter.html#examples has more info about this difference.

It is entirely possible that we prefer to require users of this flag to have to explicitly add the correct trace directive themselves with --log-filter or the env var.

Also, because .with returns different, stacked types, its hard to come up with a clean way to optionally add this, so i did the best I could


to run this, you must use something like this:
RUSTFLAGS="--cfg tokio_unstable cargo run --features tokio-console -- --dev --tokio-console . This unfortunately clashes with things like rust-analyzer, so you will often be rebuilding, but this can be added to .cargo/config to avoid this.

then, use tokio-console in another terminal to get the output. In the future, I may go into enforcing that all our tokio tasks are named, so that we can improve this. It might also be interesting to see if we could get timely to spit out the relevant info so that timely operators, which are similarly async, show up on the console


Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR adds a release note for any user-facing behavior changes.

This change is Reviewable

@benesch benesch self-requested a review December 20, 2021 04:02
@benesch
Copy link
Member

benesch commented Dec 20, 2021

This is in my queue to review!

@guswynn guswynn marked this pull request as ready for review December 20, 2021 22:57
@ruchirK ruchirK self-requested a review December 21, 2021 17:16
benesch added a commit to benesch/materialize that referenced this pull request Dec 22, 2021
Extracted from MaterializeInc#9665.

The idea here is to perform the filtering specified by the
`--log-filter` argument at the last possible moment, right before the
message is emitted to disk. This ensures that other log messages (e.g.,
trace and debug logs) are available to other parts of the logging
subsystem that may want to do something smart with them.

This already pays dividends in that the mz_log_message_total metric can
keep track of the log messages that don't get written out to the file.
It also paves the way for integrations with e.g. the Tokio console (see

Co-authored-by: Gus Wynn <[email protected]>
@benesch
Copy link
Member

benesch commented Dec 22, 2021

Ok, sorry for the long delay! I split out the Targets stuff into its own PR (#9725) because I think that's a good idea in its own right and is ready to merge.

It is entirely possible that we prefer to require users of this flag to have to explicitly add the correct trace directive themselves with --log-filter or the env var.

One thing I've wanted to be able to do for a while is dynamically toggle/view the logs at different levels via the web interface (i.e., http://materialized:6875).

Also, because .with returns different, stacked types, its hard to come up with a clean way to optionally add this, so i did the best I could

In situations like this, it comes out pretty clean if you do:

let stack = tracing_subscriber::registry()...

#[cfg(feature = "tracing_subscriber")]
let stack = stack.with(console::spawn());

stack.init();

So, the big hurdle for me here is the additional dependencies on tonic and prost. prost-build in particular stresses me out because it bundles binary blobs for protoc: https://github.com/tokio-rs/prost/tree/master/prost-build/third-party/protobuf. Besides being a security risk because they're impossible to audit, they also introduce a sleeper dependency on protoc... but only on the platforms where a pre-compiled binary blob isn't shipped with prost-build. To date we've gone out of our way to use rust-protobuf in its pure Rust mode to avoid the additional dependency.

All that said, in this platform world we're probably going to wind up using tonic and prost anyway. But, as long as there's no particular urgency, I'd like to take a little bit to look through the tradeoffs before we bring these deps into our tree.

I am super excited about this! This is exactly the kind of observability we'll need when we go distributed. I think the solution might be involve us doing some work upstream in Prost to make the binary blobs optional.

benesch added a commit to benesch/materialize that referenced this pull request Dec 22, 2021
Extracted from MaterializeInc#9665.

The idea here is to perform the filtering specified by the
`--log-filter` argument at the last possible moment, right before the
message is emitted to disk. This ensures that other log messages (e.g.,
trace and debug logs) are available to other parts of the logging
subsystem that may want to do something smart with them.

This already pays dividends in that the mz_log_message_total metric can
keep track of the log messages that don't get written out to the file.
It also paves the way for integrations with e.g. the Tokio console (see

Co-authored-by: Gus Wynn <[email protected]>
@benesch
Copy link
Member

benesch commented Dec 22, 2021

Recording a bit of conversation from Slack for posterity: I've filed tokio-rs/prost#575 to talk about the supply chain issue upstream. I'd like to wait at least a few days/weeks to see what upstream is interested in before we commit the dep to our tree and totally forget about it. But with #9725 hopefully this PR gets good and simple, and the merge conflicts won't be too brutal!

@guswynn guswynn force-pushed the console branch 2 times, most recently from c32063b to 6a17959 Compare December 22, 2021 20:39
@benesch
Copy link
Member

benesch commented Jan 18, 2022

I haven't forgotten about this! Once #10079 lands we'll be patching in our own version of prost and this should be safe to land.

@guswynn guswynn force-pushed the console branch 2 times, most recently from fd0bd82 to b7b4c05 Compare January 19, 2022 17:06
@guswynn
Copy link
Contributor Author

guswynn commented Jan 19, 2022

Cargo.lock is now correct, and doesn't use any of the crates.io versions, but tonic-build fails to build, so there might be a bug in our fork cc @benesch ?

@guswynn
Copy link
Contributor Author

guswynn commented Jan 19, 2022

error[E0599]: no method named `file_descriptor_set_path` found for struct `Config` in the current scope
   --> /Users/gus/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tonic-build-0.6.2/src/prost.rs:405:20
    |
405 |             config.file_descriptor_set_path(path);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^ method not found in `Config`

error[E0599]: no method named `include_file` found for struct `Config` in the current scope
   --> /Users/gus/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tonic-build-0.6.2/src/prost.rs:420:20
    |
420 |             config.include_file(path);
    |                    ^^^^^^^^^^^^ method not found in `Config`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `tonic-build` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: build failed

@guswynn
Copy link
Contributor Author

guswynn commented Jan 20, 2022

Ok, now its ready to review, please take a look at the Cargo.lock very closely

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Hallelujah! Thanks very much for bearing with me on this one. This looks great!

@guswynn guswynn merged commit 27b6c48 into MaterializeInc:main Jan 20, 2022
@guswynn guswynn deleted the console branch January 20, 2022 22:51
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