Add arch/version to cpu profiler #24658
Conversation
travisdowns
commented
Dec 24, 2024
travisdowns
commented
- debug api: wrong ID for cpu_profile_shard_samples
- remove unused headers
- utils: add arch library
- cpu_profiler: expose the sample period
- admin: include more info in cpu profile sample
travisdowns
changed the title
travisdowns
force-pushed
the
td-add-arch-version-utils
branch
from
1b69f68 to
d339977
Compare
Retry command for Build#60132please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#60132
test results on build#60226
test results on build#61969
test results on build#61976
test results on build#62410
|
No it's coming, mostly just wanted to put this v1 up to see if there were concerns with the approach, etc. |
Retry command for Build#60226please wait until all jobs are finished before running the slash command |
travisdowns
force-pushed
the
td-add-arch-version-utils
branch
from
5499d7a to
b455fd6
Compare
travisdowns
force-pushed
the
td-add-arch-version-utils
branch
2 times, most recently
from
cf884c4 to
bfea0d9
Compare
Retry command for Build#61969please wait until all jobs are finished before running the slash command |
|
We should also include the scheduling group id to name mapping such that we can reliably map the tags in pprof? |
Sure, but in another change? This series doesn't deal with SG changes at all, and in any case for that change I would assume that we just have the SG name directly in the json, attached to each sample, no need for a "mapping"? |
sure
Idk seems wasteful |
What alternative you considering? Using the numeric ID instead? Wasteful in terms of bytes in the json? I feel like a typical sample is already 100s of bytes? |
Yes, basically just do the conversion in
Bytes and compute/serialization.
Sure but that doesn't mean we can't save some bytes where it's not needed :) (basically the same discussion as to whether we include the arch/version in every stack/sample). |
I see those cases as quite different. Including the arch/version on every sample is the "wrong" schema, we'd just do that to avoid breaking the format, but you'd never want a schema like that if you designed it from scratch as logically the version/schema cannot vary among samples. So I think efficiency is a secondary concern there: actually having a schema that makes sense is the main driver. This other thing is purely a case of efficiency I think? Scheduler groups are logically identified by their name (note [1]), so the natural schema is just to use the name as an attribute of the sample as I see it. Doing the mapping "works" but is unwieldy and so the savings should be pretty decent. Effectively it's some kind of ad-hoc compression algorithm. As for perf, not even sure if it's faster due to the need to do the lookup per [1]? BTW you could apply the same sort of reasoning to individual frames which have a lot of duplication between samples and there I guess the savings would be significant (though frames are already fairly small as they are addresses, if they are in the main binary). [1] Note that the ID is a hidden detail of the seastar impl and not accessible, so if we want to use an ID we will probably just reconstruct it at serialization time, i.e., arbitrarily assigning incrementing integers to scheduling groups as we find them. In seastar the assigned IDs depend on the order of "create sg" calls. |
Yeah that's true.
Huh that is surprising. That does render the point moot indeed.
Right but here you'd actually have to do work to deduplicate them. If the id was accessible you could have just serialized that instead of transforming it to the string name first. |
travisdowns
force-pushed
the
td-add-arch-version-utils
branch
from
c275bed to
1bc1852
Compare
Yeah I had originally considered putting the ID, but then I realized the ID is not "fixed" and worse, it is not really accessible (sg ID is private only "name" and "short name" are exposed). It is still possible but we'd just need to generate IDs at serialization time. |
id was set to cpu_profile_sample, should be cpu_profile_shard_samples.
A small header which exposes the architecture of the current process as a constexpr value. We will use this in the cpu_profiler in order to record the arch in the output. Includes a (very trivial) test.
Expose this so we can include it in the admin API result. This can be useful to estimate how busy the reactor was, as we can calculate the utilization based on the expected number of samples (at 100% util) vs the observed number.
Change the output format of the cpu profiler API to include: - The CPU architecture - The version string - The wait_ms, if specified - The profiler sample period - The schema version of the API response The first two of the above enable us to symbolize profiles directly from the result without needing to know the version/arch and download symbols separately.
Add an option to set the stack depth of the of spin loop in the stress fiber, i.e., the spin loop will at the end of a recursive call chain (not inlined) of depth N. Good for stressing CPU proflier.
Update the attribute descriptions to be consistent with the rest of the api-doc, by Kat Batuigas. Co-authored-by: Kat Batuigas <[email protected]>
Let the stress fiber config be formatted, and print the config details when the admin API endpoint is called.
Add some tests for the v2 format, and stress tests which try to have something close to a worst-case scenario for CPU profiling, i.e., a large number of deep stacks. In order to implement the stress test, we augment the stress fiber with a mode which spins at a specific stack depth, i.e., calls N functions before spinning so that the stack depth will be at least N (in practice it will be somewhat more than N because there are other frames in the stack beyond the ones added explicitly by the spinner). Update existing DT tests for the new format.
When we get an ERROR log line which is allowed (i.e., should not fail the test), we output this info at WARN log level. WARN log lines during dt runs go to the "session log", i.e., the log that appears on stdout so this often spams the console and session log with tons of benign messages about allowed log lines. Demote this to INFO instead. It will still show in the test log.
Add type annotations to some function parameters.
travisdowns
dismissed stale reviews from kbatuigas and StephanDollberg
via
e3a69a2
travisdowns
force-pushed
the
td-add-arch-version-utils
branch
from
1bc1852 to
e3a69a2
Compare
|
e3a69a2 is a pure rebase. |
