debug: add trace flight recorder #5337
Conversation
github-actions
bot
added
area/hack
| @@ -1,6 +1,6 @@ | |||
| module github.com/moby/buildkit | |||
|
|
|||
| go 1.21.0 | |||
| go 1.22.0 | |||
There was a problem hiding this comment.
Thanks! 1.22 makes more sense to me, but since the same file was using 1.21.0 I kept it pinned to a patch release just in case there was some BuiltKit convention I was not aware of. I've updated it to use 1.22.
There was a problem hiding this comment.
but since the same file was using
1.21.0
this was because of a dep forcing it, see #5118 (comment)
There was a problem hiding this comment.
ah actually I was wrong, exp/trace also set patch so we need it as well https://cs.opensource.google/go/x/exp/+/master:go.mod;l=3
There was a problem hiding this comment.
I see, good point. I've changed it back to 1.22.0.
There was a problem hiding this comment.
check missing DCO https://github.com/moby/buildkit/pull/5337/checks
fiam
force-pushed
the
alberto/debug-flight-record
branch
from
f9678a2
to
34f4993
Compare
Thanks! I've added it. |
fiam
force-pushed
the
alberto/debug-flight-record
branch
from
34f4993
to
9f0a241
Compare
cmd/buildkitd/debug_flight.go
Outdated
| m.HandleFunc("POST "+flightTracePattern+"/set_period", r.SetTracePeriod) | ||
| m.HandleFunc("GET "+flightTracePattern, r.Trace) | ||
|
|
||
| m.HandleFunc("POST "+flightCPUPattern+"/start", r.StartCPUProfile) |
There was a problem hiding this comment.
These are already available in /debug/pprof/profile
There was a problem hiding this comment.
These are not exactly the same. The default ones require you to specify the profile duration beforehand, while these ones allow you start a continous CPU profile and stop it later (e.g. in response to some event).
There was a problem hiding this comment.
The default does seem safer. If we want to keep the new ones then:
- Why do these make sense under
/flightas it is just plain CPU profile. StartCPUProfileshould stop the profiling when connection drops and request context gets cancelled.
There was a problem hiding this comment.
I think it might be better to remove the CPU profiler endpoints from this PR and focus on the trace recorder. We can add the CPU profilers in a subsequent PR. WDYT?
There was a problem hiding this comment.
I've left only the trace profiler. PTAL.
cmd/buildkitd/debug_flight.go
Outdated
| http.Error(w, fmt.Sprintf("could not start CPU profile: %s", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Technically, a Stop request can come in here before the Wait() and cause this function to hang forever. Especially if r.mu is already locked from other flight requests.
There was a problem hiding this comment.
As discussed, we've split this part into a separate PR.
fiam
changed the title
fiam
force-pushed
the
alberto/debug-flight-record
branch
from
9f0a241
to
902ba23
Compare
cmd/buildkitd/debug_flight.go
Outdated
|
|
||
| type flightRecorder struct { | ||
| mu sync.Mutex | ||
| cond *sync.Cond |
There was a problem hiding this comment.
Thanks! It should be good now.
Use golang.org/x/exp/trace to implement an trace recorder that saves the trace to a circular buffer and can be retrieved at any time. Debug endpoints have been added under /debug/flight to start and stop the trace as well as to set its period. Due to golang.org/x/exp/trace, the minimum go version has been bumped to 1.22 Signed-off-by: Alberto Garcia Hierro <[email protected]>
fiam
force-pushed
the
alberto/debug-flight-record
branch
from
902ba23
to
892e756
Compare
|
Thanks @tonistiigi and @crazy-max 🤝 |
debug: add trace flight recorder
Use golang.org/x/exp/trace to implement an trace recorder that saves the trace
to a circular buffer and can be retrieved at any time.
Debug endpoints have been added under /debug/flight to start and stop the trace
as well as to set its period.
Due to golang.org/x/exp/trace, the minimum go version has been bumped to 1.22