-
Notifications
You must be signed in to change notification settings - Fork 435
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
profiler: start collecting profiles immediately #1417
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,6 @@ import ( | |
"runtime" | ||
"strings" | ||
"sync" | ||
"sync/atomic" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -167,17 +166,20 @@ func TestStartStopIdempotency(t *testing.T) { | |
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
for j := 0; j < 1000; j++ { | ||
for j := 0; j < 20; j++ { | ||
// startup logging makes this test very slow | ||
Start(WithLogStartup(false)) | ||
// | ||
// Also, the CPU profile is really slow to stop (200ms/iter) | ||
// so we just do the heap profile | ||
Start(WithLogStartup(false), WithProfileTypes(HeapProfile)) | ||
} | ||
}() | ||
} | ||
for i := 0; i < 5; i++ { | ||
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
for j := 0; j < 1000; j++ { | ||
for j := 0; j < 20; j++ { | ||
Stop() | ||
} | ||
}() | ||
|
@@ -241,58 +243,6 @@ func TestStopLatency(t *testing.T) { | |
} | ||
} | ||
|
||
func TestProfilerInternal(t *testing.T) { | ||
t.Run("collect", func(t *testing.T) { | ||
p, err := unstartedProfiler( | ||
WithPeriod(1*time.Millisecond), | ||
CPUDuration(1*time.Millisecond), | ||
WithProfileTypes(HeapProfile, CPUProfile), | ||
) | ||
require.NoError(t, err) | ||
var startCPU, stopCPU, writeHeap uint64 | ||
p.testHooks.startCPUProfile = func(_ io.Writer) error { | ||
atomic.AddUint64(&startCPU, 1) | ||
return nil | ||
} | ||
p.testHooks.stopCPUProfile = func() { atomic.AddUint64(&stopCPU, 1) } | ||
p.testHooks.lookupProfile = func(name string, w io.Writer, _ int) error { | ||
if name == "heap" { | ||
atomic.AddUint64(&writeHeap, 1) | ||
} | ||
_, err := w.Write(textProfile{Text: "main 5\n"}.Protobuf()) | ||
return err | ||
} | ||
|
||
tick := make(chan time.Time) | ||
wait := make(chan struct{}) | ||
|
||
go func() { | ||
p.collect(tick) | ||
close(wait) | ||
}() | ||
|
||
tick <- time.Now() | ||
|
||
var bat batch | ||
select { | ||
case bat = <-p.out: | ||
case <-time.After(200 * time.Millisecond): | ||
t.Fatalf("missing batch") | ||
} | ||
|
||
assert := assert.New(t) | ||
assert.EqualValues(1, writeHeap) | ||
assert.EqualValues(1, startCPU) | ||
assert.EqualValues(1, stopCPU) | ||
|
||
// should contain cpu.pprof, metrics.json, delta-heap.pprof | ||
assert.Equal(3, len(bat.profiles)) | ||
|
||
p.exit <- struct{}{} | ||
<-wait | ||
}) | ||
} | ||
|
||
func TestSetProfileFraction(t *testing.T) { | ||
t.Run("on", func(t *testing.T) { | ||
start := runtime.SetMutexProfileFraction(0) | ||
|
@@ -568,3 +518,32 @@ func TestTelemetryEnabled(t *testing.T) { | |
check("profile_period", time.Duration(10*time.Millisecond).String()) | ||
check("cpu_duration", time.Duration(1*time.Millisecond).String()) | ||
} | ||
|
||
func TestImmediateProfile(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I've come up with so far for testing this change. Time-based tests aren't my favorite but I think this should be okay & not flaky. I'm open to other ideas! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm okay with this test. The alternative would be a test that uses an unstartedProfiler and calls the |
||
received := make(chan struct{}, 1) | ||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
select { | ||
case received <- struct{}{}: | ||
default: | ||
} | ||
})) | ||
defer server.Close() | ||
|
||
err := Start( | ||
WithAgentAddr(server.Listener.Addr().String()), | ||
WithProfileTypes(HeapProfile), | ||
WithPeriod(3*time.Second), | ||
) | ||
require.NoError(t, err) | ||
defer Stop() | ||
|
||
// Wait a little less than 2 profile periods. We should start profiling | ||
// immediately. If it significantly longer than 1 profile period to get | ||
// a profile, consider the test failed | ||
timeout := time.After(5 * time.Second) | ||
select { | ||
case <-timeout: | ||
t.Fatal("should have ") | ||
case <-received: | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We could "fix" this test by removing the
tick <- time.Now()
line and changingp.exit <- struct{}{}
toclose(p.exit)
. That would make the test pass. However I personally think this test should just be removed.