Skip to content

fatxpool: use tracing for logging #6897

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
Merged
michalkucharczyk merged 19 commits into from
Jan 30, 2025
Merged

fatxpool: use tracing for logging #6897

michalkucharczyk merged 19 commits into from
Jan 30, 2025

Conversation

@dharjeezy
Copy link
Contributor

@dharjeezy dharjeezy commented Dec 14, 2024 •
edited
Loading

This PR modifies the fatxpool to use tracing instead of log for logging.

closes #5490

Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Dec 14, 2024

@michalkucharczyk you said i should let you know when i start with a single file, #5490 (comment)

@dharjeezy dharjeezy marked this pull request as ready for review December 28, 2024 12:15
bkchr
bkchr reviewed Dec 28, 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.

Can you change the other logging calls similar to how I have done it in the suggestions?

@dharjeezy dharjeezy requested a review from bkchr January 4, 2025 14:03
@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 6, 2025

@michalkucharczyk you said i should let you know when i start with a single file, #5490 (comment)

Sorry for the delay, I had xmas break. Looking into this now.

michalkucharczyk
michalkucharczyk reviewed Jan 6, 2025
View reviewed changes
@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 6, 2025

In general the direction is good:

  • mainly left some nitpicks (x=?x could be written as ?x).
  • some indentation is broken
  • one log_xt was removed, it shall be brought back.

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 23, 2025 •
edited
Loading

@dharjeezy are you planning to continue working on this?

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Jan 23, 2025

@dharjeezy are you planning to continue working on this?

yes. i will get it done through the week/weekend.

@dharjeezy
simplification in logging
ba54f71
@dharjeezy
Merge remote-tracking branch 'origin/master' into dami/fatxpool-traci…
81af798 
…ng-log

# Conflicts:
#	substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs
#	substrate/client/transaction-pool/src/fork_aware_txpool/view.rs
#	substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs
@dharjeezy
merge conflict resolving
cbf1316
@dharjeezy
resolving conversations
5ecd8ae
michalkucharczyk
michalkucharczyk reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

In general you common::log_xt mod shall not be used in fork_aware mod.

Here is a commit I made with all required changes:
f617182

Feel free to cherry-pick this to your branch.

@michalkucharczyk michalkucharczyk changed the title tracing for logging in fatxpool fatxpool: use tracing for logging Jan 29, 2025
michalkucharczyk
michalkucharczyk reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

Almost there. Few more changes needed. Thank you for your work.

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 29, 2025

I have some other (quite heavy) PRs incoming. Would love to see this one merged first.

@dharjeezy
Copy link
Contributor Author

dharjeezy commented Jan 29, 2025

I have some other (quite heavy) PRs incoming. Would love to see this one merged first.

Ok. Working on your feedback now

@michalkucharczyk michalkucharczyk added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Jan 29, 2025
@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 29, 2025

Two more nits, and should be good.

michalkucharczyk
michalkucharczyk approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

iulianbarbu
iulianbarbu reviewed Jan 29, 2025
View reviewed changes
iulianbarbu
iulianbarbu approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

LGTM! IMO the suggestions I have above are nice to be applied, to be consistent throughout the logging.

@dharjeezy dharjeezy requested a review from iulianbarbu January 30, 2025 11:09
@michalkucharczyk michalkucharczyk added this pull request to the merge queue Jan 30, 2025
Merged via the queue into paritytech:master with commit 07d4b46 Jan 30, 2025
203 of 206 checks passed
@bkchr
Copy link
Member

bkchr commented Jan 31, 2025

/tip small

@substrate-tip-bot
Copy link

substrate-tip-bot bot commented Jan 31, 2025

@bkchr A referendum for a small (20 DOT) tip was successfully submitted for @dharjeezy (12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW on polkadot).

Referendum number: 1408.
tip

@substrate-tip-bot
Copy link

substrate-tip-bot bot commented Jan 31, 2025

The referendum has appeared on Polkassembly.

ordian added a commit that referenced this pull request Feb 3, 2025
@ordian
Merge branch 'master' into ao-migrate-zombie-past-slashing-test
83a2992 
* master:
  Remove warnings by cleaning up the `Cargo.toml` (#7416)
  [Backport] Version bumps from stable2412-1 + prdocs reorg (#7401)
  fix pre-dispatch PoV underweight for ParasInherent (#7378)
  malus-collator: implement malicious collator submitting same collation to all backing groups (#6924)
  `fatxpool`: use tracing for logging (#6897)
  Improvements for Weekly bench (#7390)
  Replace derivative dependency with derive-where (#7324)
  Add support for feature `pallet_balances/insecure_zero_ed` in benchmarks and testing (#7379)
  Fix Snowbridge benchmark tests (#7296)
  Bridges small nits/improvements (#7383)
  Migrating cumulus-pallet-session-benchmarking to Benchmarking V2  (#6564)
  [pallet-revive] implement the block author API  (#7198)
  Use checked math in frame-balances named_reserve (#7365)
  move installation of frame-omni-bencher into a cmd.py itself (#7372)
  remove old bench & revert the frame-weight-template (#7362)
  ci: fix workflow permissions (#7366)
  [net/libp2p] Use raw `Identify` observed addresses to discover external addresses (#7338)
  Improve `set_validation_data` error message. (#7359)
  Implement pallet view function queries (#4722)
Ank4n pushed a commit that referenced this pull request Feb 6, 2025
@dharjeezy @michalkucharczyk @Ank4n
fatxpool: use tracing for logging (#6897)
3ecb653 
This PR modifies the fatxpool to use tracing instead of log for logging.

closes #5490

Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW

---------

Co-authored-by: Michal Kucharczyk <[email protected]>
@dharjeezy dharjeezy mentioned this pull request Mar 23, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 22, 2025
@dharjeezy @iulianbarbu
@bkchr @michalkucharczyk
txpool: use tracing for structured logging (#8001)
38a4dfe 
Follow up on #6897 cc:
@michalkucharczyk

Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW

Fixes: #5490

---------

Co-authored-by: Iulian Barbu <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Michal Kucharczyk <[email protected]>
castillax pushed a commit that referenced this pull request May 12, 2025
@dharjeezy @iulianbarbu
@bkchr @michalkucharczyk
txpool: use tracing for structured logging (#8001)
1a98179 
Follow up on #6897 cc:
@michalkucharczyk

Polkadot address: 12GyGD3QhT4i2JJpNzvMf96sxxBLWymz4RdGCxRH5Rj5agKW

Fixes: #5490

---------

Co-authored-by: Iulian Barbu <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Michal Kucharczyk <[email protected]>
@github-actions github-actions bot mentioned this pull request Jun 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@iulianbarbu iulianbarbu iulianbarbu approved these changes

@bkchr bkchr Awaiting requested review from bkchr

@skunert skunert Awaiting requested review from skunert

Assignees

No one assigned

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. 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.

fatxpool: use tracing instead of log

4 participants

@dharjeezy @michalkucharczyk @bkchr @iulianbarbu