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

Replace env_logger with sp_tracing #5065

Merged
merged 29 commits into from
Aug 1, 2024

Conversation

Safrout1
Copy link
Contributor

@Safrout1 Safrout1 commented Jul 18, 2024

This PR replaces env_logger with sp_tracing because of an issue with env_logger and gum #4660

polkadot address: 1upUS3E5th6CphPJg2kjKHY2gDXex6iB6J6V64qmCV7Jw7s

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 18, 2024

User @Safrout1, please sign the CLA here.

Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

That's ok

@Safrout1 Safrout1 marked this pull request as ready for review July 18, 2024 20:06
@Safrout1 Safrout1 requested review from acatangiu and a team as code owners July 18, 2024 20:06
@Safrout1 Safrout1 requested a review from AndreiEres July 18, 2024 20:07
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 18, 2024 20:07
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.

If env_logger is not used anymore anywhere, you should also remove it from the workspace Cargo.toml file.

@Safrout1
Copy link
Contributor Author

@bkchr No, it is used in 4 non-test files:
1 | bridges/relays/utils/src/initialize.rs
2 | polkadot/node/subsystem-bench/src/cli/subsystem-bench.rs
3 | substrate/client/executor/benches/bench.rs
4 | substrate/utils/frame/omni-bencher/src/main.rs

@Safrout1 Safrout1 requested a review from bkchr July 22, 2024 10:29
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 22, 2024 10:30
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Can you extend this outside of testing too? Replace env_logger completely?

substrate/frame/sassafras/src/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

A few comments

@AndreiEres
Copy link
Contributor

1 | bridges/relays/utils/src/initialize.rs

It's inside initialize_logger which used only in initialize_relay. I don't see any usages of initialize_relay in the code base. @acatangiu wdyt is it ok to replace it with try_init_simple?

2 | polkadot/node/subsystem-bench/src/cli/subsystem-bench.rs

Here is ok to replace with try_init_simple

3 | substrate/client/executor/benches/bench.rs
4 | substrate/utils/frame/omni-bencher/src/main.rs

We can try to use try_init_simple here as well. Let's change and ask guys that put it there if it's ok.

@acatangiu
Copy link
Contributor

cc @serban300 ^

@AndreiEres
Copy link
Contributor

I have updated the PR with all test files except for one: substrate/frame/contracts/src/tests.rs
This one's tests fail when using sp-tracing, I think it has to do with setting the right env, I am still figuring it out. Let me know if the test is the wrong one in that case.

Try try_init_simple.

@serban300
Copy link
Contributor

1 | bridges/relays/utils/src/initialize.rs

It's inside initialize_logger which used only in initialize_relay. I don't see any usages of initialize_relay in the code base. @acatangiu wdyt is it ok to replace it with try_init_simple?

It is used in parity-bridges-common: https://github.com/paritytech/parity-bridges-common/blob/63ac22d90462d14a1f1e9c36427473fa2fa57cb1/substrate-relay/src/cli/mod.rs#L93

I'm not sure what try_init_simple() does, but I think it would be good to keep the current characteristics, for example:

	builder.filter_level(log::LevelFilter::Warn);
	builder.filter_module("bridge", log::LevelFilter::Info);

The time format is probably not super important.

Co-authored-by: Adrian Catangiu <adrian@parity.io>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team July 25, 2024 14:15
@acatangiu
Copy link
Contributor

The CI pipeline was cancelled due to failure one of the required jobs. Job name: cargo-clippy Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6817241

@Safrout1 please fix these remaining issues

@Safrout1 Safrout1 requested a review from athei as a code owner July 28, 2024 11:42
@acatangiu acatangiu added T10-tests This PR/Issue is related to tests. T15-bridges This PR/Issue is related to bridges. labels Jul 30, 2024
@Safrout1 Safrout1 requested a review from serban300 July 30, 2024 13:11
@Safrout1
Copy link
Contributor Author

Hello,

I think the only required check that fails is continuous-integration/gitlab-check-toml-format

but whenever I run taplo format substrate/primitives/tracing/Cargo.toml on my machine, nothing new happens to the file.

Should I run a different command, the log in the job is also not helpful, WDYT?

@acatangiu @AndreiEres

@serban300
Copy link
Contributor

Hello,

I think the only required check that fails is continuous-integration/gitlab-check-toml-format

but whenever I run taplo format substrate/primitives/tracing/Cargo.toml on my machine, nothing new happens to the file.

Should I run a different command, the log in the job is also not helpful, WDYT?

@acatangiu @AndreiEres

Can you also try taplo format --config .config/taplo.toml ?

@AndreiEres
Copy link
Contributor

Look at that please as well https://github.com/paritytech/polkadot-sdk/actions/runs/10166554064/job/28141652578?pr=5065
There are suggestions for the pr_doc

Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

Good job @Safrout1, thank you

@acatangiu acatangiu added this pull request to the merge queue Aug 1, 2024
@AndreiEres AndreiEres linked an issue Aug 1, 2024 that may be closed by this pull request
Merged via the queue into paritytech:master with commit 776e957 Aug 1, 2024
161 of 164 checks passed
@AndreiEres
Copy link
Contributor

@Safrout1 could you provide your DOT address in the PR description?

ordian added a commit that referenced this pull request Aug 6, 2024
* master: (51 commits)
  Remove unused feature gated code from the minimal template (#5237)
  make polkadot-parachain startup errors pretty (#5214)
  Coretime auto-renew (#4424)
  network/strategy: Backoff and ban overloaded peers to avoid submitting the same request multiple times (#5029)
  Fix frame crate usage doc (#5222)
  beefy: Tolerate pruned state on runtime API call (#5197)
  rpc: Enable ChainSpec for polkadot-parachain (#5205)
  Add an adapter for configuring AssetExchanger (#5130)
  Replace env_logger with sp_tracing (#5065)
  Adjust sync templates flow to use new release branch (#5182)
  litep2p/discovery: Publish authority records with external addresses only (#5176)
  Run UI tests in CI for some other crates (#5167)
  Remove `pallet::getter` usage from the pallet-balances (#4967)
  pallet-timestamp: `UnixTime::now` implementation logs error only if called at genesis (#5055)
  [CI] Cache try-runtime check (#5179)
  [Backport] version bumps and the prdocs reordering from stable2407 (#5178)
  [subsystem-benchmark] Update availability-distribution-regression-bench baseline after recent subsystem changes (#5180)
  Remove pallet::getter usage from proxy (#4963)
  Remove pallet::getter macro usage from pallet-election-provider-multi-phase (#4487)
  Review-bot@2.6.0 (#5177)
  ...
@Safrout1
Copy link
Contributor Author

Safrout1 commented Aug 6, 2024

@Safrout1 could you provide your DOT address in the PR description?

@AndreiEres done, but would you like to explain why?

@AndreiEres
Copy link
Contributor

AndreiEres commented Aug 6, 2024

@AndreiEres done, but would you like to explain why?

I'll try to tip you for your job

@sandreim
Copy link
Contributor

sandreim commented Aug 6, 2024

/tip small

Copy link

@sandreim A referendum for a small (20 DOT) tip was successfully submitted for @Safrout1 (1upUS3E5th6CphPJg2kjKHY2gDXex6iB6J6V64qmCV7Jw7s on polkadot).

Referendum number: 1055.
tip

Copy link

The referendum has appeared on Polkassembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”. T10-tests This PR/Issue is related to tests. T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace env_logger with sp_tracing
7 participants