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.

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

Already on GitHub? Sign in to your account

proposal: net/http/pprof: provide a mechanism to render on /debug/pprof profiles than those that invoke pprof.NewProfile #41324

Closed
brancz opened this issue Sep 10, 2020 · 17 comments

Comments

@brancz
Copy link
Contributor

brancz commented Sep 10, 2020

I'm investigating the possibility of discovering available profiles of a process through the /debug/pprof/ index page. If that's a bad idea then we can stop right here 馃檪 , but if not, then it would be great if there could be a mechanism to extend the profiles rendered on this page.

I have looked around and found that it appears that if profiles are created using the pprof.NewProfile function, then they are automatically included in profiles returned by pprof.Profiles(), which is what the /debug/pprof index page calls to retrieve the list of available profiles.

The custom profile I was looking at including is fgprof, however, fgprof is unsuitable for making use of pprof.NewProfile(). This led me to opening this issue.

Would it be possible to have another mechanism to add profile endpoints to be rendered into the /debug/pprof/ index page? I could see the possibility of an extensible list of endpoints in net/http/pprof/.

Happy to discuss other strategies if the general idea sounds acceptable, and once agreed I'd also be happy to contribute it.

Related: felixge/fgprof#5 parca-dev/parca#69

@odeke-em odeke-em changed the title Extensible pprof index page proposal: net/http/pprof: provide a mechanism to render on /debug/pprof profiles than those that invoke pprof.NewProfile Sep 11, 2020
@gopherbot gopherbot added this to the Proposal milestone Sep 11, 2020
@odeke-em
Copy link
Member

Thank you for this report @brancz and welcome to the Go project!
Awesome to see diversity of profilers in the Go community. I've converted this issue into a proposal since it is anyways a request
that we allow some hooks for registering hooks into net/http/pprof's /debug/pprof to render profiles.

The proposal committee shall take a look and also others, please feel to chime in here with ideas or suggestions.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Sep 12, 2020
@rsc
Copy link
Contributor

rsc commented Sep 16, 2020

It sounds like the request is to be able to add arbitrary HTML (or at least linked text) to the /debug/pprof page?
That's not something we've done before and it seems like it might easy to abuse.

Maybe instead the /debug page (which I think does nothing now) should list the available pages under /debug?
Then when fgprof registers /debug/fgprof, it gets listed in /debug automatically?

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 16, 2020
@brancz
Copy link
Contributor Author

brancz commented Sep 17, 2020

I don't think arbitrary HTML is required, just a link with a name, much like what profile.NewProfile(name) does (it wouldn't need to be more flexible than profile.NewProfile).

@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

@brancz What is the exact API, with function signatures and doc comments, that you are proposing?

@brancz
Copy link
Contributor Author

brancz commented Sep 20, 2020

I'm thinking in the direction of a new function called AddProfile in the net/http/pprof package.

// AddProfile adds the profileHandler, serving a custom profile, to be served under `/debug/pprof/<name>` as well as list the profile with its description on the `/debug/pprof` index page.
func AddProfile(name, description string, profileHandler http.Handler)

@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

It still seems a bit odd that this is for registering non-pprof profiles.

Above I suggested:

Maybe instead the /debug page (which I think does nothing now) should list the available pages under /debug? Then when fgprof registers /debug/fgprof, it gets listed in /debug automatically?

Any reaction to that? It seems clearer not to put it in the /debug/pprof list or URL.

@brancz
Copy link
Contributor Author

brancz commented Sep 24, 2020

What do you mean by non-pprof profiles? fgprof does produce profiles in pprof format.

Any reaction to that? It seems clearer not to put it in the /debug/pprof list or URL.

