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

Remove redundant sync primitives for metrics#14564

Merged
paritytech-processbot[bot] merged 6 commits intomasterfrom
vstakhov-minor-network-changes
Jul 14, 2023
Merged

Remove redundant sync primitives for metrics#14564
paritytech-processbot[bot] merged 6 commits intomasterfrom
vstakhov-minor-network-changes

Conversation

@vstakhov
Copy link
Contributor

Since metrics struct can be safely send between threads (it has it's own locking logic around prometheus primitives) there is no need to do extra locking nor reference counting I assume.

@vstakhov vstakhov added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 12, 2023
Comment on lines 187 to 195
} else if sender.warning_fired &&
current_pending < sender.queue_size_warning.wrapping_div(2)
{
sender.warning_fired = false;
info!(
"Channel `{}` is no longer overflowed. Number of events: {}",
sender.name, current_pending
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this. Otherwise it will get really spammy. This is just about saying "hey, there is somehing" and not on/off/on/off/on/off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The information like on/off would be very useful when we want to debug networking issue. However, I agree that it could be quite spammy. What about emitting error! on the first trigger, and then indicate all subsequent free/busy events on info or even debug level?

Copy link
Member

Choose a reason for hiding this comment

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

debug sounds reasonable

@bkchr bkchr requested a review from a team July 13, 2023 12:44
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Anton <anton.kalyaev@gmail.com>
@vstakhov
Copy link
Contributor Author

By the way, does anybody have an idea why benches job fails on this PR?

@bkchr
Copy link
Member

bkchr commented Jul 13, 2023

You didn't used the latest master to create this pr.

@bkchr
Copy link
Member

bkchr commented Jul 13, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@vstakhov vstakhov requested a review from bkchr July 14, 2023 09:15
Co-authored-by: Bastian Köcher <git@kchr.de>
@vstakhov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b06e109 into master Jul 14, 2023
@paritytech-processbot paritytech-processbot bot deleted the vstakhov-minor-network-changes branch July 14, 2023 10:47
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Remove redundant locks

* Re-enable warning for a sender when a queue got processed

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Anton <anton.kalyaev@gmail.com>

* Use debug for subsequent logging

* Update client/network/src/service/out_events.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Anton <anton.kalyaev@gmail.com>
Co-authored-by: parity-processbot <>
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
* Remove redundant locks

* Re-enable warning for a sender when a queue got processed

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Anton <anton.kalyaev@gmail.com>

* Use debug for subsequent logging

* Update client/network/src/service/out_events.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Anton <anton.kalyaev@gmail.com>
Co-authored-by: parity-processbot <>
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants