Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6a5d2c3
hub interface & instanciation
rxlabz Oct 15, 2020
4f01775
- hub capture event
rxlabz Oct 15, 2020
ea5c55f
- hub capture exception
rxlabz Oct 15, 2020
bd09da0
hub captureMessage
rxlabz Oct 15, 2020
c5317a4
fix some tests
rxlabz Oct 15, 2020
6a8910e
fix more tests
rxlabz Oct 15, 2020
f5d3dd0
changelog
rxlabz Oct 15, 2020
f102ac3
fix all tests
rxlabz Oct 15, 2020
055b9b0
feedbacks
rxlabz Oct 15, 2020
370d2d5
fix test : revert to ArgumentError.notNull('options')
rxlabz Oct 15, 2020
974d1f6
remove required hub methods
rxlabz Oct 15, 2020
c8c1df0
implement `Hub.close()`
rxlabz Oct 16, 2020
6c6c649
feedbacks : remove the IHub interface + minors
rxlabz Oct 16, 2020
a924040
Hub.configureScope
rxlabz Oct 16, 2020
823c91c
Hub.clone and Scope.clone
rxlabz Oct 16, 2020
b04d67c
remove the non required scope methods
rxlabz Oct 16, 2020
37cf83c
replace the ListQueue _stack by a DoubleLinkedQueue
rxlabz Oct 16, 2020
b9f9f9e
rename `response` to `sentryId`
rxlabz Oct 16, 2020
5067a36
serialize the potential error stackTrace
rxlabz Oct 16, 2020
54729ba
Revert the DoubleLinkedQueue to a resourceless ListQueue
rxlabz Oct 16, 2020
729d6bc
fix a json serialization test
rxlabz Oct 16, 2020
4ea1537
add a const SentryId.emptyId
rxlabz Oct 16, 2020
5932534
don't assign a new lastEventId if the client capture method is not ca…
rxlabz Oct 16, 2020
97f6c53
feedbacks:
rxlabz Oct 16, 2020
e89c488
feedbacks:
rxlabz Oct 16, 2020
647f1b0
simplify the captureMessage API
rxlabz Oct 19, 2020
91f709a
capture message
rxlabz Oct 19, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions dart/lib/src/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,10 @@ abstract class SentryClient {
}

