Send Less Client Reports When Rate Limited

CHANGELOG.md

@@ -24,6 +24,8 @@
   );
   ```
 
+- Send Less Client Reports When Rate Limited ([#2380](https://github.com/getsentry/sentry-dart/pull/2380))
+
 ### Enhancements
 
 - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365))

dart/lib/src/sentry_client.dart

@@ -20,6 +20,7 @@ import 'sentry_options.dart';
 import 'sentry_stack_trace_factory.dart';
 import 'sentry_trace_context_header.dart';
 import 'sentry_user_feedback.dart';
+import 'transport/client_report_transport.dart';
 import 'transport/data_category.dart';
 import 'transport/http_transport.dart';
 import 'transport/noop_transport.dart';
@@ -54,10 +55,16 @@ class SentryClient {
     if (options.sendClientReports) {
       options.recorder = ClientReportRecorder(options.clock);
     }
+    RateLimiter? rateLimiter;
     if (options.transport is NoOpTransport) {
-      final rateLimiter = RateLimiter(options);
+      rateLimiter = RateLimiter(options);
       options.transport = HttpTransport(options, rateLimiter);
     }
+    options.transport = ClientReportTransport(
+      rateLimiter,
+      options,
+      options.transport,
+    );
     // TODO: Web might change soon to use the JS SDK so we can remove it here later on
     final enableFlutterSpotlight = (options.spotlight.enabled &&
         (options.platformChecker.isWeb ||
@@ -427,7 +434,7 @@ class SentryClient {
 
   /// Reports the [envelope] to Sentry.io.
   Future<SentryId?> captureEnvelope(SentryEnvelope envelope) {
-    return _attachClientReportsAndSend(envelope);
+    return _options.transport.send(envelope);
   }
 
   /// Reports the [userFeedback] to Sentry.io.
@@ -439,7 +446,7 @@ class SentryClient {
       _options.sdk,
       dsn: _options.dsn,
     );
-    return _attachClientReportsAndSend(envelope);
+    return _options.transport.send(envelope);
   }
 
   /// Reports the [feedback] to Sentry.io.
@@ -469,7 +476,7 @@ class SentryClient {
       _options.sdk,
       dsn: _options.dsn,
     );
-    final id = await _attachClientReportsAndSend(envelope);
+    final id = await _options.transport.send(envelope);
     return id ?? SentryId.empty();
   }
 
@@ -621,10 +628,4 @@ class SentryClient {
     }
     return DataCategory.error;
   }
-
-  Future<SentryId?> _attachClientReportsAndSend(SentryEnvelope envelope) {
-    final clientReport = _options.recorder.flush();
-    envelope.addClientReport(clientReport);
-    return _options.transport.send(envelope);
-  }
 }

dart/lib/src/transport/client_report_transport.dart

@@ -0,0 +1,57 @@
+import 'package:meta/meta.dart';
+import '../../sentry.dart';
+import '../client_reports/client_report_recorder.dart';
+import '../sentry_envelope_header.dart';
+import 'rate_limiter.dart';
+
+/// Decorator that handles attaching of client reports in tandem with rate
+/// limiting. The rate limiter is optional.
+@internal
+class ClientReportTransport implements Transport {
+  final RateLimiter? _rateLimiter;
+  final SentryOptions _options;
+  final Transport _transport;
+
+  ClientReportTransport(this._rateLimiter, this._options, this._transport);
+
+  @visibleForTesting
+  get rateLimiter => _rateLimiter;
+
+  int _numberOfDroppedEnvelopes = 0;
+
+  @visibleForTesting
+  get numberOfDroppedEvents => _numberOfDroppedEnvelopes;
+
+  @override
+  Future<SentryId?> send(SentryEnvelope envelope) async {
+    final rateLimiter = _rateLimiter;
+
+    SentryEnvelope? filteredEnvelope = envelope;
+    if (rateLimiter != null) {
+      filteredEnvelope = rateLimiter.filter(envelope);
+    }
+    if (filteredEnvelope == null) {
+      _numberOfDroppedEnvelopes += 1;
+    }
+    if (_numberOfDroppedEnvelopes >= 10) {
+      // Create new envelope that could only contain client reports
+      filteredEnvelope = SentryEnvelope(
+        SentryEnvelopeHeader(SentryId.newId(), _options.sdk),
+        [],
+      );
+    }
+    if (filteredEnvelope == null) {
+      return SentryId.empty();
+    }
+    _numberOfDroppedEnvelopes = 0;
+
+    final clientReport = _options.recorder.flush();
+    filteredEnvelope.addClientReport(clientReport);
+
+    if (filteredEnvelope.items.isNotEmpty) {
+      return _transport.send(filteredEnvelope);
+    } else {
+      return SentryId.empty();
+    }
+  }
+}

dart/lib/src/transport/http_transport.dart

@@ -35,14 +35,16 @@ class HttpTransport implements Transport {
 
   @override
   Future<SentryId?> send(SentryEnvelope envelope) async {
-    final filteredEnvelope = _rateLimiter.filter(envelope);
-    if (filteredEnvelope == null) {
-      return SentryId.empty();
-    }
-    filteredEnvelope.header.sentAt = _options.clock();
+    // final filteredEnvelope = _rateLimiter.filter(envelope);
+    // if (filteredEnvelope == null) {
+    //   return SentryId.empty();
+    // }
+    // final clientReport = _options.recorder.flush();
+    // envelope.addClientReport(clientReport);
+
+    envelope.header.sentAt = _options.clock();
 
-    final streamedRequest =
-        await _requestHandler.createRequest(filteredEnvelope);
+    final streamedRequest = await _requestHandler.createRequest(envelope);
 
     final response = await _options.httpClient
         .send(streamedRequest)

dart/test/mocks/mock_transport.dart

@@ -16,6 +16,8 @@ class MockTransport implements Transport {
     return _calls;
   }
 
+  bool parseFromEnvelope = true;
+
   bool called(int calls) {
     return _calls == calls;
   }
@@ -28,7 +30,9 @@ class MockTransport implements Transport {
     // failure causes. Instead, we log them and check on access to [calls].
     try {
       envelopes.add(envelope);
-      await _eventFromEnvelope(envelope);
+      if (parseFromEnvelope) {
+        await _eventFromEnvelope(envelope);
+      }
     } catch (e, stack) {
       _exceptions += '$e\n$stack\n\n';
       rethrow;

dart/test/sentry_client_test.dart

@@ -4,22 +4,21 @@ import 'dart:typed_data';
 
 import 'package:collection/collection.dart';
 import 'package:sentry/sentry.dart';
-import 'package:sentry/src/client_reports/client_report.dart';
 import 'package:sentry/src/client_reports/discard_reason.dart';
-import 'package:sentry/src/client_reports/discarded_event.dart';
 import 'package:sentry/src/client_reports/noop_client_report_recorder.dart';
 import 'package:sentry/src/metrics/metric.dart';
 import 'package:sentry/src/sentry_item_type.dart';
 import 'package:sentry/src/sentry_stack_trace_factory.dart';
 import 'package:sentry/src/sentry_tracer.dart';
+import 'package:sentry/src/transport/client_report_transport.dart';
 import 'package:sentry/src/transport/data_category.dart';
+import 'package:sentry/src/transport/noop_transport.dart';
 import 'package:sentry/src/transport/spotlight_http_transport.dart';
 import 'package:sentry/src/utils/iterable_utils.dart';
 import 'package:test/test.dart';
 
 import 'mocks.dart';
 import 'mocks/mock_client_report_recorder.dart';
-import 'mocks/mock_envelope.dart';
 import 'mocks/mock_hub.dart';
 import 'mocks/mock_platform.dart';
 import 'mocks/mock_platform_checker.dart';
@@ -1600,106 +1599,74 @@ void main() {
     });
   });
 
-  group('ClientReportRecorder', () {
+  group('ClientReportTransport', () {
     late Fixture fixture;
 
     setUp(() {
       fixture = Fixture();
     });
 
-    test('recorder is not noop if client reports are enabled', () async {
-      fixture.options.sendClientReports = true;
-
+    test('set on options on init', () async {
       fixture.getSut(
         eventProcessor: DropAllEventProcessor(),
         provideMockRecorder: false,
       );
 
-      expect(fixture.options.recorder is NoOpClientReportRecorder, false);
-      expect(fixture.options.recorder is MockClientReportRecorder, false);
+      expect(fixture.options.transport is ClientReportTransport, true);
     });
 
-    test('recorder is noop if client reports are disabled', () {
-      fixture.options.sendClientReports = false;
-
+    test('has rateLimiter with http transport', () async {
       fixture.getSut(
         eventProcessor: DropAllEventProcessor(),
         provideMockRecorder: false,
+        transport: NoOpTransport(), // this will set http transport
       );
 
-      expect(fixture.options.recorder is NoOpClientReportRecorder, true);
+      expect(fixture.options.transport is ClientReportTransport, true);
+      final crt = fixture.options.transport as ClientReportTransport;
+      expect(crt.rateLimiter, isNotNull);
     });
 
-    test('captureEnvelope calls flush', () async {
-      final client = fixture.getSut(eventProcessor: DropAllEventProcessor());
-
-      final envelope = MockEnvelope();
-      envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())];
-
-      await client.captureEnvelope(envelope);
-
-      expect(fixture.recorder.flushCalled, true);
-    });
-
-    test('captureEnvelope adds client report', () async {
-      final clientReport = ClientReport(
-        DateTime(0),
-        [DiscardedEvent(DiscardReason.rateLimitBackoff, DataCategory.error, 1)],
+    test('does not have rateLimiter without http transport', () async {
+      fixture.getSut(
+        eventProcessor: DropAllEventProcessor(),
+        provideMockRecorder: false,
+        transport: MockTransport(),
       );
-      fixture.recorder.clientReport = clientReport;
-
-      final client = fixture.getSut(eventProcessor: DropAllEventProcessor());
 
-      final envelope = MockEnvelope();
-      envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())];
+      expect(fixture.options.transport is ClientReportTransport, true);
+      final crt = fixture.options.transport as ClientReportTransport;
+      expect(crt.rateLimiter, isNull);
+    });
+  });
 
-      await client.captureEnvelope(envelope);
+  group('ClientReportRecorder', () {
+    late Fixture fixture;
 
-      expect(envelope.clientReport, clientReport);
+    setUp(() {
+      fixture = Fixture();
     });
 
-    test('captureUserFeedback calls flush', () async {
-      final client = fixture.getSut(eventProcessor: DropAllEventProcessor());
+    test('recorder is not noop if client reports are enabled', () async {
+      fixture.options.sendClientReports = true;
 
-      final id = SentryId.newId();
-      // ignore: deprecated_member_use_from_same_package
-      final feedback = SentryUserFeedback(
-        eventId: id,
-        comments: 'this is awesome',
-        email: '[email protected]',
-        name: 'Rockstar Developer',
+      fixture.getSut(
+        eventProcessor: DropAllEventProcessor(),
+        provideMockRecorder: false,
       );
-      // ignore: deprecated_member_use_from_same_package
-      await client.captureUserFeedback(feedback);
 
-      expect(fixture.recorder.flushCalled, true);
+      expect(fixture.options.recorder is NoOpClientReportRecorder, false);
     });
 
-    test('captureUserFeedback adds client report', () async {
-      final clientReport = ClientReport(
-        DateTime(0),
-        [DiscardedEvent(DiscardReason.rateLimitBackoff, DataCategory.error, 1)],
-      );
-      fixture.recorder.clientReport = clientReport;
-
-      final client = fixture.getSut(eventProcessor: DropAllEventProcessor());
+    test('recorder is noop if client reports are disabled', () {
+      fixture.options.sendClientReports = false;
 
-      final id = SentryId.newId();
-      // ignore: deprecated_member_use_from_same_package
-      final feedback = SentryUserFeedback(
-        eventId: id,
-        comments: 'this is awesome',
-        email: '[email protected]',
-        name: 'Rockstar Developer',
+      fixture.getSut(
+        eventProcessor: DropAllEventProcessor(),
+        provideMockRecorder: false,
       );
-      // ignore: deprecated_member_use_from_same_package
-      await client.captureUserFeedback(feedback);
-
-      final envelope = fixture.transport.envelopes.first;
-      final item = envelope.items.last;
 
-      // Only partial test, as the envelope is created internally from feedback.
-      expect(item.header.type, SentryItemType.clientReport);
+      expect(fixture.options.recorder is NoOpClientReportRecorder, true);
     });
 
     test('record event processor dropping event', () async {
@@ -2255,14 +2222,8 @@ class Fixture {
     EventProcessor? eventProcessor,
     bool provideMockRecorder = true,
     bool debug = false,
+    Transport? transport,
   }) {
-    final hub = Hub(options);
-    _context = SentryTransactionContext(
-      'name',
-      'op',
-    );
-    tracer = SentryTracer(_context, hub);
-
     options.tracesSampleRate = 1.0;
     options.sendDefaultPii = sendDefaultPii;
     options.enableMetrics = enableMetrics;
@@ -2278,7 +2239,19 @@ class Fixture {
     if (eventProcessor != null) {
       options.addEventProcessor(eventProcessor);
     }
-    options.transport = transport;
+
+    // Internally also creates a SentryClient instance
+    final hub = Hub(options);
+    _context = SentryTransactionContext(
+      'name',
+      'op',
+    );
+    tracer = SentryTracer(_context, hub);
+
+    // Reset transport
+    options.transport = transport ?? this.transport;
+
+    // Again create SentryClient instance
     final client = SentryClient(options);
 
     if (provideMockRecorder) {