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

Add logger configuration hook #10440

Merged
merged 12 commits into from
Dec 16, 2021
Merged

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Dec 7, 2021

I'm aiming to leverage the WasmTracing infrastructure for emitting Prometheus metrics from wasm runtime - paritytech/polkadot#4383. I extended ProfilingLayer to work with multiple TraceHandlers. Since the entire logger and Profiling layer initialization is hidden deep in Substrate, I'm proposing to use a closure to inject changes in CliConfiguration::init() implementation.

In Polkadot we'll have a TraceHandler implementation that parses TraceEvents coming from wasm ("wasm_tracing") and publishes Prometheus metrics.

Polkadot companion: paritytech/polkadot#4483
Cumulus companion: paritytech/cumulus#856

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim added A0-please_review Pull request needs code review. B5-clientnoteworthy 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 Dec 7, 2021
@sandreim sandreim requested a review from bkchr December 7, 2021 16:43
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved
client/tracing/src/logging/mod.rs Outdated Show resolved Hide resolved
client/tracing/src/logging/mod.rs Outdated Show resolved Hide resolved
client/tracing/src/logging/mod.rs Outdated Show resolved Hide resolved
client/service/src/config.rs Outdated Show resolved Hide resolved
client/tracing/src/lib.rs Outdated Show resolved Hide resolved
client/tracing/src/logging/mod.rs Outdated Show resolved Hide resolved
client/cli/src/config.rs Show resolved Hide resolved
client/cli/src/lib.rs Show resolved Hide resolved
…m/custom_profiling

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim requested a review from bkchr December 9, 2021 15:50
/// Add a custom profiler.
pub fn with_custom_profiling(
&mut self,
custom_profiler: Box<dyn crate::TraceHandler>,
Copy link
Member

Choose a reason for hiding this comment

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

I mean you could have left the vector :P It was just about not taking a vec directly. But fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it doesn't make sense to have many of these, one is enough, since you can still multiplex further. However, we can change it to vec later if there is demand 😁

/// Create a runner for the command provided in argument. The `logger_hook` can be used to setup
/// a custom profiler or update the logger configuration before it is initialized.
///
/// Example:
Copy link
Member

Choose a reason for hiding this comment

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

👍

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@bkchr
Copy link
Member

bkchr commented Dec 13, 2021

bot merge force

@paritytech-processbot
Copy link

Error: It's not possible to meet the minimal approval count of 1 in paritytech/cumulus#856

@ordian
Copy link
Member

ordian commented Dec 14, 2021

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/polkadot#4483 is not mergeable

@bkchr
Copy link
Member

bkchr commented Dec 16, 2021

bot merge force

@paritytech-processbot
Copy link

Bot will approve on the behalf of @bkchr, since they are a team lead, in an attempt to reach the minimum approval count

@paritytech-processbot paritytech-processbot bot merged commit fb08d15 into master Dec 16, 2021
@paritytech-processbot paritytech-processbot bot deleted the sandreim/custom_profiling branch December 16, 2021 11:18
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
* Initial poc

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Make config available to logger hook

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Fix tests

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Add metric prefix option in sc_cli::RunCmd

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Remove print

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Review fixes

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fix docs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Initial poc

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Make config available to logger hook

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Fix tests

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Add metric prefix option in sc_cli::RunCmd

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Remove print

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Review fixes

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fix docs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Initial poc

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Make config available to logger hook

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Fix tests

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Add metric prefix option in sc_cli::RunCmd

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Remove print

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* Review fixes

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fmt

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

* fix docs

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

3 participants