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

remove gum dependency on jaeger #2106

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

jpserrat
Copy link
Contributor

@jpserrat jpserrat commented Oct 31, 2023

Description

As suggested, tracing-gum only relies on one function of jaeger and the dependency can be removed by copying this function.
Let me know if this is what you guys had in mind.

Closes #649

@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Oct 31, 2023
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 13:19
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 13:49
@mrcnski
Copy link
Contributor

mrcnski commented Nov 1, 2023

I had previously used cargo udeps to check for unused deps, but it seems like it doesn't really work, because I went and manually checked each dep and many were actually not used. So I just removed them.

It doesn't look like this PR actually cut down on the number of syscalls in the binaries, which is good. It means that LTO was working well and not including unused code. tokio had already been removed previously, which was a big win.

Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

A good clean-up! 👍

Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

I experimented with removing all the log lines and the gum dependency, verifying that it didn't appear in cargo tree output anymore. However, this didn't decrease the detected syscalls at all. So we can keep gum, and as far as I know there is no other dependency that can be removed like we did with tokio - therefore I'm closing the linked issue.

Thank you very much @jpserrat for the cleanup and your continued contributions. ⭐️

@mrcnski mrcnski merged commit 2726d5a into paritytech:master Nov 1, 2023
118 of 119 checks passed
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Use generated runtimes for BHR/BHW

* Deduplicate BHR/BHW runtimes

* Add script for regenerating runtimes
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”.
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

PVF worker: look into removing unneeded dependencies
4 participants