From 34e479445308dd76ff3cb517625596bef3ffaa71 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 30 Oct 2024 15:27:08 +0100 Subject: [PATCH 01/11] Send Less Client Reports When Rate Limited --- dart/lib/src/sentry_client.dart | 18 ++- .../transport/client_report_transport.dart | 41 ++++++ dart/lib/src/transport/http_transport.dart | 18 ++- dart/test/mocks/mock_transport.dart | 6 +- dart/test/sentry_client_test.dart | 127 +++++++----------- .../client_report_transport_test.dart | 94 +++++++++++++ dart/test/transport/http_transport_test.dart | 71 ++-------- 7 files changed, 217 insertions(+), 158 deletions(-) create mode 100644 dart/lib/src/transport/client_report_transport.dart create mode 100644 dart/test/transport/client_report_transport_test.dart diff --git a/dart/lib/src/sentry_client.dart b/dart/lib/src/sentry_client.dart index 0ff4f49af7..c5aa5c1b66 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,13 @@ 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.recorder, 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 +431,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 +443,7 @@ class SentryClient { _options.sdk, dsn: _options.dsn, ); - return _attachClientReportsAndSend(envelope); + return _options.transport.send(envelope); } /// Reports the [feedback] to Sentry.io. @@ -469,7 +473,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 +625,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 0000000000..6dbf6d5a57 --- /dev/null +++ b/dart/lib/src/transport/client_report_transport.dart @@ -0,0 +1,41 @@ +import 'package:meta/meta.dart'; +import '../../sentry.dart'; +import '../client_reports/client_report_recorder.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 ClientReportRecorder _recorder; + + final Transport _transport; + + ClientReportTransport(this._rateLimiter, this._recorder, this._transport); + + @visibleForTesting + get rateLimiter => _rateLimiter; + + @override + Future send(SentryEnvelope envelope) async { + final rateLimiter = _rateLimiter; + + SentryEnvelope? filteredEnvelope; + if (rateLimiter != null) { + filteredEnvelope = rateLimiter.filter(envelope); + // TODO handle case where we send a client reports anyway if we were rate limited too many times + } else { + filteredEnvelope = envelope; + } + + if (filteredEnvelope == null) { + return SentryId.empty(); + } + + final clientReport = _recorder.flush(); + envelope.addClientReport(clientReport); + + return _transport.send(envelope); + } +} diff --git a/dart/lib/src/transport/http_transport.dart b/dart/lib/src/transport/http_transport.dart index cf1e375f07..c7fece270e 100644 --- a/dart/lib/src/transport/http_transport.dart +++ b/dart/lib/src/transport/http_transport.dart @@ -2,6 +2,8 @@ import 'dart:async'; import 'dart:convert'; import 'package:http/http.dart'; +import 'package:meta/meta.dart'; +import '../client_reports/client_report_recorder.dart'; import '../utils/transport_utils.dart'; import 'http_transport_request_handler.dart'; @@ -35,14 +37,16 @@ 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(); + // 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) diff --git a/dart/test/mocks/mock_transport.dart b/dart/test/mocks/mock_transport.dart index 55ae027f21..024fd70843 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 a8421039a3..ccc1b46681 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 0000000000..a624ac23dc --- /dev/null +++ b/dart/test/transport/client_report_transport_test.dart @@ -0,0 +1,94 @@ +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/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(); + 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(); + 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); + }); + }); +} + +class Fixture { + final options = defaultTestOptions(); + + late var recorder = MockClientReportRecorder(); + late var mockTransport = MockTransport(); + + ClientReportTransport getSut({RateLimiter? rateLimiter}) { + mockTransport.parseFromEnvelope = false; + return ClientReportTransport(rateLimiter, recorder, mockTransport); + } +} diff --git a/dart/test/transport/http_transport_test.dart b/dart/test/transport/http_transport_test.dart index 6546cd73eb..7da9a568a1 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, From 860c2c85678ba30da02d895356717578e6d5f199 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 30 Oct 2024 15:27:57 +0100 Subject: [PATCH 02/11] dart fix --- dart/lib/src/transport/http_transport.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/dart/lib/src/transport/http_transport.dart b/dart/lib/src/transport/http_transport.dart index c7fece270e..e386e7d9a6 100644 --- a/dart/lib/src/transport/http_transport.dart +++ b/dart/lib/src/transport/http_transport.dart @@ -2,8 +2,6 @@ import 'dart:async'; import 'dart:convert'; import 'package:http/http.dart'; -import 'package:meta/meta.dart'; -import '../client_reports/client_report_recorder.dart'; import '../utils/transport_utils.dart'; import 'http_transport_request_handler.dart'; From 59779114b17e75487f7cbb3c9c119016255aed8a Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 30 Oct 2024 16:15:54 +0100 Subject: [PATCH 03/11] try to send client reports only after getting fully rate limited 10 times --- dart/lib/src/sentry_client.dart | 7 +- .../transport/client_report_transport.dart | 38 +++++++--- .../client_report_transport_test.dart | 73 ++++++++++++++++++- 3 files changed, 104 insertions(+), 14 deletions(-) diff --git a/dart/lib/src/sentry_client.dart b/dart/lib/src/sentry_client.dart index c5aa5c1b66..3fac13d6b8 100644 --- a/dart/lib/src/sentry_client.dart +++ b/dart/lib/src/sentry_client.dart @@ -60,8 +60,11 @@ class SentryClient { rateLimiter = RateLimiter(options); options.transport = HttpTransport(options, rateLimiter); } - options.transport = - ClientReportTransport(rateLimiter, options.recorder, options.transport); + 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 || diff --git a/dart/lib/src/transport/client_report_transport.dart b/dart/lib/src/transport/client_report_transport.dart index 6dbf6d5a57..afbaff3ebd 100644 --- a/dart/lib/src/transport/client_report_transport.dart +++ b/dart/lib/src/transport/client_report_transport.dart @@ -1,6 +1,7 @@ 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 @@ -8,34 +9,49 @@ import 'rate_limiter.dart'; @internal class ClientReportTransport implements Transport { final RateLimiter? _rateLimiter; - final ClientReportRecorder _recorder; - + final SentryOptions _options; final Transport _transport; - ClientReportTransport(this._rateLimiter, this._recorder, this._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; + SentryEnvelope? filteredEnvelope = envelope; if (rateLimiter != null) { filteredEnvelope = rateLimiter.filter(envelope); - // TODO handle case where we send a client reports anyway if we were rate limited too many times - } else { - filteredEnvelope = 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 = _recorder.flush(); - envelope.addClientReport(clientReport); + final clientReport = _options.recorder.flush(); + filteredEnvelope.addClientReport(clientReport); - return _transport.send(envelope); + if (filteredEnvelope.items.isNotEmpty) { + return _transport.send(filteredEnvelope); + } else { + return SentryId.empty(); + } } } diff --git a/dart/test/transport/client_report_transport_test.dart b/dart/test/transport/client_report_transport_test.dart index a624ac23dc..378baaa555 100644 --- a/dart/test/transport/client_report_transport_test.dart +++ b/dart/test/transport/client_report_transport_test.dart @@ -2,6 +2,7 @@ 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'; @@ -79,6 +80,75 @@ void main() { 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 { @@ -89,6 +159,7 @@ class Fixture { ClientReportTransport getSut({RateLimiter? rateLimiter}) { mockTransport.parseFromEnvelope = false; - return ClientReportTransport(rateLimiter, recorder, mockTransport); + options.recorder = recorder; + return ClientReportTransport(rateLimiter, options, mockTransport); } } From f2a276fa0c037dc972278181d62efeeb525fcce1 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 30 Oct 2024 16:17:43 +0100 Subject: [PATCH 04/11] add cl entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9260793a20..5b8c213703 100644 --- a/CHANGELOG.md +++ b/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)) From 0c1983bc17b3ef55ab19cf3e6198f857fef0328b Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Mon, 4 Nov 2024 11:28:21 +0100 Subject: [PATCH 05/11] remove unused code --- dart/lib/src/transport/http_transport.dart | 7 ------- 1 file changed, 7 deletions(-) diff --git a/dart/lib/src/transport/http_transport.dart b/dart/lib/src/transport/http_transport.dart index e386e7d9a6..45d201f791 100644 --- a/dart/lib/src/transport/http_transport.dart +++ b/dart/lib/src/transport/http_transport.dart @@ -35,13 +35,6 @@ class HttpTransport implements Transport { @override Future send(SentryEnvelope envelope) async { - // 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(envelope); From 9a345d1e7c9518e83241a06df3c9f1239f07e169 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Mon, 4 Nov 2024 11:39:39 +0100 Subject: [PATCH 06/11] fix tests --- dart/lib/src/transport/client_report_transport.dart | 1 - dart/test/transport/client_report_transport_test.dart | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dart/lib/src/transport/client_report_transport.dart b/dart/lib/src/transport/client_report_transport.dart index afbaff3ebd..0ff59fad4e 100644 --- a/dart/lib/src/transport/client_report_transport.dart +++ b/dart/lib/src/transport/client_report_transport.dart @@ -1,6 +1,5 @@ import 'package:meta/meta.dart'; import '../../sentry.dart'; -import '../client_reports/client_report_recorder.dart'; import '../sentry_envelope_header.dart'; import 'rate_limiter.dart'; diff --git a/dart/test/transport/client_report_transport_test.dart b/dart/test/transport/client_report_transport_test.dart index 378baaa555..df9447c768 100644 --- a/dart/test/transport/client_report_transport_test.dart +++ b/dart/test/transport/client_report_transport_test.dart @@ -27,6 +27,8 @@ void main() { final sut = fixture.getSut(rateLimiter: mockRateLimiter); final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + await sut.send(envelope); expect(mockRateLimiter.envelopeToFilter, envelope); @@ -38,6 +40,8 @@ void main() { final sut = fixture.getSut(rateLimiter: mockRateLimiter); final envelope = MockEnvelope(); + envelope.items = [SentryEnvelopeItem.fromEvent(SentryEvent())]; + final eventId = await sut.send(envelope); expect(eventId, SentryId.empty()); From c7f254c9ea3da8909eef1220daa02c79b53bf5ff Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Mon, 4 Nov 2024 11:58:33 +0100 Subject: [PATCH 07/11] use mock transport in test --- dart/test/exception_identifier_test.dart | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dart/test/exception_identifier_test.dart b/dart/test/exception_identifier_test.dart index 29e2a4a735..382a90b5d8 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(); } From c55da5eae0b9bf5b39f647f68a885ee955917000 Mon Sep 17 00:00:00 2001 From: Giancarlo Buenaflor Date: Thu, 7 Nov 2024 14:18:49 +0000 Subject: [PATCH 08/11] Update sentry_client.dart --- dart/lib/src/sentry_client.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dart/lib/src/sentry_client.dart b/dart/lib/src/sentry_client.dart index 3fac13d6b8..5daa2b5a84 100644 --- a/dart/lib/src/sentry_client.dart +++ b/dart/lib/src/sentry_client.dart @@ -60,6 +60,8 @@ class SentryClient { rateLimiter = RateLimiter(options); options.transport = HttpTransport(options, rateLimiter); } + // rateLimiter is null if FileSystemTransport is active + // Native SDKs take care of rate limiting options.transport = ClientReportTransport( rateLimiter, options, From 1d0b702ce3edb2f50614eb48d9d6becc19c347a3 Mon Sep 17 00:00:00 2001 From: Giancarlo Buenaflor Date: Thu, 7 Nov 2024 14:19:35 +0000 Subject: [PATCH 09/11] Update sentry_client.dart --- dart/lib/src/sentry_client.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dart/lib/src/sentry_client.dart b/dart/lib/src/sentry_client.dart index 5daa2b5a84..911e639b65 100644 --- a/dart/lib/src/sentry_client.dart +++ b/dart/lib/src/sentry_client.dart @@ -60,8 +60,7 @@ class SentryClient { rateLimiter = RateLimiter(options); options.transport = HttpTransport(options, rateLimiter); } - // rateLimiter is null if FileSystemTransport is active - // Native SDKs take care of rate limiting + // rateLimiter is null if FileSystemTransport is active since Native SDKs take care of rate limiting options.transport = ClientReportTransport( rateLimiter, options, From 7099874c2d0548d8183c1086e8854170e05e0c10 Mon Sep 17 00:00:00 2001 From: Giancarlo Buenaflor Date: Thu, 7 Nov 2024 15:22:16 +0000 Subject: [PATCH 10/11] Update CHANGELOG.md --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b8c213703..ab4ae76346 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,10 +24,9 @@ ); ``` -- Send Less Client Reports When Rate Limited ([#2380](https://github.com/getsentry/sentry-dart/pull/2380)) - ### Enhancements +- Avoid sending 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. From d60170351993784c71df149fa5ad0aee07603ec0 Mon Sep 17 00:00:00 2001 From: Giancarlo Buenaflor Date: Thu, 7 Nov 2024 15:22:39 +0000 Subject: [PATCH 11/11] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab4ae76346..ac746bec6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ ### Enhancements -- Avoid sending empty client reports when Http Transport is used ([#2380](https://github.com/getsentry/sentry-dart/pull/2380)) +- 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.