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

[Mempool] reduce mempool log noise #9767

Merged
merged 2 commits into from
Aug 25, 2023
Merged

[Mempool] reduce mempool log noise #9767

merged 2 commits into from
Aug 25, 2023

Conversation

bchocho
Copy link
Contributor

@bchocho bchocho commented Aug 24, 2023 •
edited
Loading

Description

Reduce the noise added by #9309. Make expected "errors" trace, and sample regardless.

@bchocho bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Aug 24, 2023
@bchocho bchocho marked this pull request as ready for review August 24, 2023 22:48
@bchocho bchocho requested a review from JoshLind August 24, 2023 22:48
@bchocho bchocho requested a review from sitalkedia August 24, 2023 22:52
JoshLind
JoshLind approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@JoshLind JoshLind left a comment

Choose a reason for hiding this comment

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

LGTM!

mempool/src/shared_mempool/tasks.rs
@@ -69,8 +69,17 @@ pub(crate) async fn execute_broadcast<NetworkClient, TransactionValidator>(
)
.peer(&peer)
.error(&error)),
BroadcastError::NoTransactions(_) | BroadcastError::PeerNotPrioritized(_, _) => {
sample!(
SampleRate::Duration(Duration::from_secs(60)),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could consider making this a const, e.g.:

const BROADCAST_LOG_FREQ_SECS: u64 = 60;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to sample trace? Just sampling overhead for something that we don't use quite often.

@bchocho bchocho requested a review from ibalajiarun August 24, 2023 23:00
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.6.2 ==> aea3489cd0967de6d1638fc9eec66f1171675b68

Compatibility test results for aptos-node-v1.6.2 ==> aea3489cd0967de6d1638fc9eec66f1171675b68 (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4574 txn/s, latency: 6924 ms, (p50: 6300 ms, p90: 10200 ms, p99: 17200 ms), latency samples: 187540
2. Upgrading first Validator to new version: aea3489cd0967de6d1638fc9eec66f1171675b68
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1857 txn/s, latency: 15847 ms, (p50: 18100 ms, p90: 21700 ms, p99: 22300 ms), latency samples: 92860
3. Upgrading rest of first batch to new version: aea3489cd0967de6d1638fc9eec66f1171675b68
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1880 txn/s, latency: 15525 ms, (p50: 19100 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 92120
4. upgrading second batch to new version: aea3489cd0967de6d1638fc9eec66f1171675b68
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3424 txn/s, latency: 9187 ms, (p50: 10000 ms, p90: 12900 ms, p99: 13300 ms), latency samples: 136960
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> aea3489cd0967de6d1638fc9eec66f1171675b68 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on aea3489cd0967de6d1638fc9eec66f1171675b68

two traffics test: inner traffic : committed: 7350 txn/s, latency: 5341 ms, (p50: 5100 ms, p90: 6600 ms, p99: 9900 ms), latency samples: 3175340
two traffics test : committed: 100 txn/s, latency: 3455 ms, (p50: 3400 ms, p90: 4200 ms, p99: 6600 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.220, avg: 0.211", "QsPosToProposal: max: 0.141, avg: 0.132", "ConsensusProposalToOrdered: max: 0.534, avg: 0.514", "ConsensusOrderedToCommit: max: 0.501, avg: 0.489", "ConsensusProposalToCommit: max: 1.016, avg: 1.002"]
Max round gap was 1 [limit 4] at version 1553080. Max no progress secs was 3.836849 [limit 10] at version 1553080.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> aea3489cd0967de6d1638fc9eec66f1171675b68

Compatibility test results for aptos-node-v1.5.1 ==> aea3489cd0967de6d1638fc9eec66f1171675b68 (PR)
Upgrade the nodes to version: aea3489cd0967de6d1638fc9eec66f1171675b68
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4478 txn/s, latency: 7361 ms, (p50: 7500 ms, p90: 10200 ms, p99: 15100 ms), latency samples: 165700
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> aea3489cd0967de6d1638fc9eec66f1171675b68 passed
Test Ok

@bchocho bchocho merged commit 091469b into main Aug 25, 2023
@bchocho bchocho deleted the brian/noisy-mempool-log branch August 25, 2023 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zekun000 zekun000 zekun000 approved these changes

@JoshLind JoshLind JoshLind approved these changes

@ibalajiarun ibalajiarun ibalajiarun approved these changes

@sitalkedia sitalkedia Awaiting requested review from sitalkedia

Assignees
No one assigned
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bchocho @ibalajiarun @JoshLind @zekun000