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

[Bug Fix] - Incorrect trace caused by use of Span::enter in asynchronous code #835

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Conversation

patrickkuo
Copy link
Contributor

jsonrpsee is emitting incorrect trace because of incorrect use of Span::enter in async code

Symtoms:

logs are very spammy as the trace is entering incorrect span

_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_getObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferObject}:method_call{method=sui_transferObject}:method_call{method=sui_transferObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_splitCoin}:method_call{method=sui_transferObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_syncAccountState}:method_call{method=sui_getObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferObject}:method_call{method=sui_splitCoin}:method_call{method=sui_transferObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferObject}:method_call{method=sui_executeTransaction}:method_call{method=sui_transferSui}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:method_call{method=sui_executeTransaction}:gateway_execute_transaction{tx_digest=VXrf5dF9bFPlPQsyxOXhvXRZ+5zZneS2t90nhI+6WCM= tx_kind="Single"}:execute_transaction{tx_digest=VXrf5dF9bFPlPQsyxOXhvXRZ+5zZneS2t90nhI+6WCM= tx_kind="Single"}:process_tx: sui_core::safe_client: Client error error=PairwiseSyncFailed { xsource: k#e97638a9e10b9cc46d85b81191a60a6a6fcae63dacd811c07502db9c5cfc55b5, destination: k#89cafd721eeb1b13483d5fb0968500e78103e8146b45edb30d93781d946d0aeb, tx_digest: VXrf5dF9bFPlPQsyxOXhvXRZ+5zZneS2t90nhI+6WCM=, error: RpcError("Timeout expired") } authority=k#e97638a9e10b9cc46d85b81191a60a6a6fcae63dacd811c07502db9c5cfc55b5

this PR replace the use of Span::enter with async {...}.instrument(span).await as suggested by the tracing doc
https://docs.rs/tracing/latest/tracing/struct.Span.html#in-asynchronous-code

@patrickkuo patrickkuo requested a review from a team as a code owner July 29, 2022 08:34
let params = Params::new(req.params.map(|params| params.get()));
let name = &req.method;
let id = req.id;
let params = Params::new(req.params.map(|params| params.get()));
Copy link
Member

@niklasad1 niklasad1 Jul 29, 2022

Choose a reason for hiding this comment

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

Can't params, name & id be extracted outside the async block?

@niklasad1
Copy link
Member

nice catch and work, looks good

@jsdw
Copy link
Collaborator

jsdw commented Jul 29, 2022

Thanks for the PR!

This does make it clear that it can be quite hard to get tracing right when we need a span to cover multiple await points properly; perhaps we should be using the attribute suggested in the docs on these functions instead (#[tracing::instrument(level = "info")]) so that we don't need to litter the code with async {} blocks and worry about this sort of thing.

I also noted that in a couple of cases in the PR we were making an async {} block around places which only had a single await. Not a big deal but not necessary.

So I'm happy to merge this, but let's consider simplifying the code a bit and using the macro, unless it has some downsides?

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.

4 participants