-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
Enable Jaeger tracing, Tokio Console, file logging for wallet #859
Conversation
…distributed tracing
Hey @velvia -- is this still WIP or are you looking for reviews? |
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.
The density of annotations is getting heavy. Can we annotate using function attributes, which does not obscure code? Also: what are then bench before and the bench after applying this patch?
You can work this out by doing:
cargo run --release --bin=bench -- --use-move --num-accounts 160000 --max-in-flight 4000
for suitable parameters.
@@ -343,9 +347,12 @@ impl AuthorityState { | |||
} | |||
|
|||
// Check the certificate and retrieve the transfer data. | |||
certificate.check(&self.committee)?; | |||
tracing::trace_span!("cert_check_signature") |
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.
General comment: can we not use
#[tracing::instrument]
on the functions instead of annotating so many lines of code?
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.
I'll look into it. The auto instrumenting requires even more lines, it's just that they are outside the function instead of in code. It defaults to instrumenting every param, which we usually don't want, and it's not clear you can choose the level, which is really important, or the tags, which are also important. I'll see when we can substitute.
Nice one @velvia |
…s#859 This enacts the reversion of MystenLabs#818
* Update narwhal pointer * fix: adapt the Narwhal pointer in Sui post merge of narwhal/#859 (#4354) This enacts the reversion of #818 * Add comments from #4219 * Update authority.rs after rebase * hakari generate Co-authored-by: François Garillot <[email protected]>
Adds a couple backends for Tokio tracing, including Jaeger output support, a subscriber for Tokio console, as well as rotating file logging for the wallet CLI. For now, configurable using env vars.
NOTE: I'm going to move the subscriber setup out of sui.rs and wallet.rs and to Mysten-infra