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.

Sign up for GitHub

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

Jump to bottom

Improve remote externalities logging #7021

Merged
merged 11 commits into from
Jan 2, 2025
Merged

Improve remote externalities logging #7021

merged 11 commits into from
Jan 2, 2025

Conversation

aurexav
Copy link
Contributor

@aurexav aurexav commented Dec 31, 2024 •
edited
Loading

Changes:

  • Automatically detect if current env is tty. If not disable the spinner logging.
  • Add Result type.
  • Format log style.

Originally reported from:

Closes #7010.

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

@aurexav
Copy link
Contributor Author

aurexav commented Dec 31, 2024

@bkchr 🙏🏻

bkchr
bkchr reviewed Dec 31, 2024
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.

Ty!

Some nitpicks, otherwise it looks good

substrate/utils/frame/remote-externalities/src/logging.rs Show resolved Hide resolved
substrate/utils/frame/remote-externalities/src/lib.rs Outdated Show resolved Hide resolved
substrate/utils/frame/remote-externalities/src/logging.rs Show resolved Hide resolved
substrate/utils/frame/remote-externalities/src/logging.rs Outdated Show resolved Hide resolved
@aurexav
Copy link
Contributor Author

aurexav commented Dec 31, 2024 •
edited
Loading

Some nitpicks, otherwise it looks good

Done.

Happy New Year :)

Wait, it won't print the final msg if it is not a terminal.

Done.

@aurexav
Print start and end anyway
ddbb337 
Signed-off-by: Xavier Lau <[email protected]>
@aurexav aurexav force-pushed the optional-try-runtime-log branch from 4657b5f to ddbb337 Compare December 31, 2024 16:15
@aurexav aurexav requested a review from bkchr December 31, 2024 16:15
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Jan 1, 2025
@bkchr
Copy link
Member

bkchr commented Jan 1, 2025

/cmd prdoc --audience node_dev --bump patch

@bkchr bkchr requested a review from skunert January 1, 2025 21:37
@bkchr bkchr enabled auto-merge January 1, 2025 21:37
bkchr
bkchr approved these changes Jan 2, 2025
View reviewed changes
prdoc/pr_7021.prdoc Outdated Show resolved Hide resolved
ggwpez
ggwpez approved these changes Jan 2, 2025
View reviewed changes
substrate/utils/frame/remote-externalities/src/logging.rs
let timer = Instant::now();
let mut maybe_sp = start(start_msg);

Ok(end(f()?, timer, maybe_sp.as_mut(), end_msg))
Copy link
Member

Choose a reason for hiding this comment

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

Could probably get rid of the sync variant if you wanted to and always use the async one.

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2025

Happy new year and thanks for the fix!

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2025

/tip small

@ggwpez
Copy link
Member

ggwpez commented Jan 2, 2025

/bot fmt

@substrate-tip-bot substrate-tip-bot
Copy link

@ggwpez A referendum for a small (20 DOT) tip was successfully submitted for @AurevoirXavier (156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y on polkadot).

Referendum number: 1369.
tip

@substrate-tip-bot substrate-tip-bot
Copy link

The referendum has appeared on Polkassembly.

@bkchr
Copy link
Member

bkchr commented Jan 2, 2025

bot fmt

@command-bot
Copy link

command-bot bot commented Jan 2, 2025 •
edited
Loading

@bkchr https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7964469 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 26-a5ec9153-057a-44d5-9c69-f7bc14603b0b to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jan 2, 2025

@bkchr Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7964469 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7964469/artifacts/download.

@bkchr bkchr added this pull request to the merge queue Jan 2, 2025
Merged via the queue into paritytech:master with commit b7e2695 Jan 2, 2025
194 of 201 checks passed
@aurexav aurexav deleted the optional-try-runtime-log branch January 3, 2025 01:30
@aurexav aurexav mentioned this pull request Jan 3, 2025
Open
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
@aurexav @bkchr
@ggwpez @dudo50
Improve remote externalities logging (paritytech#7021)
1a3e9b3 
Changes:
- Automatically detect if current env is tty. If not disable the spinner
logging.
- Add `Result` type.
- Format log style.

Originally reported from:
-
https://github.com/hack-ink/polkadot-runtime-releaser/blob/4811d2b419649a73edd5bd1f748a858b846eb139/action/try-runtime/action.yml#L75-L91
-
hack-ink/polkadot-runtime-releaser-workshop#3 (comment)

Closes paritytech#7010.

---

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

---------

Signed-off-by: Xavier Lau <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
ordian added a commit that referenced this pull request Jan 7, 2025
@ordian
Merge branch 'master' into ao-fix-issue-64
b4fea58 
* master: (256 commits)
  fix chunk fetching network compatibility zombienet test (#6988)
  chore: delete repeat words (#7034)
  Print taplo version in CI (#7041)
  Implement cumulus StorageWeightReclaim as wrapping transaction extension + frame system ReclaimWeight (#6140)
  Make `TransactionExtension` tuple of tuple transparent for implication (#7028)
  Replace duplicated whitelist with whitelisted_storage_keys (#7024)
  [WIP] Fix networking-benchmarks (#7036)
  [docs] Fix release naming (#7032)
  migrate pallet-mixnet to umbrella crate (#6986)
  Improve remote externalities logging (#7021)
  Fix polkadot sdk doc. (#7022)
  Remove warning log from frame-omni-bencher CLI (#7020)
  [pallet-revive] fix file case (#6981)
  Add workflow for networking benchmarks (#7029)
  [CI] Skip SemVer on R0-silent and update docs (#6285)
  correct path in cumulus README (#7001)
  sync: Send already connected peers to new subscribers (#7011)
  Excluding chainlink domain for link checker CI (#6524)
  pallet-bounties: Fix benchmarks for 0 ED (#7013)
  Log peerset set ID -> protocol name mapping (#7005)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@bkchr bkchr bkchr approved these changes

@skunert skunert Awaiting requested review from skunert

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optional Try Runtime Logging
3 participants
@aurexav @bkchr @ggwpez