cpu_profiler: add scheduler group #25394
Conversation
travisdowns
requested review from
StephanDollberg
and removed request for
a team
CI test resultstest results on build#63191
test results on build#63208
test results on build#63243
test results on build#63347
|
| for (auto& result : results_buffer.samples) { | ||
| backtraces[result.user_backtrace]++; | ||
| ++backtraces[{result.user_backtrace, result.sg.name()}]; |
There was a problem hiding this comment.
This will still allow us to later aggregate the scheduling group away on the pprof side right?
There was a problem hiding this comment.
Yeah, and I think that happens by default? I.e., it doesn't split by additional tags by default.
... and currently I don't do anything with this attribute in the convert.py, so right now it should always be aggregated together (but I better double check the pprof behavoir here: that it's fine to get two identical backtraces in its input and sums their occurrences).
There was a problem hiding this comment.
@ballard26 will probably know but I guess it begs the question why we do this sample aggregation in the first place.
There was a problem hiding this comment.
@StephanDollberg - yeah I'm pretty sure pprof would work fine if every sample just had occurrences=1 and there were repeats. I guess the aggregation helps reduce the size of everything after it happens, e.g., the json response, etc. Aggregation makes sense to me currently though if we want to add more attributes it might stop working at some point, e.g., if added a timestamp or serial number then every sample would be unique.
There was a problem hiding this comment.
PProf works fine with identical samples AFAIK. I added aggregation for memory usage and json size savings.
There was a problem hiding this comment.
I.e, currently we see PProf aggregate identical traces from different shards as we'd expect when the shard label isn't being set as the tag root.
There was a problem hiding this comment.
@StephanDollberg tested a bit and confirmed:
- pprof seems fine with the same stack multiple times in the input, in views like flame graph it just sums the sample count from any identical stacks
- the difference is still visible in some cases, e.g., at
pprofCLI usingtracesshows unaggregated values, similar to the input (i.e., if you have two identical stacks with 5 and 5 samples, they behave the same mostly as 1 stack with 10 samples, but intracesoutput you do see 5/5
None of the above really has anything to with scheduling group per-se: that is a "label" and doesn't have any effect on the aggregation by default. If you use --tag_root then it uses the tag to create a synthetic root node as you are probably aware.
Here's a example from localhost using --tag_root scheduling_group:
Note that I added an sg: prefix to the scheduling group, since otherwise main group gets conflated with main() function causing weirdness.
There was a problem hiding this comment.
Sorry I hadn't refreshed and seen @ballard26 comments before posting my reply but they amount to the same thing.
There was a problem hiding this comment.
Great. Btw did you test does --tag_root take multiple args? I.e.: can you do shard+sg?
There was a problem hiding this comment.
@StephanDollberg yes, it does, here's the result of pprof -tagroot thread_id,scheduling_group -http 127.0.0.1:10002 prof.proto:
travisdowns
force-pushed
the
td-sg-in-cpu-profile
branch
from
d28b526 to
ea5accd
Compare
|
/ci-repeat 1 |
Add scheduler group to each sample. This is recorded on the seastar at the moment of the sample side and we now include it in the output. This is useful for understanding what is running in what scheduler group. Add a new unit test case for this functionality.
travisdowns
force-pushed
the
td-sg-in-cpu-profile
branch
from
ea5accd to
67a9d53
Compare
|
Push 67a9d53 removes the shared commit with an PR that went in earlier. |
|
Oops! Something went wrong. |
|
/backport v25.1.x |
cpu_profiler: add scheduler group
Add scheduler group to each sample. This is recorded on the seastar at
the moment of the sample side and we now include it in the output.
This is useful for understanding what is running in what scheduler
group.
Backports Required
Release Notes
Improvements