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

* update refresh rate condition * update test * update log * Update CHANGELOG * Update CHANGELOG * update * fix tests * avoid print * update
getsentry · Sep 25, 2024 · bf8d36c · bf8d36c
1 parent b66cc27
commit bf8d36c
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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))
 

diff --git a/flutter/lib/src/span_frame_metrics_collector.dart b/flutter/lib/src/span_frame_metrics_collector.dart
@@ -26,10 +26,12 @@ class SpanFrameMetricsCollector implements PerformanceContinuousCollector {
   /// 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));
 
@@ -39,7 +41,8 @@ class SpanFrameMetricsCollector implements PerformanceContinuousCollector {
   bool get isTrackingRegistered => _isTrackingRegistered;
   bool _isTrackingRegistered = false;
 
-  int displayRefreshRate = 60;
+  @visibleForTesting
+  int? displayRefreshRate;
 
   final _stopwatch = Stopwatch();
 
@@ -59,25 +62,33 @@ class SpanFrameMetricsCollector implements PerformanceContinuousCollector {
     }
 
     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,
+          'Invalid display refresh rate. Skipping frame tracking for all active spans.');
+      clear();
+      return;
+    }
+
     final frameMetrics =
-        calculateFrameMetrics(span, endTimestamp, displayRefreshRate);
+        calculateFrameMetrics(span, endTimestamp, displayRefreshRate!);
     _applyFrameMetricsToSpan(span, frameMetrics);
 
     activeSpans.remove(span);
@@ -258,6 +269,6 @@ class SpanFrameMetricsCollector implements PerformanceContinuousCollector {
     _isTrackingPaused = true;
     frames.clear();
     activeSpans.clear();
-    displayRefreshRate = 60;
+    displayRefreshRate = null;
   }
 }
diff --git a/flutter/test/span_frame_metrics_collector_test.dart b/flutter/test/span_frame_metrics_collector_test.dart
@@ -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',
@@ -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);