diff --git a/CHANGELOG.md b/CHANGELOG.md index ee0226a1c..fc28bfd3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Bug fixes + +- Fix an infinite loop in the profiler if a transaction's duration is longer than 30 seconds ([#725](https://github.com/getsentry/sentry-go/pull/725)) + ## 0.24.1 The Sentry SDK team is happy to announce the immediate availability of Sentry Go SDK v0.24.1. diff --git a/profiler.go b/profiler.go index 5517c11d1..c0b858cc1 100644 --- a/profiler.go +++ b/profiler.go @@ -236,18 +236,32 @@ func (p *profileRecorder) getBuckets(relativeStartNS, relativeEndNS uint64) (sam return 0, nil, nil } - // Search for the first item after the given startTime. - var start = end - samplesCount = 0 - buckets = make([]*profileSamplesBucket, 0, int64((relativeEndNS-relativeStartNS)/uint64(profilerSamplingRate.Nanoseconds()))+1) - for start.Value != nil { - var bucket = start.Value.(*profileSamplesBucket) - if bucket.relativeTimeNS < relativeStartNS { - break + { // Find the first item after the given startTime. + var start = end + var prevBucket *profileSamplesBucket + samplesCount = 0 + buckets = make([]*profileSamplesBucket, 0, int64((relativeEndNS-relativeStartNS)/uint64(profilerSamplingRate.Nanoseconds()))+1) + for start.Value != nil { + var bucket = start.Value.(*profileSamplesBucket) + + // If this bucket's time is before the requests start time, don't collect it (and stop iterating further). + if bucket.relativeTimeNS < relativeStartNS { + break + } + + // If this bucket time is greater than previous the bucket's time, we have exhausted the whole ring buffer + // before we were able to find the start time. That means the start time is not present and we must break. + // This happens if the slice duration exceeds the ring buffer capacity. + if prevBucket != nil && bucket.relativeTimeNS > prevBucket.relativeTimeNS { + break + } + + samplesCount += len(bucket.goIDs) + buckets = append(buckets, bucket) + + start = start.Prev() + prevBucket = bucket } - samplesCount += len(bucket.goIDs) - buckets = append(buckets, bucket) - start = start.Prev() } // Edge case - if the period requested was too short and we haven't collected enough samples. diff --git a/profiler_test.go b/profiler_test.go index b0d6ffd95..888c1a809 100644 --- a/profiler_test.go +++ b/profiler_test.go @@ -126,6 +126,53 @@ func TestProfilerCollection(t *testing.T) { }) } +func TestProfilerRingBufferOverflow(t *testing.T) { + if testing.Short() { + t.Skip("Skipping in short mode.") + } + + var require = require.New(t) + ticker := setupProfilerTestTicker(io.Discard) + defer restoreProfilerTicker() + + profiler := startProfiling(time.Now()) + defer profiler.Stop(true) + + // Skip a few ticks to emulate the profiler already running before a transaction starts + for i := 0; i < 100; i++ { + require.True(ticker.Tick()) + } + start := time.Now() + + // Emulate a transaction running for longer than the limit (30 seconds). + // The buffer should be 3030 items long, i.e. 30 seconds * 101 samples per second. + recorder := profiler.(*profileRecorder) + require.Equal(3030, recorder.samplesBucketsHead.Len()) + for i := 0; i < recorder.samplesBucketsHead.Len(); i++ { + require.True(ticker.Tick()) + } + + // Add a few more ticks after the transaction ends but prior collecting it. + // This emulates how the SDK normally behaves. + const ticksAfterEnd = 5 + end := time.Now() + for i := 0; i < ticksAfterEnd; i++ { + require.True(ticker.Tick()) + } + + result := profiler.GetSlice(start, end) + require.NotNil(result) + validateProfile(t, result.trace, end.Sub(start)) + + // Calculate the number of buckets (profiler internal representation). + // It should be the same as the length of the ring buffer minus ticksAfterEnd. + buckets := make(map[uint64]bool) + for _, sample := range result.trace.Samples { + buckets[sample.ElapsedSinceStartNS] = true + } + require.Equal(recorder.samplesBucketsHead.Len()-ticksAfterEnd, len(buckets)) +} + // Check the order of frames for a known stack trace (i.e. this test case). func TestProfilerStackTrace(t *testing.T) { var require = require.New(t)