-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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
Take 2: Add Jaeger/tracing, Tokio-Console to Sui and wallet runtimes #1068
Conversation
…distributed tracing
@gdanezis @oxade @laura-makdah Sorry need you to approve the PR again, I had to re-create it because of issues with my old fork not working anymore. Changes since last time: a bunch of setup code was moved to Mysten-infra, and some inline instrumentation moved to Also, benchmark results vs main are included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for pushing this one!
Historic PR at #859 for ref.
deny.toml
Outdated
@@ -55,6 +55,10 @@ ignore = [ | |||
# no safe upgrade is available, but once one is, we should upgrade Move | |||
# and remove this | |||
"RUSTSEC-2022-0002", | |||
# telemetry component in mysten-infra clashes with crates.io telemetry package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Evan! Great work. I have a bunch of tiny suggestions and a couple of errors caught.
doc/src/contribute/observability.md
Outdated
nested and key-value pairs in spans give context to events or logs inside the function. | ||
|
||
* spans and their key-value pairs adds essential context to enclosed logs, such as a transaction ID. | ||
* spans also track time spent in different sections of code, enabling distributed tracing functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow "functionality" with a period to match the other bullet points in this list.
doc/src/contribute/observability.md
Outdated
To see nested spans visualized with [Jaeger](https://www.jaegertracing.io), do the following: | ||
|
||
1. Run this to get a local Jaeger container: `docker run -d -p6831:6831/udp -p6832:6832/udp -p16686:16686 jaegertracing/all-in-one:latest` | ||
2. Run sui like this (trace enables the most detailed spans): `SUI_TRACING_ENABLE=1 RUST_LOG="info,sui_core=trace" ./sui start` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use 1. for each numbered step and let the numbers be dynamically generated. Try it in preview.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True though in my experience it's also harmless, my editor just does it. However it does work as you say.
doc/src/contribute/observability.md
Outdated
### Live async inspection / Tokio Console | ||
|
||
[Tokio-console](https://github.com/tokio-rs/console) is an awesome CLI tool designed to analyze and help debug Rust apps using Tokio, in real time! It relies on a special subscriber. | ||
|
||
1. Build Sui using a special flag: `RUSTFLAGS="--cfg tokio_unstable" cargo build` | ||
2. Start Sui with `SUI_TOKIO_CONSOLE` set to 1 | ||
3. Clone the console repo and `cargo run` to alunch the console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to launch (spelled as to alunch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also end sentence with a period to match the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make sure you have peers test your commands. I will try to get to that in time but don't want to hold you up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Evan!
#[instrument]
Benchmark results
On main:
On this branch, using default logging settings in bench: