Do not allow setting measurement if transaction is finished (#1026)

getsentry · Sep 29, 2022 · 4c2c7c7 · 4c2c7c7
1 parent a62d9aa
commit 4c2c7c7
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Tracer does not allow setting measurement if finished ([#1026](https://github.com/getsentry/sentry-dart/pull/1026))
+
 ## 6.11.1
 
 ### Fixes

diff --git a/dart/lib/src/protocol/sentry_span.dart b/dart/lib/src/protocol/sentry_span.dart
@@ -19,7 +19,7 @@ class SentrySpan extends ISentrySpan {
 
   SpanStatus? _status;
   final Map<String, String> _tags = {};
-  void Function({DateTime? endTimestamp})? _finishedCallback;
+  Function({DateTime? endTimestamp})? _finishedCallback;
 
   @override
   final SentryTracesSamplingDecision? samplingDecision;
@@ -62,7 +62,7 @@ class SentrySpan extends ISentrySpan {
     if (_throwable != null) {
       _hub.setSpanContext(_throwable, this, _tracer.name);
     }
-    _finishedCallback?.call(endTimestamp: _endTimestamp);
+    await _finishedCallback?.call(endTimestamp: _endTimestamp);
     return super.finish(status: status, endTimestamp: _endTimestamp);
   }
 

diff --git a/dart/lib/src/sentry_tracer.dart b/dart/lib/src/sentry_tracer.dart
@@ -106,8 +106,10 @@ class SentryTracer extends ISentrySpan {
         }
       }
 
-      await _rootSpan.finish(endTimestamp: _rootEndTimestamp);
+      // the callback should run before because if the span is finished,
+      // we cannot attach data, its immutable after being finished.
       await _onFinish?.call(this);
+      await _rootSpan.finish(endTimestamp: _rootEndTimestamp);
 
       // remove from scope
       await _hub.configureScope((scope) {
@@ -216,21 +218,23 @@ class SentryTracer extends ISentrySpan {
       _hub,
       samplingDecision: _rootSpan.samplingDecision,
       startTimestamp: startTimestamp,
-      finishedCallback: ({
-        DateTime? endTimestamp,
-      }) {
-        final finishStatus = _finishStatus;
-        if (finishStatus.finishing) {
-          finish(status: finishStatus.status, endTimestamp: endTimestamp);
-        }
-      },
+      finishedCallback: _finishedCallback,
     );
 
     _children.add(child);
 
     return child;
   }
 
+  Future<void> _finishedCallback({
+    DateTime? endTimestamp,
+  }) async {
+    final finishStatus = _finishStatus;
+    if (finishStatus.finishing) {
+      await finish(status: finishStatus.status, endTimestamp: endTimestamp);
+    }
+  }
+
   @override
   SpanStatus? get status => _rootSpan.status;
 
@@ -284,6 +288,9 @@ class SentryTracer extends ISentrySpan {
 
   @override
   void setMeasurement(String name, num value, {SentryMeasurementUnit? unit}) {
+    if (finished) {
+      return;
+    }
     final measurement = SentryMeasurement(name, value, unit: unit);
     _measurements[name] = measurement;
   }

diff --git a/dart/test/sentry_tracer_test.dart b/dart/test/sentry_tracer_test.dart
@@ -373,6 +373,37 @@ void main() {
 
       expect(sut.startChild('child3'), isA<NoOpSentrySpan>());
     });
+
+    test('tracer sets measurement', () async {
+      final sut = fixture.getSut();
+
+      sut.setMeasurement('key', 1.0);
+
+      expect(sut.measurements['key']!.value, 1.0);
+
+      await sut.finish();
+    });
+
+    test('tracer sets custom measurement unit', () async {
+      final sut = fixture.getSut();
+
+      sut.setMeasurement('key', 1.0, unit: SentryMeasurementUnit.hour);
+
+      expect(sut.measurements['key']!.value, 1.0);
+      expect(sut.measurements['key']?.unit, SentryMeasurementUnit.hour);
+
+      await sut.finish();
+    });
+
+    test('tracer does not allow setting measurement if finished', () async {
+      final sut = fixture.getSut();
+
+      await sut.finish();
+
+      sut.setMeasurement('key', 1.0);
+
+      expect(sut.measurements.isEmpty, true);
+    });
   });
 
   group('$SentryBaggageHeader', () {

diff --git a/flutter/lib/src/navigation/sentry_navigator_observer.dart b/flutter/lib/src/navigation/sentry_navigator_observer.dart
@@ -1,5 +1,3 @@
-// ignore: implementation_imports
-import 'package:sentry/src/sentry_tracer.dart';
 import 'package:flutter/widgets.dart';
 
 import '../../sentry_flutter.dart';
@@ -181,20 +179,17 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
       autoFinishAfter: _autoFinishAfter,
       trimEnd: true,
       onFinish: (transaction) async {
-        // ignore: invalid_use_of_internal_member
-        if (transaction is SentryTracer) {
-          final nativeFrames = await _native
-              .endNativeFramesCollection(transaction.context.traceId);
-          if (nativeFrames != null) {
-            final measurements = nativeFrames.toMeasurements();
-            for (final item in measurements.entries) {
-              final measurement = item.value;
-              transaction.setMeasurement(
-                item.key,
-                measurement.value,
-                unit: measurement.unit,
-              );
-            }
+        final nativeFrames = await _native
+            .endNativeFramesCollection(transaction.context.traceId);
+        if (nativeFrames != null) {
+          final measurements = nativeFrames.toMeasurements();
+          for (final item in measurements.entries) {
+            final measurement = item.value;
+            transaction.setMeasurement(
+              item.key,
+              measurement.value,
+              unit: measurement.unit,
+            );
           }
         }
       },