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

Comments

@prashantv
Copy link
Contributor

@prashantv prashantv commented Jul 14, 2016

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

This comment has been minimized.

Copy link
Contributor

@quentinmit quentinmit commented Oct 11, 2016

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

@quentinmit quentinmit added the NeedsFix 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 and removed NeedsFix labels Oct 21, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 19, 2017
@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah 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

This comment has been minimized.

Copy link
Member

@aclements 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

This comment has been minimized.

Copy link
Member

@rakyll 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

This comment has been minimized.

Copy link

@cstyan cstyan commented Oct 2, 2017

any update? I could make use of this :)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 30, 2017

@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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 18, 2018

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

This comment has been minimized.

Copy link
Contributor

@cep21 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

This comment has been minimized.

Copy link
Contributor

@Sajmani 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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Jun 29, 2018

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 9, 2018

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

This comment has been minimized.

Copy link
Contributor

@Sajmani Sajmani commented Jul 9, 2018

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

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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 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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.