pprofui: add support for collecting goroutines with labels #105916
Conversation
|
This change is |
adityamaru
force-pushed
the
collect-job-stacks
branch
2 times, most recently
from
d3b6b78 to
c618d44
Compare
|
Tested on
|
There was a problem hiding this comment.
Looks good! I went a little nitty with the review just because I'm noticing that some practices we have "dialed in" on the replication team could be useful here too and I wanted to proselytise a little. Concretely, this could've been split up into a few commits where the logic changes are isolated and much easier to review. (As is, the review is a tad shallow, though I did make an effort at "really" reviewing the code). Please don't change it now, that's just busy work. I see this as an investment in the future - small commit style can be annoying at first, but in my opinion it's really worth it in that reviewers can much more easily engage with changes at a deeper level.
This is a great improvement, thanks for putting in the work! The main reason I'm requesting changes is that I feel strongly that non-fatal errors (missing nodes, etc) should be propagated back with the JSONResponse. I often look at clusters where we have partial data only (recently had one where it took >20 seconds just to render a 404...), and not knowing when you do can be difficult to deal with.
Other than that, mostly nits. If you haven't already, please give all of the profiles a smoke test. With changes like these, I can never quite be convinced that they work from code review alone, no matter how hard I look.
| var debug int | ||
| buf := bytes.NewBuffer(nil) | ||
| if req.WithLabels { | ||
| debug = 1 |
There was a problem hiding this comment.
nit: why the local debug and not just req.WithLabels everywhere?
There was a problem hiding this comment.
I'm not sure I follow this comment. The debug int is used in the WriteTo method call below.
There was a problem hiding this comment.
My thought was to s/debug/req.WithLabels/g. There's just one use of the debug local. Easier to read, for me at least, if the indirection is avoided. This is minor, so your call!
adityamaru
force-pushed
the
collect-job-stacks
branch
from
c618d44 to
b25619f
Compare
| var debug int | ||
| buf := bytes.NewBuffer(nil) | ||
| if req.WithLabels { | ||
| debug = 1 |
There was a problem hiding this comment.
My thought was to s/debug/req.WithLabels/g. There's just one use of the debug local. Easier to read, for me at least, if the indirection is avoided. This is minor, so your call!
adityamaru
force-pushed
the
collect-job-stacks
branch
from
b25619f to
fd32a93
Compare
This change refactors some of the logic in the pprofui/. Additionally, we add support for collecting cluster-wide goroutine profiles. These goroutine profiles can be collected with or without labels. When collected without labels we request pprof profiles with debug=0, which generates Profile protobufs on every node and `pprof.Merge`s them before redirecting the client to a flamegraph. When collected with labels we request pprof profiles with debug=1, which generates a legacy text format with comments translating addresses to function names and line numbers, so that a programmer can read the profile without tools. These profiles cannot be `pprof.Merged` and so we manually stitch them together on a per-node basis before downloading them for the client as txt file. This change also adds support for filtering the aforementioned goroutines by pprof label. This will be used by the Jobs Profiler to collect cluster-wide stacks relevant to the running job. Informs: cockroachdb#105440 Release note: None
adityamaru
force-pushed
the
collect-job-stacks
branch
from
fd32a93 to
b38951a
Compare
|
TFTR! bors r=tbg |
|
Build succeeded: |
This change collect cluster-wide goroutines that have a pprof label tying it to the particular job's execution, whose job execution details have been requested. This relies on the support added to the pprofui server to collect cluster-wide, labelled goroutines in cockroachdb#105916. Informs: cockroachdb#105076 Release note: None
106654: sql,server: support collecting labelled goroutines in the job profiler r=dt a=adityamaru This change collect cluster-wide goroutines that have a pprof label tying it to the particular job's execution, whose job execution details have been requested. This relies on the support added to the pprofui server to collect cluster-wide, labelled goroutines in #105916. Informs: #105076 Release note: None Co-authored-by: adityamaru <[email protected]>
This change refactors some of the logic in the pprofui/. Additionally, we add support for collecting cluster-wide goroutine profiles.
These goroutine profiles can be collected with or without labels. When collected without labels we request pprof profiles with debug=0, which generates Profile protobufs on every node and
pprof.Merges them before redirecting the client to a flamegraph.When collected with labels we request pprof profiles with debug=1, which generates a legacy text format with comments translating addresses to function names and line numbers, so that a programmer can read the profile without tools. These profiles cannot be
pprof.Mergedand so we manually stitch them together on a per-node basis before downloading them for the client as txt file.This change also adds support for filtering the aforementioned goroutines by pprof label. This will be used by the Jobs Profiler to collect cluster-wide stacks relevant to the running job.
Informs: #105440
Release note: None