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

fatxpool: use tracing for logging #6897

Merged
merged 19 commits into from
Jan 30, 2025
Merged

fatxpool: use tracing for logging #6897

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

@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?

substrate/client/transaction-pool/src/fork_aware_txpool/dropped_watcher.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
@dharjeezy dharjeezy requested a review from bkchr January 4, 2025 14:03
@michalkucharczyk
Copy link
Contributor

@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
substrate/client/transaction-pool/src/fork_aware_txpool/import_notification_sink.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/import_notification_sink.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/import_notification_sink.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/import_notification_sink.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/import_notification_sink.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
@michalkucharczyk
Copy link
Contributor

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 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.

substrate/client/transaction-pool/src/common/tracing_log_xt.rs Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/common/tracing_log_xt.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/common/tracing_log_xt.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/common/tracing_log_xt.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs Outdated Show resolved Hide resolved
@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.

substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs Outdated Show resolved Hide resolved
@michalkucharczyk
Copy link
Contributor

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

@dharjeezy
Copy link
Contributor Author

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-silent Changes should not be mentioned in any release notes label Jan 29, 2025
@michalkucharczyk michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Jan 29, 2025
@michalkucharczyk
Copy link
Contributor

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
substrate/client/transaction-pool/Cargo.toml Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Outdated Show resolved Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/view.rs Outdated Show resolved Hide resolved
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 substrate-tip-bot
Copy link

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

Referendum number: 1408.
tip

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

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 <1728078+michalkucharczyk@users.noreply.github.com>
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-silent Changes should not be mentioned in any release notes 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