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

Make NumFDs not allocate excessively, implement throttling, and disable Prometheus process collector #1633

Merged
merged 13 commits into from
May 16, 2019
Prev Previous commit
Next Next commit
address feedback
Richard Artoul committed May 16, 2019
commit f7d36d9fad62d5f06d27df16e39b4d69150f2657
4 changes: 2 additions & 2 deletions src/x/instrument/config.go
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ import (
"io"
"time"

promfork "github.com/m3db/prometheus_client_golang/prometheus"
prom "github.com/m3db/prometheus_client_golang/prometheus"
"github.com/uber-go/tally"
"github.com/uber-go/tally/m3"
"github.com/uber-go/tally/multi"
@@ -88,7 +88,7 @@ func (mc *MetricsConfiguration) NewRootScope() (tally.Scope, io.Closer, error) {
// collecting the number of F.Ds for a process that has many of them can take a long
// time and be very CPU intensive, especially the Prometheus implementation which is
// less optimized than the M3 implementation.
Registry: promfork.NewRegistry(),
Registry: prom.NewRegistry(),
}
r, err := mc.PrometheusReporter.NewReporter(opts)
if err != nil {
60 changes: 30 additions & 30 deletions src/x/instrument/extended.go
Original file line number Diff line number Diff line change
@@ -184,36 +184,6 @@ type extendedMetricsReporter struct {
runtime runtimeMetrics
}

func (e *extendedMetricsReporter) Start() error {
if err := e.baseReporter.Start(); err != nil {
return err
}

if e.processReporter != nil {
if err := e.processReporter.Start(); err != nil {
return err
}
}

return nil
}

func (e *extendedMetricsReporter) Stop() error {
multiErr := xerrors.NewMultiError()

if err := e.baseReporter.Stop(); err != nil {
multiErr = multiErr.Add(err)
}

if e.processReporter != nil {
if err := e.processReporter.Stop(); err != nil {
multiErr = multiErr.Add(err)
}
}

return multiErr.FinalError()
}

// NewExtendedMetricsReporter creates a new extended metrics reporter
// that reports runtime and process metrics.
func NewExtendedMetricsReporter(
@@ -258,3 +228,33 @@ func NewExtendedMetricsReporter(

return r
}

func (e *extendedMetricsReporter) Start() error {
if err := e.baseReporter.Start(); err != nil {
return err
}

if e.processReporter != nil {
if err := e.processReporter.Start(); err != nil {
return err
}
}

return nil
}

func (e *extendedMetricsReporter) Stop() error {
multiErr := xerrors.NewMultiError()

if err := e.baseReporter.Stop(); err != nil {
multiErr = multiErr.Add(err)
}

if e.processReporter != nil {
if err := e.processReporter.Stop(); err != nil {
multiErr = multiErr.Add(err)
}
}

return multiErr.FinalError()
}
4 changes: 2 additions & 2 deletions src/x/process/process_linux.go
Original file line number Diff line number Diff line change
@@ -40,10 +40,10 @@ var (
doubleDotBytes = []byte("..")
)

// NumFDsReference returns the number of file descriptors for a given process.
// numFDsSlow returns the number of file descriptors for a given process.
// This is a reference implementation that can be used to compare against for
// correctness.
func NumFDsReference(pid int) (int, error) {
func numFDsSlow(pid int) (int, error) {
statPath := fmt.Sprintf("/proc/%d/fd", pid)
d, err := os.Open(statPath)
if err != nil {
19 changes: 13 additions & 6 deletions src/x/process/process_linux_test.go
Original file line number Diff line number Diff line change
@@ -33,6 +33,13 @@ import (

type cleanupFn func()

// Stdin
// stdout
// stderr
// /proc/<PID>/fd
// One more (not sure what it is, probably something related to the Go test runner.)
const numStdProcessFiles = 5

func TestNumFDs(t *testing.T) {
for i := 0; i <= 8; i++ {
var numFiles int
@@ -43,14 +50,14 @@ func TestNumFDs(t *testing.T) {
}

func() {
numExpectedFds := numFiles + 5
numExpectedFds := numFiles + numStdProcessFiles
cleanupFn := createTempFiles(numFiles)
defer cleanupFn()

selfPID := os.Getpid()

t.Run(fmt.Sprintf("func: %s, numFiles: %d", "NumFDsReference", numFiles), func(t *testing.T) {
numFDs, err := NumFDsReference(selfPID)
t.Run(fmt.Sprintf("func: %s, numFiles: %d", "numFDsSlow", numFiles), func(t *testing.T) {
numFDs, err := numFDsSlow(selfPID)
require.NoError(t, err)
require.Equal(t, numExpectedFds, numFDs)
})
@@ -76,15 +83,15 @@ func BenchmarkNumFDs(b *testing.B) {
// when performing actual benchmarking.
numFiles = 16000
// +5 to account for standard F.Ds that each process gets.
numExpectedFds = numFiles + 5
numExpectedFds = numFiles + numStdProcessFiles
)
cleanupFn := createTempFiles(numFiles)
defer cleanupFn()

selfPID := os.Getpid()
b.Run("NumFDsReference", func(b *testing.B) {
b.Run("numFDsSlow", func(b *testing.B) {
for i := 0; i < b.N; i++ {
numFDs, err := NumFDsReference(selfPID)
numFDs, err := numFDsSlow(selfPID)
if err != nil {
b.Fatal(err)
}