-
Notifications
You must be signed in to change notification settings - Fork 591
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.
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
test_utils: add profile_section
to get profiles in tests
#24246
base: dev
Are you sure you want to change the base?
Conversation
b4c05e2
to
62a4eed
Compare
}; | ||
|
||
ss::future<> profile_helper::start() { | ||
co_await _cpu_profiler.start( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason you didn't use the plain seastar APIs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the memory sampler I guess there isn't much reason not to use the seastar APIs. However, for the cpu profiler there is some logic in the service that I didn't want to replicate in this. I.e, setting up a timer to poll for samples from seastar. I'll stop using the memory sampler service here and use the APIs directly.
* Where `N` indicates which run of the section the profile is for, with 0 | ||
* indicating the first run. | ||
* | ||
* Note that this function will not override any existing files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer when it overwrites stuff because it means you don't have to change follow up post-processing steps but that is just my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, does this mean that if you run the benchmark twice, the files will be the ones written from the 1st run?
We should overwrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I avoided over-writing because I wasn't positive how we would want that to behave. If a given profile_section
runs multiple times during one execution of a binary do we want each of those profiles or only the latest? The easiest behavior would be just retaining the latest.
*/ | ||
ss::future<> profile_section( | ||
std::string_view section_name, | ||
std::function<ss::future<>()> section, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a ss::noncopyable_function to increase flexibility (since std::function can't handle move-only types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, will switch over to that.
Adds a convince wrapper around the cpu/memory profiling services so that they can easily be used in unit/microbenchmark tests. See header file for complete documentation/examples.
Backports Required
Release Notes