Maybe I misunderstood the suggestion. For me, if the result is an endpoint (I don't really care about the path) that lists all available profile endpoints and having this list be extensible with further profiles like fgprof, then that would be sufficient.

@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

Sorry, I didn't realize fgprof used pprof format.

Suppose we did this somehow. It's not enough to hijack the HTTP view. We'd need to make runtime/pprof.Profiles include it in the list. That would mean some way to provide a different implementation that constructed a *Profile and could answer the WriteTo method directly. Maybe that would be enough?

type Custom interface {
    WriteTo(w io.Writer, debug int) error
}

func NewCustom(name string, impl Custom) *Profile

The Add/Count/Remove methods on this Profile would panic I guess.

This raises the question of why fgprof isn't using NewProfile and p.Add directly, but I assume there's a good reason.

@rsc
Copy link
Contributor

rsc commented Oct 1, 2020

I spent a little bit looking at fgprof. I don't see how this would help fgprof, which has additional parameters on the URL handler. /debug/pprof/fgprof would not accept (or pass through) the format= argument in this example from the home page:

curl -s 'localhost:6060/debug/fgprof?seconds=3&format=folded' > fgprof.folded

Also, fgprof seems to be a special case in that I am aware of no other profile that would need this functionality.
I still lean toward letting it use /debug/fgprof and leave things alone.

I looked at whether /debug/ can show an index of things registered under /debug/, but http.ServeMux doesn't make that available, and I'm reluctant to change that at this point.

It sounds like maybe there's not much to do here.

@brancz
Copy link
Contributor Author

brancz commented Oct 2, 2020

It does behave by default like the CPU profile though, producing pprof format instead of the folder format.

That said, if this is too niche, I could understand until there may be more cases like fgprof.

@felixge
Copy link
Contributor

felixge commented Oct 4, 2020

@rsc fgprof author here. The format argument for fgprof isn't critical, I think most people will prefer the default pprof output.

Also, do you think it's worth proposing something similar to fgprof to be included in the go project itself? I think that could simplify the integration with http/pprof, as it could be done without expanding the public API surface. If there is no obvious reason against it, I'd be happy to write a proposal and submit some code for it.

@rsc
Copy link
Contributor

rsc commented Oct 5, 2020

@felixge, I'm not really sure how viable fgprof is beyond small examples (and in particular single-goroutine programs).

I see in the code that fgprof samples all goroutines 99 times per second. That won't scale to large programs with 1000s of goroutines. (Not to mention that the current implementation stops the program entirely to get the stack traces.)

Instead we'd probably have to pick a random goroutine and profile that one. But we still don't have a way to stop a goroutine running on a different thread. I suppose we could pretend the OS has already chosen among the running threads fairly and say that if we randomly pick any running goroutine then the we record the one on the current thread. That might be OK. It's still even racy to grab the stack of a stopped goroutine without stopping the world, although maybe we could fix that (the fix would be pretty subtle).

But then once that is done, any multi-goroutine program is going to end up with a profile weighted by number of goroutines. Kick off 10 goroutines calling Sleep, and Sleep looks 10X more expensive than the rest of the program. It's just not clear to me that this is a useful kind of profile in general. I do see the benefit in small programs like in your example. It works out to a nice trace for those.

For us on the Go team, I think improved tracing would be a better place to spend our effort than working out how to build fgprof into the core Go runtime.

@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Oct 7, 2020
@felixge
Copy link
Contributor

felixge commented Oct 8, 2020

@rsc thanks for your detailed reply. Below are a few more thoughts on a potential proposal for supporting wallclock profiling similar to fgprof in Go itself. Please let me know if you really think it's a dead-end or if I should open a new issue for this.

@felixge, I'm not really sure how viable fgprof is beyond small examples (and in particular single-goroutine programs).

I see in the code that fgprof samples all goroutines 99 times per second. That won't scale to large programs with 1000s of goroutines. (Not to mention that the current implementation stops the program entirely to get the stack traces.)

Scalability is a concern, I agree.

Instead we'd probably have to pick a random goroutine and profile that one. But we still don't have a way to stop a goroutine running on a different thread. I suppose we could pretend the OS has already chosen among the running threads fairly and say that if we randomly pick any running goroutine then the we record the one on the current thread. That might be OK. It's still even racy to grab the stack of a stopped goroutine without stopping the world, although maybe we could fix that (the fix would be pretty subtle).

Yeah, I'd be happy to investigate and evaluate different approaches. I understand why the Go core should be hesitant to integrate fgprof "as is".

But then once that is done, any multi-goroutine program is going to end up with a profile weighted by number of goroutines. Kick off 10 goroutines calling Sleep, and Sleep looks 10X more expensive than the rest of the program. It's just not clear to me that this is a useful kind of profile in general. I do see the benefit in small programs like in your example. It works out to a nice trace for those.

I agree that the overall profile/flamegraph produced by fgprof will be unintelligible for non-trivial programs. That being said, once you filter the graph down to a stack prefix that corresponds to e.g. your http handler, the results become pretty useful in my experience.

For us on the Go team, I think improved tracing would be a better place to spend our effort than working out how to build fgprof into the core Go runtime.

I agree that fgprof is problematic as-is. But I think wallclock profiling is a big gap in Go's otherwise excellent profiling functionality. And the number of stars on the fgprof repo shows a decent amount of community interest.

So I'd be okay to go back to the drawing board for the implementation based on your concerns. If there are already tracing based ideas floating around, please let me know, otherwise I'd be happy to create a new issue/proposal for "wallclock profiling". What do you think?

@rsc
Copy link
Contributor

rsc commented Oct 12, 2020

If there are already tracing based ideas floating around, please let me know, otherwise I'd be happy to create a new issue/proposal for "wallclock profiling". What do you think?

I don't know of any tracing ideas that are concrete enough to be proposals.

I would suggest not proposing "wallclock profiling", for the reasons I gave in my earlier reply. One you get past small examples, it stops being useful. Probably the right thing to do is figure out more of a trace like the current trace profiles but perhaps less low level.

@felixge
Copy link
Contributor

felixge commented Oct 13, 2020

I would suggest not proposing "wallclock profiling", for the reasons I gave in my earlier reply. One you get past small examples, it stops being useful. Probably the right thing to do is figure out more of a trace like the current trace profiles but perhaps less low level.

Ok, got it. I'll take a look and let you know if I can come up with some ideas based on this direction.

@rsc
Copy link
Contributor

rsc commented Oct 14, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Oct 14, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Oct 14, 2020
@golang golang locked and limited conversation to collaborators Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants