server: enable continuous CPU profiler #118850
Conversation
aadityasondhi
requested review from
abarganier
and removed request for
a team
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
aadityasondhi
force-pushed
the
20240206.enable-cont-profiler
branch
from
672ff8c
to
7196e2c
Compare
There was a problem hiding this comment.
I think there was some original desire for benchmarking, but I feel fairly confident that this is acceptable given the 20 minute interval. I also recall @Santamaura doing a good amount of benchmarking originally and not being able to find any notable performance hits. FWIW, the CRL telemetry cluster has had this feature enabled with a 5 minute interval and 80% threshold for many months now and hasn't experienced issues.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @kvoli and @sumeerbhola)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
|
Am I correct in understanding that the CPU metric will be read and the threshold will be checked (in Can we reduce the threshold to 65%? We have seen examples of high runnable goroutines when the CPU utilization doesn't look very high (because the utilization is smoothing over some interval). For example: |
There was a problem hiding this comment.
Reviewable status:
|
Discussed offline with @aadityasondhi, we should discuss the cleanup story here. Do we need to also introduce a mechanism which removes profiles after X days or rotates profiles so that there is a ceiling? Probably worth looking into how memory profiles profiles are handled. |
There was a problem hiding this comment.
TFTR all!
@sumeerbhola Yes that is correct, startSampleEnvironment is where we read the metric and it is read at the sampling interval which is defaulted to 10s. Lowered to 65%.
@kvoli If I am reading the code in startSampleEnvironment correctly, it seems that both the heap and cpu profiler use the same underlying pattern neither do any sort of clean up. A new file seems to be generated each time the profile is taken. I think it is safe to punt this for now and revisit the gc story if it becomes a problem, thoughts?
Reviewable status:
There was a problem hiding this comment.
Correction to above, we do have a GC policy in all of our dump stores. For CPU profiles specifically, we use the cluster setting server.cpu_profile.total_dump_size_limit, which is set to 128MiB as the default.
Reviewable status:
This patch enables the CPU profile with a threshold of 75% and a max frequency of 20min. The motivation for this change is that recently, I have noticed a few cases during investigations where CPU utilization spikes but we lack profiles after the fact. This hinders our ability to dig deeper into the source of high CPU usage. Having profiles can help inform us of future AC integrations that we may need, or other performance improvements we can do elsewhere. Informs cockroachdb#97699. Release note (ops change): CRDB will now automatically generate CPU profiles if there is an increase in CPU utilization. This can help investigate possible issues after the fact.
aadityasondhi
force-pushed
the
20240206.enable-cont-profiler
branch
from
7196e2c
to
9036430
Compare
|
bors r+ |
|
Build succeeded: |
This patch enables the CPU profile with a threshold of 75% and a max frequency of 20min.
The motivation for this change is that recently, I have noticed a few cases during investigations where CPU utilization spikes but we lack profiles after the fact. This hinders our ability to dig deeper into the source of high CPU usage. Having profiles can help inform us of future AC integrations that we may need, or other performance improvements we can do elsewhere.
Informs #97699.
Release note (ops change): CRDB will now automatically generate CPU profiles if there is an increase in CPU utilization. This can help investigate possible issues after the fact.