Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add XCM Tracing #3353

Merged
8 commits merged into from
Jul 5, 2021
Merged

Add XCM Tracing #3353

8 commits merged into from
Jul 5, 2021

Conversation

apopiak
Copy link
Contributor

@apopiak apopiak commented Jun 23, 2021 •
edited
Loading

This PR adds log::trace logging for a selection of XCM methods.
This includes:

  • log::debug (should this also be trace?) the generic execute_xcm implementation
  • trace the relevant methods in the xcm-executor
  • trace the tuple implementations of various traits in case of failure/filter/miss

The logging choices are tentative and open for feedback. Let me know what should be added/removed.

Usage

Add -lxcm=trace to any Polkadot node.

@apopiak apopiak added A0-please_review Pull request needs code review. B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jun 23, 2021
apopiak
apopiak commented Jun 23, 2021
View reviewed changes
xcm/src/v0/traits.rs Outdated
Comment on lines 137 to 141
log::debug!(
target: "xcm::execute_xcm",
"origin: {:?}, message: {:?}, weight_limit: {:?}",
&origin, &message, weight_limit,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

log I feel unsure about
Feels useful to be able to just add -lxcm=debug and get some measure of logging, but also adds this to (most) every implementation automatically. Also doesn't capture direct calls to execute_xcm_in_credit (which e.g. teleport_assets does).

@shawntabrizi
Copy link
Member

shawntabrizi commented Jun 23, 2021 •
edited
Loading

Can we confirm this is zero cost to the performance? (someone from the core dev team can probably comment)

Otherwise, a great idea imo

@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021 •
edited
Loading

Another general point: Compared to the amount of logging I just added to my Polkadot branch this is quite coarse-grained.

@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021

Can we confirm this is zero cost to the performance? (someone from the core dev team can probably comment)

Otherwise, a great idea imo

based on my understanding of
https://github.com/paritytech/substrate/pull/8655/files#diff-58e928d2a8ca73ab1d4a7ef80178314f6144506cba80ff0bd51b8d9928796bf2R44
it should be fine

@apopiak
Copy link
Contributor Author

apopiak commented Jun 23, 2021

Basti confirmed that logging is stripped for release builds of wasm runtimes.

@pepyakin
Copy link
Contributor

pepyakin commented Jun 23, 2021 •
edited
Loading

Just a quick and random chime in: if you are interested, there is an easy way to double check. Get the release wasm binary and run strings wasm.wasm | grep <x> where x is one of the strings you do not expect to be compiled in.

bkchr
bkchr approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

One last thing, using the function name as target sounds like a good idea. However, this is done nowhere else, so yeah, not sure we should start using it here?

bridges/modules/dispatch/src/lib.rs Outdated Show resolved Hide resolved
xcm/src/v0/traits.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/should_execute.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/transact_asset.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/transact_asset.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/transact_asset.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/transact_asset.rs Outdated Show resolved Hide resolved
apopiak and others added 2 commits July 5, 2021 16:23
@apopiak @bkchr
remove bridge log target
3b8a410 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@apopiak @bkchr
log argument formatting
0fd385c 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Jul 5, 2021

Trying merge.

@ghost ghost merged commit a78434a into master Jul 5, 2021
@ghost ghost deleted the apopiak/xcm-logging branch July 5, 2021 22:44
ordian added a commit that referenced this pull request Jul 6, 2021
@ordian
Merge branch 'master' into rh-chain-selection-db-and-stagnant
8feb436 
* master: (33 commits)
  Update all weights, add run_all_benches.sh script (#3400)
  Enable over-bridge-messaging in Rococo/Wococo runtime (#3377)
  paras.rs to FRAME V2 (#3403)
  Add XCM Tracing (#3353)
  Use MaxEncodedLen trait from new parity-scale-codec v2.2 (#3412)
  bump a bunch of deps in parity-common (#3402)
  Warn on low connectivity. (#3408)
  origin to frame v2 (#3405)
  Enable logging in the puppet worker (#3411)
  make it easier to dbg stalls (#3351)
  XCM `canonicalize` + `prepend_with` fix (#3269)
  cleanup stream polls (#3397)
  Staking Miner (#3141)
  Companion for Substrate#8953 (#3140)
  Bump version, specs & substrate in prep for v0.9.8 (#3387)
  Fix busy loops. (#3392)
  Minor refactor (#3386)
  add simnet tests (#3381)
  BEEFY: adjust gossip (#3372)
  Companion for #9193 (#3376)
  ...
@github-actions github-actions bot mentioned this pull request Aug 24, 2021
Closed
@apopiak apopiak mentioned this pull request Nov 3, 2021
Merged
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@gavofyork gavofyork Awaiting requested review from gavofyork

@pepyakin pepyakin Awaiting requested review from pepyakin

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apopiak @shawntabrizi @pepyakin @bkchr