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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: os: profile open file descriptors through pprof #16379

Closed
prashantv opened this issue Jul 14, 2016 · 17 comments
Closed

proposal: os: profile open file descriptors through pprof #16379

prashantv opened this issue Jul 14, 2016 · 17 comments
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal
Milestone

Comments

@prashantv
Copy link
Contributor

The runtime should profile file descriptors (possibly in a similar way to memory profiling, controlled by an environment variable) and expose this information over pprof profiles.

E.g., when there's a file descriptor leak, it would be great to know:

  • what type of fds are leaking (files, sockets)
  • for sockets, more information about the remote peer
  • call stack when the file descriptor was allocated

By default the rate of profiling could be 0% or 1% to avoid impacting performance, but the ability to turn this up to 100% would significantly improve the experience for debugging file descriptor leaks. The cost of capturing the call stack shouldn't significantly effect the open operation either.

cc @Sajmani @bradfitz

@quentinmit
Copy link
Contributor

Perhaps opening an fd is rare enough that we could just trace at 100% by default.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 21, 2016
@rsc rsc changed the title runtime: Profile open file descriptors through pprof os: profile open file descriptors through pprof Oct 21, 2016
@rsc rsc added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 21, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 19, 2017
@hyangah
Copy link
Contributor

hyangah commented Aug 18, 2017

I also think opening an fd is relatively rare enough (compared to memory allocation), so it can be either 100% or not.

I can imagine capturing the type of fds and the call stack can be done with the runtime/pprof api. But, I can't think of a good way to maintain detailed info more than call stack (such as peers of socket type fds) with the current runtime/pprof api in a scalable way.

By the way, just out of curiosity - I am still not sure why it is a good thing to be done from go runtime or standard packages. In google, at least, many of fd are instrumented from higher level packages (e.g. rpc, google file, ...). Not perfect, but could be done outside go runtime or standard packages. Sure, if they were profiled already from runtime, we wouldn't have to write the instrumentation code from higher level packages. :-)

@aclements
Copy link
Member

aclements commented Aug 18, 2017

Perhaps opening an fd is rare enough that we could just trace at 100% by default.

I also think opening an fd is relatively rare enough (compared to memory allocation), so it can be either 100% or not.

I have no data, but I'm not sure I believe this. I'm worried about the case where a server accepts connections at a very high rate but does very little per-connection work. But maybe the cost of accepting a connection already dwarfs the cost of doing the stack walk.

@rakyll
Copy link
Contributor

rakyll commented Aug 21, 2017

By the way, just out of curiosity - I am still not sure why it is a good thing to be done from go runtime or standard packages.

AFAIK, most users who are asking for the standard library support are already import leaking libraries and they have no way to change the abstractions at the dependencies layer to add custom profiling.

@cstyan
Copy link

cstyan commented Oct 2, 2017

any update? I could make use of this :)

@bradfitz
Copy link
Contributor

Ping @golang/proposal-review (@rsc, @ianlancetaylor, et al)

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 30, 2017
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
@rsc rsc changed the title os: profile open file descriptors through pprof proposal: os: profile open file descriptors through pprof Jun 11, 2018
@ianlancetaylor
Copy link
Contributor

The idea sounds good in principle. It's not completely clear what this would look like for the user. It would be useful to develop a prototype to confirm that it behaves as expected. What level should we profile at?
Do we need to track every open, socket, and close system call?

@cep21
Copy link
Contributor

cep21 commented Jun 21, 2018

To debug a DNS connectivity issue a few months ago, I went into writing a draft that did this. My implementation was to add it next to the runtime.SetFinalizer(fd, (*netFD).Close) call, since I thought that would be a good source of truth for when the descriptors are created, and I would remove it on the Close call.

But then I went down that rabbit hole and realized the finalizer was the place all roads go. The bigger problem was to track everything that wanted a Finalizer ran, but hadn't ran yet. This covers file descriptors, sql connections, exec (not tickers though) and pretty much everything else in the category of leaked things that need to be cleaned up, so they are probably important.

I wonder if a utility that can create pprof style stacks of finalizers by object type would be generally useful.

@Sajmani
Copy link
Contributor

Sajmani commented Jun 29, 2018

I think we could prototype this without finalizers by instrumenting standard library calls with a custom pprof profile (pprof.NewProfile). A strawman might be an internal fileprof package that's called from os and the net packages and the corresponding Closers. Or perhaps do it directly in the syscall package, if that's common for all fd operations.

@josharian
Copy link
Contributor

Somewhat related: #18454. I keep meaning to work on that but I have more projects than time, and the ratio keeps worsening. :)

@gopherbot
Copy link

Change https://golang.org/cl/122617 mentions this issue: os: add a pprof profile for open os.Files. A profile entry is added each time a

@Sajmani
Copy link
Contributor

Sajmani commented Jul 9, 2018

I uploaded a proof-of-concept in https://golang.org/cl/122617 .

@rsc
Copy link
Contributor

rsc commented Jul 9, 2018

Semantically, putting a File in a profile will keep it from getting garbage collected, so a program might start running out of fds where it wouldn't before. If the profile keys were the fd integers, then it would be OK. But it's still a lot of overhead being added to Open/Close.

@rsc
Copy link
Contributor

rsc commented Jul 9, 2018

For the 1% sampling suggested in the original report, that should already show up in the memory profiles (as the allocation of the os.File struct). The rest of the discussion here seems to be about a 100% profile, which would certainly be more useful.

Maybe as an opt-in?

@rsc
Copy link
Contributor

rsc commented Jul 23, 2018

runtime/pprof imports os, so we'd have to do this through some extra internal third package they both import, to let them see each other. And it would only kick in if runtime/pprof were in the binary anyway. And we'd need some function to enable it from os (we don't want everything with os to require runtime/pprof). And that function would have to panic if runtime/pprof were unavailable. Seems like a lot of complexity but we could do it.

@rsc
Copy link
Contributor

rsc commented Jul 23, 2018

For the 1% it seems like if you took a memory profile and loaded it into pprof and told pprof to focus on os.NewFile, that would show the stacks already. Maybe that's enough.

@rsc
Copy link
Contributor

rsc commented Aug 6, 2018

It doesn't sound like anyone is arguing strenuously for this. For sampled profiling, the memory objects should suffice. Let's leave things as they are instead of adding complexity.

@rsc rsc closed this as completed Aug 6, 2018
@golang golang locked and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal
Projects
None yet
Development

No branches or pull requests