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

Fix blips in the transaction pool size metric #9174

Merged
merged 1 commit into from
Jun 12, 2023
Merged

Fix blips in the transaction pool size metric #9174

merged 1 commit into from
Jun 12, 2023

Conversation

aborg-dev
Copy link
Contributor

Right now the metric has noticeable blips due to rapid change when transactions are drawn from the pool: https://nearinc.grafana.net/goto/YZwyDllVR?orgId=1 To avoid this, we only decrease the metric after the pool iterator is dropped.

This is a part of #3284

@aborg-dev aborg-dev added the A-congestion Work aimed at ensuring good system performance under congestion label Jun 12, 2023
@aborg-dev aborg-dev requested a review from a team as a code owner June 12, 2023 10:18
@aborg-dev aborg-dev requested a review from jakmeier June 12, 2023 10:18
jakmeier
jakmeier approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Interesting, I notice these blips just this morning while briefly looking at results from my weekend run. And here you already have the fix :D

@aborg-dev
Fix blips in the transaction pool size metric
3c45c83 
Right now the metric has a noticeable blips due to rapid change when
transactions are drawn from the pool: https://nearinc.grafana.net/goto/YZwyDllVR?orgId=1
To avoid this, we only decrease the metric after the pool iterator is
dropped.
@aborg-dev aborg-dev force-pushed the fix_transaction_pool_size_blips branch from 5a02c6f to 3c45c83 Compare June 12, 2023 13:41
@near-bulldozer near-bulldozer bot merged commit 733c1d9 into near:master Jun 12, 2023
@jakmeier
Copy link
Contributor

@Akashin a question related to this: Shouldn't the metric be per shard? Like, when we call metrics::TRANSACTION_POOL_SIZE.set(self.pool.transaction_size() as i64) this sets a shared value between all shards. I think that's why I observe values jumping between 0 and several GBs in my test where a single shard causes congestion. Does that make sense?

nikurt pushed a commit that referenced this pull request Jun 13, 2023
@aborg-dev @nikurt
Fix blips in the transaction pool size metric (#9174)
4b95f2c 
Right now the metric has noticeable blips due to rapid change when transactions are drawn from the pool: https://nearinc.grafana.net/goto/YZwyDllVR?orgId=1 To avoid this, we only decrease the metric after the pool iterator is dropped.

This is a part of #3284
@aborg-dev
Copy link
Contributor Author

@Akashin a question related to this: Shouldn't the metric be per shard? Like, when we call metrics::TRANSACTION_POOL_SIZE.set(self.pool.transaction_size() as i64) this sets a shared value between all shards. I think that's why I observe values jumping between 0 and several GBs in my test where a single shard causes congestion. Does that make sense?

Oh, that's a good catch! Yes, it should be per-shard. I'll send a PR for this now.

@aborg-dev aborg-dev mentioned this pull request Jun 13, 2023
Merged
aborg-dev added a commit that referenced this pull request Jun 13, 2023
@aborg-dev
Make transaction pool metrics per-shard (#9185)
8fd8feb 
As there is actually 1 transaction pool per shard and their metrics mix together.

See #9174 (comment) for some details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakmeier jakmeier jakmeier approved these changes

Assignees
No one assigned
Labels
A-congestion Work aimed at ensuring good system performance under congestion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aborg-dev @jakmeier