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

fix: only start frame tracking if we receive valid refresh data #2307

Merged
merged 12 commits into from
Sep 25, 2024
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

### Fixes

-- Incoming Change
- Only start frame tracking if we receive valid display refresh data ([#2307](https://github.com/getsentry/sentry-dart/pull/2307))
- Rounding error used on frames.total and reject frame measurements if frames.total is less than frames.slow or frames.frozen ([#2308](https://github.com/getsentry/sentry-dart/pull/2308))
- iOS replay integration when only `onErrorSampleRate` is specified ([#2306](https://github.com/getsentry/sentry-dart/pull/2306))

Expand Down
27 changes: 19 additions & 8 deletions flutter/lib/src/span_frame_metrics_collector.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@
/// Stores frame timestamps and their durations in milliseconds.
/// Keys are frame timestamps, values are frame durations.
/// The timestamps mark the end of the frame.
@visibleForTesting
final frames = SplayTreeMap<DateTime, int>();

/// Stores the spans that are actively being tracked.
/// After the frames are calculated and stored in the span the span is removed from this list.
@visibleForTesting
final activeSpans = SplayTreeSet<ISentrySpan>(
(a, b) => a.startTimestamp.compareTo(b.startTimestamp));

Expand All @@ -39,7 +41,8 @@
bool get isTrackingRegistered => _isTrackingRegistered;
bool _isTrackingRegistered = false;

int displayRefreshRate = 60;
@visibleForTesting
int? displayRefreshRate;

final _stopwatch = Stopwatch();

Expand All @@ -59,25 +62,33 @@
}

final fetchedDisplayRefreshRate = await _native?.displayRefreshRate();
if (fetchedDisplayRefreshRate != null) {
if (fetchedDisplayRefreshRate != null && fetchedDisplayRefreshRate > 0) {
options.logger(SentryLevel.debug,
'Retrieved display refresh rate at $fetchedDisplayRefreshRate');
displayRefreshRate = fetchedDisplayRefreshRate;

// Start tracking frames only when refresh rate is valid
activeSpans.add(span);
startFrameTracking();
} else {
options.logger(SentryLevel.debug,
'Could not fetch display refresh rate, keeping at 60hz by default');
'Retrieved invalid display refresh rate: $fetchedDisplayRefreshRate. Not starting frame tracking.');
}

activeSpans.add(span);
startFrameTracking();
}

@override
Future<void> onSpanFinished(ISentrySpan span, DateTime endTimestamp) async {
if (span is NoOpSentrySpan || !activeSpans.contains(span)) return;

if (displayRefreshRate == null || displayRefreshRate! <= 0) {
options.logger(SentryLevel.warning,

Check warning on line 84 in flutter/lib/src/span_frame_metrics_collector.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/span_frame_metrics_collector.dart#L84

Added line #L84 was not covered by tests
'Invalid display refresh rate. Skipping frame tracking for all active spans.');
clear();

Check warning on line 86 in flutter/lib/src/span_frame_metrics_collector.dart

View check run for this annotation

Codecov / codecov/patch

flutter/lib/src/span_frame_metrics_collector.dart#L86

Added line #L86 was not covered by tests
return;
}

final frameMetrics =
calculateFrameMetrics(span, endTimestamp, displayRefreshRate);
calculateFrameMetrics(span, endTimestamp, displayRefreshRate!);
_applyFrameMetricsToSpan(span, frameMetrics);

activeSpans.remove(span);
Expand Down Expand Up @@ -258,6 +269,6 @@
_isTrackingPaused = true;
frames.clear();
activeSpans.clear();
displayRefreshRate = 60;
displayRefreshRate = null;
}
}
34 changes: 18 additions & 16 deletions flutter/test/span_frame_metrics_collector_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,31 +47,32 @@ void main() {
expect(sut.isTrackingRegistered, isFalse);
});

test(
'captures metrics with display refresh rate of 60 if native refresh rate is null',
() async {
test('does not start frame tracking if native refresh rate is null', () {
final sut = fixture.sut;
fixture.options.tracesSampleRate = 1.0;
fixture.options.addPerformanceCollector(sut);
final startTimestamp = DateTime.now();
final endTimestamp =
startTimestamp.add(Duration(milliseconds: 1000)).toUtc();

when(fixture.mockSentryNative.displayRefreshRate())
.thenAnswer((_) async => null);

final tracer = SentryTracer(
SentryTransactionContext('name', 'op', description: 'tracerDesc'),
fixture.hub,
startTimestamp: startTimestamp);
final span = MockSentrySpan();
sut.onSpanStarted(span);

await Future<void>.delayed(Duration(milliseconds: 500));
await tracer.finish(endTimestamp: endTimestamp);
expect(sut.isTrackingRegistered, isFalse);
});

expect(tracer.data['frames.slow'], expectedSlowFrames);
expect(tracer.data['frames.frozen'], expectedFrozenFrames);
expect(tracer.data['frames.delay'], expectedFramesDelay);
expect(tracer.data['frames.total'], expectedTotalFrames);
test('does not start frame tracking if native refresh rate is 0', () {
final sut = fixture.sut;
fixture.options.tracesSampleRate = 1.0;
fixture.options.addPerformanceCollector(sut);

when(fixture.mockSentryNative.displayRefreshRate())
.thenAnswer((_) async => 0);

final span = MockSentrySpan();
sut.onSpanStarted(span);

expect(sut.isTrackingRegistered, isFalse);
});

test('onSpanFinished removes frames older than span start timestamp',
Expand All @@ -83,6 +84,7 @@ void main() {
final span2 = MockSentrySpan();
final spanStartTimestamp = DateTime.now();
final spanEndTimestamp = spanStartTimestamp.add(Duration(seconds: 1));
sut.displayRefreshRate = 60;

when(span1.isRootSpan).thenReturn(false);
when(span1.startTimestamp).thenReturn(spanStartTimestamp);
Expand Down
Loading