profiler: start collecting profiles immediately #1417
Conversation
Previously, it would take two minutes for the first profile to be uploaded. One minute for the internal time.Ticker to fire, and then an additional minute to collect the profiles for the first time. This means we wouldn't get profiles for the first minute of activity. Two tests had to be changed due to this: * TestStartStopIdempotency became significantly slower, because it would start 5000 profiles, and stopping each one involved waiting 200 ms for the CPU profile to stop. Changed to just collect the heap profile and to do fewer iterations. * TestProfilerInternal broke because it expected one round of profiling to happen after the "ticker" fired, but now one happens sooner so you see multiple profiles. It also broke because the test was supposed to close p.exit, not send to it. In general, the test is too tied to the specific implementation. TestAllUploaded covers all the same behavior and more, and didn't get broken by this implementation change. So I opted to just delete TestProfilerInternal.
| @@ -241,58 +243,6 @@ func TestStopLatency(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestProfilerInternal(t *testing.T) { | |||
There was a problem hiding this comment.
Note: We could "fix" this test by removing the tick <- time.Now() line and changing p.exit <- struct{}{} to close(p.exit). That would make the test pass. However I personally think this test should just be removed.
| @@ -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.
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.
I'm okay with this test. The alternative would be a test that uses an unstartedProfiler and calls the collect() method with a ticket that never fires and still expect a profile.
| @@ -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.
I'm okay with this test. The alternative would be a test that uses an unstartedProfiler and calls the collect() method with a ticket that never fires and still expect a profile.
Previously, it would take two minutes for the first profile to be
uploaded. One minute for the internal time.Ticker to fire, and then an
additional minute to collect the profiles for the first time. This means
we wouldn't get profiles for the first minute of activity.
Two tests had to be changed due to this:
start 5000 profiles, and stopping each one involved waiting 200 ms
for the CPU profile to stop. Changed to just collect the heap profile
and to do fewer iterations.
to happen after the "ticker" fired, but now one happens sooner so you
see multiple profiles. It also broke because the test was supposed to
close p.exit, not send to it. In general, the test is too tied to the
specific implementation. TestAllUploaded covers all the same behavior
and more, and didn't get broken by this implementation change. So I
opted to just delete TestProfilerInternal.