Add a scoped utility to monitor cumulative duration and iterations #6501
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwtian
changed the title
andll
approved these changes
Dec 1, 2022
crates/mysten-metrics/src/lib.rs
Outdated
|
|
||
| #[macro_export] | ||
| macro_rules! monitored_scope { | ||
| ($name: expr) => {{ |
There was a problem hiding this comment.
If we have a 'name', perhaps we don't even need location at this point?
Locations change as code evolve, so they are not going to be used in dashboards or anything.
Even for monitored_futures, etc, I would prefer to just go with explicit name instead of deriving location from file/line
And cherry on top is that if we don't need location then this entire thing can turn into regular function
There was a problem hiding this comment.
Good point. I removed the location label for monitored_scope, and converted the macro into a function.
mwtian
force-pushed
the
cheaper-handle-consensus
branch
from
502b3f1 to
86c9a26
Compare
Jibz1
pushed a commit
that referenced
this pull request
Dec 7, 2022
…6501) This is a generalization of TaskUtilizationExt that we use in a few places. There are a few select loops and sections guarded by mutex that may become the bottleneck of the system. It is useful to monitor how "loaded" they are, and if increased load comes from increased number of calls. In future we can also record the number of times where a call is particularly long (> 1s or a user specified threshold).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a generalization of
TaskUtilizationExtthat we use in a few places. There are a few select loops and sections guarded by mutex that may become the bottleneck of the system. It is useful to monitor how "loaded" they are, and if increased load comes from increased number of calls. In future we can also record the number of times where a call is particularly long (> 1s or a user specified threshold).@andll if you would like to, I can update the other callsites of
TaskUtilizationExtand update the dashboards. Or I can revert the refactor in handle_consensus_transaction() etc.As @mystenmark suggested this can also be implemented via trace spans. But the API is more involved, and I'm a bit worried about performance regression in tracing hurting hot loop performance. We can revisit if needed.