diff --git a/CHANGELOG.md b/CHANGELOG.md index 9260793a2..ac746bec6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ ### Enhancements +- Avoid sending too many empty client reports when Http Transport is used ([#2380](https://github.com/getsentry/sentry-dart/pull/2380)) - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365)) - Handle backpressure earlier in pipeline ([#2371](https://github.com/getsentry/sentry-dart/pull/2371)) - Drops max un-awaited parallel tasks earlier, so event processors & callbacks are not executed for them. diff --git a/dart/lib/src/sentry_client.dart b/dart/lib/src/sentry_client.dart index 0ff4f49af..911e639b6 100644 --- a/dart/lib/src/sentry_client.dart +++ b/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,17 @@ 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); } + // rateLimiter is null if FileSystemTransport is active since Native SDKs take care of rate limiting + 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 +435,7 @@ class SentryClient { /// Reports the [envelope] to Sentry.io. Future captureEnvelope(SentryEnvelope envelope) { - return _attachClientReportsAndSend(envelope); + return _options.transport.send(envelope); } /// Reports the [userFeedback] to Sentry.io. @@ -439,7 +447,7 @@ class SentryClient { _options.sdk, dsn: _options.dsn, ); - return _attachClientReportsAndSend(envelope); + return _options.transport.send(envelope); } /// Reports the [feedback] to Sentry.io. @@ -469,7 +477,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 +629,4 @@ class SentryClient { } return DataCategory.error; } - - Future _attachClientReportsAndSend(SentryEnvelope envelope) { - final clientReport = _options.recorder.flush(); - envelope.addClientReport(clientReport); - return _options.transport.send(envelope); - } } diff --git a/dart/lib/src/transport/client_report_transport.dart b/dart/lib/src/transport/client_report_transport.dart new file mode 100644 index 000000000..0ff59fad4 --- /dev/null +++ b/dart/lib/src/transport/client_report_transport.dart @@ -0,0 +1,56 @@ +import 'package:meta/meta.dart'; +import '../../sentry.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 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(); + } + } +} diff --git a/dart/lib/src/transport/http_transport.dart b/dart/lib/src/transport/http_transport.dart index cf1e375f0..45d201f79 100644 --- a/dart/lib/src/transport/http_transport.dart +++ b/dart/lib/src/transport/http_transport.dart @@ -35,14 +35,9 @@ class HttpTransport implements Transport { @override Future send(SentryEnvelope envelope) async { - final filteredEnvelope = _rateLimiter.filter(envelope); - if (filteredEnvelope == null) { - return SentryId.empty(); - } - filteredEnvelope.header.sentAt = _options.clock(); + envelope.header.sentAt = _options.clock(); - final streamedRequest = - await _requestHandler.createRequest(filteredEnvelope); + final streamedRequest = await _requestHandler.createRequest(envelope); final response = await _options.httpClient .send(streamedRequest) diff --git a/dart/test/exception_identifier_test.dart b/dart/test/exception_identifier_test.dart index 29e2a4a73..382a90b5d 100644 --- a/dart/test/exception_identifier_test.dart +++ b/dart/test/exception_identifier_test.dart @@ -126,7 +126,7 @@ void main() { group('Integration test', () { setUp(() { - fixture.options.transport = MockTransport(); + fixture.options.transport = fixture.mockTransport; }); test( @@ -139,7 +139,7 @@ void main() { await client.captureException(ObfuscatedException()); - final transport = fixture.options.transport as MockTransport; + final transport = fixture.mockTransport; final capturedEnvelope = transport.envelopes.first; final capturedEvent = await eventFromEnvelope(capturedEnvelope); @@ -154,7 +154,7 @@ void main() { await client.captureException(ObfuscatedException()); - final transport = fixture.options.transport as MockTransport; + final transport = fixture.mockTransport; final capturedEnvelope = transport.envelopes.first; final capturedEvent = await eventFromEnvelope(capturedEnvelope); @@ -165,6 +165,7 @@ void main() { } class Fixture { + final mockTransport = MockTransport(); SentryOptions options = defaultTestOptions(); } diff --git a/dart/test/mocks/mock_transport.dart b/dart/test/mocks/mock_transport.dart index 55ae027f2..024fd7084 100644 --- a/dart/test/mocks/mock_transport.dart +++ b/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; diff --git a/dart/test/sentry_client_test.dart b/dart/test/sentry_client_test.dart index a8421039a..ccc1b4668 100644 --- a/dart/test/sentry_client_test.dart +++ b/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: 'sentry@example.com', - 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: 'sentry@example.com', - 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) { diff --git a/dart/test/transport/client_report_transport_test.dart b/dart/test/transport/client_report_transport_test.dart new file mode 100644 index 000000000..df9447c76 --- /dev/null +++ b/dart/test/transport/client_report_transport_test.dart @@ -0,0 +1,169 @@ +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/sentry_item_type.dart'; +import 'package:sentry/src/transport/client_report_transport.dart'; +import 'package:sentry/src/transport/data_category.dart'; +import 'package:sentry/src/transport/rate_limiter.dart'; +import 'package:test/test.dart'; + +import '../mocks.dart'; +import '../mocks/mock_client_report_recorder.dart'; +import '../mocks/mock_envelope.dart'; +import '../mocks/mock_transport.dart'; +import '../test_utils.dart'; + +void main() { + group('filter', () { + late Fixture fixture; + + setUp(() { + fixture = Fixture(); + }); + + test('filter called', () async { + final mockRateLimiter = MockRateLimiter(); + final sut = fixture.getSut(rateLimiter: mockRateLimiter); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + await sut.send(envelope); + + expect(mockRateLimiter.envelopeToFilter, envelope); + expect(fixture.mockTransport.envelopes.first, envelope); + }); + + test('send nothing when filtered event null', () async { + final mockRateLimiter = MockRateLimiter()..filterReturnsNull = true; + final sut = fixture.getSut(rateLimiter: mockRateLimiter); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + final eventId = await sut.send(envelope); + + expect(eventId, SentryId.empty()); + expect(fixture.mockTransport.called(0), true); + }); + }); + + group('client reports', () { + late Fixture fixture; + + setUp(() { + fixture = Fixture(); + }); + + test('send calls flush', () async { + final sut = fixture.getSut(); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + await sut.send(envelope); + + expect(fixture.recorder.flushCalled, true); + }); + + test('send adds client report', () async { + final clientReport = ClientReport( + DateTime(0), + [DiscardedEvent(DiscardReason.rateLimitBackoff, DataCategory.error, 1)], + ); + fixture.recorder.clientReport = clientReport; + + final sut = fixture.getSut(); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + await sut.send(envelope); + + expect(envelope.clientReport, clientReport); + }); + }); + + group('client report only event', () { + late Fixture fixture; + + setUp(() { + fixture = Fixture(); + }); + + test('send after filtering out 10 times and client report', () async { + final clientReport = ClientReport( + DateTime(0), + [DiscardedEvent(DiscardReason.rateLimitBackoff, DataCategory.error, 1)], + ); + fixture.recorder.clientReport = clientReport; + + final mockRateLimiter = MockRateLimiter()..filterReturnsNull = true; + + final sut = fixture.getSut(rateLimiter: mockRateLimiter); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + for (int i = 0; i < 10; i++) { + await sut.send(envelope); + } + + expect(fixture.mockTransport.called(1), true); + + final sentEnvelope = fixture.mockTransport.envelopes.first; + expect(sentEnvelope.items.length, 1); + expect(sentEnvelope.items[0].header.type, SentryItemType.clientReport); + }); + + test('filter out after 10 times with no client reports', () async { + final mockRateLimiter = MockRateLimiter()..filterReturnsNull = true; + + final sut = fixture.getSut(rateLimiter: mockRateLimiter); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + for (int i = 0; i < 10; i++) { + await sut.send(envelope); + } + + expect(fixture.mockTransport.called(0), true); + }); + + test('reset counter', () async { + final clientReport = ClientReport( + DateTime(0), + [DiscardedEvent(DiscardReason.rateLimitBackoff, DataCategory.error, 1)], + ); + fixture.recorder.clientReport = clientReport; + + final mockRateLimiter = MockRateLimiter()..filterReturnsNull = true; + + final sut = fixture.getSut(rateLimiter: mockRateLimiter); + + final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + + for (int i = 0; i < 20; i++) { + await sut.send(envelope); + } + + expect(fixture.mockTransport.called(2), true); + }); + }); +} + +class Fixture { + final options = defaultTestOptions(); + + late var recorder = MockClientReportRecorder(); + late var mockTransport = MockTransport(); + + ClientReportTransport getSut({RateLimiter? rateLimiter}) { + mockTransport.parseFromEnvelope = false; + options.recorder = recorder; + return ClientReportTransport(rateLimiter, options, mockTransport); + } +} diff --git a/dart/test/transport/http_transport_test.dart b/dart/test/transport/http_transport_test.dart index 6546cd73e..7da9a568a 100644 --- a/dart/test/transport/http_transport_test.dart +++ b/dart/test/transport/http_transport_test.dart @@ -1,13 +1,8 @@ -import 'dart:convert'; - import 'package:collection/collection.dart'; import 'package:http/http.dart' as http; import 'package:http/testing.dart'; import 'package:sentry/sentry.dart'; import 'package:sentry/src/client_reports/discard_reason.dart'; -import 'package:sentry/src/sentry_envelope_header.dart'; -import 'package:sentry/src/sentry_envelope_item_header.dart'; -import 'package:sentry/src/sentry_item_type.dart'; import 'package:sentry/src/sentry_tracer.dart'; import 'package:sentry/src/transport/data_category.dart'; import 'package:sentry/src/transport/http_transport.dart'; @@ -20,42 +15,14 @@ import '../mocks/mock_hub.dart'; import '../test_utils.dart'; void main() { - SentryEnvelope givenEnvelope() { - final filteredEnvelopeHeader = SentryEnvelopeHeader(SentryId.empty(), null); - final filteredItemHeader = - SentryEnvelopeItemHeader(SentryItemType.event, () async { - return 2; - }, contentType: 'application/json'); - final dataFactory = () async { - return utf8.encode('{}'); - }; - final filteredItem = SentryEnvelopeItem(filteredItemHeader, dataFactory); - return SentryEnvelope(filteredEnvelopeHeader, [filteredItem]); - } - - group('filter', () { + group('send', () { late Fixture fixture; setUp(() { fixture = Fixture(); }); - test('filter called', () async { - final httpMock = MockClient((http.Request request) async { - return http.Response('{}', 200); - }); - - fixture.options.compressPayload = false; - final mockRateLimiter = MockRateLimiter(); - final sut = fixture.getSut(httpMock, mockRateLimiter); - - final sentryEnvelope = givenEnvelope(); - await sut.send(sentryEnvelope); - - expect(mockRateLimiter.envelopeToFilter, sentryEnvelope); - }); - - test('send filtered event', () async { + test('event with http client', () async { List? body; final httpMock = MockClient((http.Request request) async { @@ -63,11 +30,9 @@ void main() { return http.Response('{}', 200); }); - final filteredEnvelope = givenEnvelope(); - fixture.options.compressPayload = false; - final mockRateLimiter = MockRateLimiter() - ..filteredEnvelope = filteredEnvelope; + final mockRateLimiter = MockRateLimiter(); + final sut = fixture.getSut(httpMock, mockRateLimiter); final sentryEvent = SentryEvent(); @@ -79,35 +44,12 @@ void main() { await sut.send(envelope); final envelopeData = []; - await filteredEnvelope + await envelope .envelopeStream(fixture.options) .forEach(envelopeData.addAll); expect(body, envelopeData); }); - - test('send nothing when filtered event null', () async { - var httpCalled = false; - final httpMock = MockClient((http.Request request) async { - httpCalled = true; - return http.Response('{}', 200); - }); - - fixture.options.compressPayload = false; - final mockRateLimiter = MockRateLimiter()..filterReturnsNull = true; - final sut = fixture.getSut(httpMock, mockRateLimiter); - - final sentryEvent = SentryEvent(); - final envelope = SentryEnvelope.fromEvent( - sentryEvent, - fixture.options.sdk, - dsn: fixture.options.dsn, - ); - final eventId = await sut.send(envelope); - - expect(eventId, SentryId.empty()); - expect(httpCalled, false); - }); }); group('updateRetryAfterLimits', () { @@ -130,6 +72,9 @@ void main() { fixture.options.sdk, dsn: fixture.options.dsn, ); + + mockRateLimiter.filter(envelope); + await sut.send(envelope); expect(mockRateLimiter.envelopeToFilter?.header.eventId,