Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http/pprof: traces collected from the /debug/pprof/trace endpoint don't include CPU profile samples by default #66679

Open
nsrip-dd opened this issue Apr 4, 2024 · 2 comments
Open

net/http/pprof: traces collected from the /debug/pprof/trace endpoint don't include CPU profile samples by default #66679

nsrip-dd opened this issue Apr 4, 2024 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@nsrip-dd
Copy link
Contributor

nsrip-dd commented Apr 4, 2024

The runtime execution tracer records CPU profiler samples if the CPU profiler is running during tracing. Having these samples contextualized in a trace is very useful. However, if I collect an execution trace via the /debug/pprof/trace endpoint, that execution trace will not include CPU profile samples by default. Currently I have to hit the /debug/pprof/profile endpoint simultaneously to activate CPU profiling, or otherwise arrange for it to be activated at the right time, to get this data in the trace. I think we should just have the /debug/pprof/trace endpoint turn on CPU profiling. I see two ways to go about this:

  1. Turn on CPU profiling unconditionally in /debug/pprof/trace
  2. Add a query parameter (cpuprofiler?) to the endpoint which, if provided, enables CPU profiling during the trace recording

I can't think of a reason why I wouldn't want CPU profile samples in an execution trace, so I might be inclined to option 1. However, having it on unconditionally would conflict with a user collecting a CPU profile via /debug/pprof/profile simultaneously: whichever endpoint call completes first would presumably stop CPU profiling, interfering with the other endpoint call. So perhaps option 2 is better?

cc @mknyszek
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 4, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2024
@rhysh
Copy link
Contributor

rhysh commented Apr 4, 2024

I can't think of a reason why I wouldn't want CPU profile samples in an execution trace

Stopping the CPU profile takes about 200ms (#63043), but I expect that extra delay would be fine for users of /debug/pprof/trace.

whichever endpoint call completes first would presumably stop CPU profiling, interfering with the other endpoint call

Maybe the question here is whether an error from runtime/pprof.StartCPUProfile should cause /debug/pprof/trace to outright fail?

@mknyszek mknyszek added this to the Backlog milestone Apr 10, 2024
@mknyszek
Copy link
Contributor

I think I personally prefer option 2. Though more broadly, I wonder if we should just rethink this package altogether. It has some behaviors (such as implicitly attaching to the default mux) that aren't great.

Maybe the question here is whether an error from runtime/pprof.StartCPUProfile should cause /debug/pprof/trace to outright fail?

This seems like the right behavior to me. It might be a bit surprising, but it's less surprising if you're explicitly asking for it. (What happens if you try to start two CPU profiles via net/http/pprof today?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rhysh @mknyszek @dmitshur @gopherbot @nsrip-dd