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

Meter tx manager poll duration #6688

Merged
merged 16 commits into from
Feb 22, 2024
Merged

Meter tx manager poll duration #6688

merged 16 commits into from
Feb 22, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 20, 2024 •
edited
Loading

Closes paradigmxyz/reth/ issues/ 6686. Closes #6737.

Adds Gauges for TransactionsManager future and its nested streams, to measure the accumulated time spent in the future on a whole and in each nested stream.

@emhane emhane added C-enhancement New feature or request C-perf A change motivated by improving speed, memory usage or disk footprint A-tx-pool Related to the transaction mempool A-observability Related to tracing, metrics, logs and other observability tools labels Feb 20, 2024
mattsse
mattsse approved these changes Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense

pending @Rjected

should this be a histogram instead?

crates/net/network/src/transactions/mod.rs Outdated Show resolved Hide resolved
onbjerg
onbjerg reviewed Feb 20, 2024
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

some doc nits. also, is acc accumulated time spent? if so, should be in the docs, too

crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
Rjected
Rjected requested changes Feb 20, 2024
View reviewed changes
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

I think this makes sense as a Gauge, not sure that we really need the histogram bucketing. I just have one comment about macro placement, otherwise this looks good to me

crates/net/network/src/transactions/mod.rs Outdated Show resolved Hide resolved
crates/net/network/src/transactions/mod.rs Outdated Show resolved Hide resolved
@emhane emhane requested a review from gakonst as a code owner February 21, 2024 20:35
emhane and others added 9 commits February 21, 2024 21:37
@emhane
fixup! fixup! Generalise macro rule for time metered execution
36c7c1b
@emhane
Revert name change and docs fix, out of PR scope
5d7a087
@emhane @onbjerg
Fix typo
31e90a3 
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
@emhane @onbjerg
Update docs
9e53bfb 
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
@emhane @onbjerg
Update docs
5d7ca38 
Co-authored-by: Oliver Nordbjerg <onbjerg@users.noreply.github.com>
@emhane
Fix merge conflicts
d6d685b
@emhane
Update docs
419df1f
@emhane
Drive-by, fix comments
78350a0
@emhane
Update Cargo.lock
0157a00
onbjerg
onbjerg reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

we need to be explicit here since there is no hint in prometheus on what the unit is, so we have to be super clear when communicating it to the user. the docs on each individual metric is served to the user directly

crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Outdated Show resolved Hide resolved
crates/net/network/src/metrics.rs Show resolved Hide resolved
@emhane emhane requested a review from Rjected February 22, 2024 21:05
@emhane emhane changed the base branch from main to emhane/update-enr February 22, 2024 22:08
@emhane emhane requested a review from Evalir as a code owner February 22, 2024 22:08
@emhane emhane changed the base branch from emhane/update-enr to main February 22, 2024 22:08
Rjected
Rjected approved these changes Feb 22, 2024
View reviewed changes
Copy link
Member

@Rjected Rjected 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 to me! one question about duration

crates/net/network/src/transactions/mod.rs Show resolved Hide resolved
@emhane emhane added this pull request to the merge queue Feb 22, 2024
Merged via the queue into main with commit 9f91c6a Feb 22, 2024
30 checks passed
@emhane emhane deleted the emhane/metrics-tx-manager branch February 22, 2024 23:34
This was referenced Feb 27, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onbjerg onbjerg onbjerg left review comments

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Rjected approved these changes

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin

@joshieDo joshieDo Awaiting requested review from joshieDo

@rakita rakita Awaiting requested review from rakita

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk

@Evalir Evalir Awaiting requested review from Evalir

Assignees
No one assigned
Labels
A-observability Related to tracing, metrics, logs and other observability tools A-tx-pool Related to the transaction mempool C-enhancement New feature or request C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance metrics for TransactionsManager future
4 participants
@emhane @Rjected @onbjerg @mattsse