/// Reports an [event] to Sentry.io.
Future<SentryId> captureEvent({
@required Event event,
Future<SentryId> captureEvent(
Event event, {
StackFrameFilter stackFrameFilter,
Scope scope,
}) async {
final now = _clock();
var authHeader = 'Sentry sentry_version=6, sentry_client=$clientId, '
Expand Down Expand Up @@ -201,7 +202,7 @@ abstract class SentryClient {
exception: throwable,
stackTrace: stackTrace,
);
return captureEvent(event: event);
return captureEvent(event);
}

/// Reports the [message]
Expand All @@ -210,7 +211,7 @@ abstract class SentryClient {
SeverityLevel level = SeverityLevel.info,
}) {
final event = Event(message: message, level: level);
return captureEvent(event: event);
return captureEvent(event);
}

Future<void> close() async {
Expand Down
58 changes: 49 additions & 9 deletions dart/lib/src/hub.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'dart:async';
import 'dart:collection';

import 'client.dart';
import 'noop_client.dart';
import 'protocol.dart';
import 'scope.dart';
import 'sentry_options.dart';
Expand Down Expand Up @@ -76,7 +77,7 @@ class Hub {
final item = _stack.last;
if (item != null) {
try {
sentryId = await item.client.captureEvent(event: event);
sentryId = await item.client.captureEvent(event, scope: item.scope);
} catch (err) {
/* TODO add Event.id */
_options.logger(
Expand Down Expand Up @@ -116,6 +117,7 @@ class Hub {
final item = _stack.last;
if (item != null) {
try {
// TODO pass the scope
sentryId = await item.client.captureException(
throwable,
stackTrace: stackTrace,
Expand Down Expand Up @@ -158,10 +160,8 @@ class Hub {
final item = _stack.last;
if (item != null) {
try {
sentryId = await item.client.captureMessage(
message,
level: level,
);
// TODO pass the scope
sentryId = await item.client.captureMessage(message, level: level);
} catch (err) {
_options.logger(
SeverityLevel.error,
Expand All @@ -181,7 +181,26 @@ class Hub {

/// Binds a different client to the hub
void bindClient(SentryClient client) {
_stack.add(_StackItem(client, _stack.last.scope));
if (!_isEnabled) {
_options.logger(SeverityLevel.warning,
"Instance is disabled and this 'bindClient' call is a no-op.");
} else {
final item = _stack.last;
if (item != null) {
if (client != null) {
_options.logger(SeverityLevel.debug, 'New client bound to scope.');
item.client = client;
} else {
_options.logger(SeverityLevel.debug, 'NoOp client bound to scope.');
item.client = NoOpSentryClient();
}
} else {
_options.logger(
SeverityLevel.fatal,
'Stack peek was null when bindClient',
);
}
}
}

/// Clones the Hub
Expand Down Expand Up @@ -220,8 +239,29 @@ class Hub {

/// Configures the scope through the callback.
void configureScope(ScopeCallback callback) {
// TODO: implement configureScope
throw UnimplementedError();
if (!_isEnabled) {
_options.logger(
SeverityLevel.warning,
"Instance is disabled and this 'configureScope' call is a no-op.",
);
} else {
final item = _stack.last;
if (item != null) {
try {
callback(item.scope);
} catch (err) {
_options.logger(
SeverityLevel.error,
"Error in the 'configureScope' callback.",
);
}
} else {
_options.logger(
SeverityLevel.fatal,
'Stack peek was NULL when configureScope',
);
}
}
}

/// Runs the callback with a new scope which gets dropped at the end
Expand All @@ -243,7 +283,7 @@ class Hub {
}

class _StackItem {
final SentryClient client;
SentryClient client;

final Scope scope;

Expand Down
71 changes: 71 additions & 0 deletions dart/lib/src/noop_client.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import 'dart:async';

import 'package:http/src/client.dart';

import 'client.dart';
import 'protocol.dart';

class NoOpSentryClient implements SentryClient {
@override
User userContext;

@override
List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers,
) =>
[];

@override
Map<String, String> buildHeaders(String authHeader) => {};

@override
Future<SentryId> captureEvent(Event event, {stackFrameFilter, scope}) =>
Future.value(SentryId.empty());

@override
Future<SentryId> captureException(throwable, {stackTrace}) =>
Future.value(SentryId.empty());

@override
Future<SentryId> captureMessage(
Message message, {
SeverityLevel level = SeverityLevel.info,
}) =>
Future.value(SentryId.empty());

@override
String get clientId => 'No-op';

@override
Future<void> close() async {
return;
}

@override
Uri get dsnUri => null;

@override
Event get environmentAttributes => null;

@override
Client get httpClient => null;

@override
String get origin => null;

@override
String get postUri => null;

@override
String get projectId => null;

@override
String get publicKey => null;

@override
Sdk get sdk => null;

@override
String get secretKey => null;
}
26 changes: 26 additions & 0 deletions dart/lib/src/scope.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'dart:collection';

import 'package:collection/collection.dart';

import 'protocol.dart';
import 'sentry_options.dart';

Expand Down Expand Up @@ -136,4 +138,28 @@ class Scope {
void removeExtra(String key) {
_extra.remove(key);
}

@override
bool operator ==(Object other) =>
identical(this, other) ||
other is Scope &&
runtimeType == other.runtimeType &&
_level == other.level &&
_transaction == other.transaction &&
_user == other.user &&
IterableEquality().equals(_fingerprint, other.fingerprint) &&
IterableEquality().equals(_breadcrumbs, other.breadcrumbs) &&
MapEquality().equals(_tags, other.tags) &&
MapEquality().equals(_extra, other.extra);

@override
int get hashCode =>
_level.hashCode ^
_transaction.hashCode ^
_user.hashCode ^
_fingerprint.hashCode ^
_breadcrumbs.hashCode ^
_tags.hashCode ^
_extra.hashCode ^
_options.hashCode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to test scopes equality in tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this code to the tests instead? I'd avoid having this here if we don't need to maintain as production code (like putting scopes in as Map keys)

Copy link
Contributor Author

@rxlabz rxlabz Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I can without changing this kind of tests :

test('should capture event', () {
      hub.captureEvent(fakeEvent);
      verify(
        client.captureEvent(fakeEvent, scope: Scope(options), stackFrameFilter: null),
      ).called(1);
    });

If we don't override the ==operator the test will fail as it will only compare scope reference.
To remove the need of this override, I could change the test to :

test('should capture event', () {
      hub.captureEvent(fakeEvent);
      verify(
        client.captureEvent(fakeEvent,scope: anyNamed('scope'), stackFrameFilter: null),
      ).called(1);
    });

but it less precise. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the name of the tests, we're not really asserting anything about the scope, is that right? We just test that an event was captured?

For a test that requires validating equality of the argument, is there a arg validator that takes a callback?
like:

arg.is(s => s.a == expected.a) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "dartiest" way ! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but needs Dart >=2.6 :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer to keep this type of code needed only for tests, on the test packages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ moved in the test side, as a simple method for now
We should decide if we update the SDK constraints to use extensions and other "modern dart" features :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
8 changes: 0 additions & 8 deletions dart/lib/src/sentry.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ class Sentry {
final options = SentryOptions();
optionsConfiguration(options);
_hub = Hub(options);
/*_client = SentryClient(
dsn: options.dsn,
environmentAttributes: options.environmentAttributes,
compressPayload: options.compressPayload,
httpClient: options.httpClient,
clock: options.clock,
uuidGenerator: options.uuidGenerator,
);*/
}

/// Reports an [event] to Sentry.io.
Expand Down
1 change: 1 addition & 0 deletions dart/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ dependencies:
meta: ^1.0.0
stack_trace: ^1.0.0
uuid: ^2.0.0
collection: ^1.14.13

dev_dependencies:
mockito: ^4.1.1
Expand Down
73 changes: 66 additions & 7 deletions dart/test/hub_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ void main() {
});

test('should instanciate with a dsn', () {
final hub = Hub(SentryOptions(dsn: fakeDns));
final hub = Hub(SentryOptions(dsn: fakeDsn));
expect(hub.isEnabled, true);
});
});
Expand All @@ -29,15 +29,21 @@ void main() {
MockSentryClient client;

setUp(() {
options = SentryOptions(dsn: fakeDns);
options = SentryOptions(dsn: fakeDsn);
hub = Hub(options);
client = MockSentryClient();
hub.bindClient(client);
});

test('should capture event', () {
hub.captureEvent(fakeEvent);
verify(client.captureEvent(event: fakeEvent)).called(1);
verify(
client.captureEvent(
fakeEvent,
scope: Scope(options),
stackFrameFilter: null,
),
).called(1);
});

test('should capture exception', () {
Expand All @@ -54,11 +60,64 @@ void main() {
});
});

group('Close hub', () {
test('should close an enabled hub', () {
final hub = Hub(SentryOptions(dsn: fakeDns));
final client = MockSentryClient();
group('Hub scope', () {
Hub hub;
SentryClient client;

setUp(() {
hub = Hub(SentryOptions(dsn: fakeDsn));
client = MockSentryClient();
hub.bindClient(client);
});

test('should configure its scope', () {
hub.configureScope((Scope scope) {
scope
..level = SeverityLevel.debug
..user = fakeUser
..fingerprint = ['1', '2'];
});
hub.captureEvent(fakeEvent);

verify(
client.captureEvent(
fakeEvent,
scope: Scope(SentryOptions(dsn: fakeDsn))
..level = SeverityLevel.debug
..user = fakeUser
..fingerprint = ['1', '2'],
stackFrameFilter: null,
),
).called(1);
});
});

group('Hub Client', () {
Hub hub;
SentryClient client;
SentryOptions options;

setUp(() {
options = SentryOptions(dsn: fakeDsn);
hub = Hub(options);
client = MockSentryClient();
hub.bindClient(client);
});

test('should bind a new client', () {
final client2 = MockSentryClient();
hub.bindClient(client2);
hub.captureEvent(fakeEvent);
verify(
client2.captureEvent(
fakeEvent,
scope: Scope(options),
stackFrameFilter: null,
),
).called(1);
});

test('should close its client', () {
hub.close();

expect(hub.isEnabled, false);
Expand Down
4 changes: 3 additions & 1 deletion dart/test/mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import 'package:sentry/src/protocol.dart';

class MockSentryClient extends Mock implements SentryClient {}

final fakeDns = 'https://[email protected]/1234567';
final fakeDsn = 'https://[email protected]/1234567';

final fakeException = Exception('Error');

final fakeMessage =
Message(formatted: 'message 1', message: 'message %d', params: ['1']);

final fakeUser = User(id: '1', email: 'test@test');

final fakeEvent = Event(
loggerName: 'main',
serverName: 'server.dart',
Expand Down
Loading