Feat : add a Hub class #113
Conversation
- replace SentryResponse with SentryId
|
@marandaneto @bruno-garcia Hi, I started this PR, early feedbacks are welcome :) |
dart/lib/src/hub.dart
Outdated
| dynamic throwable, | ||
| dynamic stackTrace, |
There was a problem hiding this comment.
why are we using dynamic? should we not use the types?
There was a problem hiding this comment.
there is no type union in Dart, and both Error and Exception are throwable, and not related. In fact you can throw anything in Dart ( but null which throws NullThrownError ).
There was a problem hiding this comment.
I see, well making it dynamic works but if it could be literally anything, how we would know which fields to read the info out of the exception, stack trace, messages and so on, we'd need to think this thru, if its the case, we could offer overloads like
captureException(exception type)
captureError(error type)
capture??(any?)
I am just thinking out loud here :)
There was a problem hiding this comment.
There is no common interface for Dart Errors
- a Dart
Errorhas no fields https://api.dart.dev/stable/2.10.2/dart-core/Error-class.html. Exceptionhas an optional argument ( String message ) https://api.dart.dev/stable/2.10.2/dart-core/Exception-class.html
For now we send theexception.runtimeTypeand theexception.toString(), whatever the exception is.
if (exception != null) {
json['exception'] = [
<String, dynamic>{
'type': '${exception.runtimeType}',
'value': '$exception',
}
];
}There was a problem hiding this comment.
I see but an Error has a stack trace field https://api.dart.dev/stable/2.10.2/dart-core/Error/stackTrace.html
There was a problem hiding this comment.
stackTrace could be read in SentryClient from the throwable if its an error and not taken as params in the Sentry static class/Hub
There was a problem hiding this comment.
I found the reason why Dart exceptions have no stacktrace : https://groups.google.com/a/dartlang.org/g/misc/c/O1OKnYTUcoo/m/ipIE1jqHD9sJ
I think the current implementation allows to send both :
- an exception and it's stacktrace if the user catch it and want to send it,
- and an error with the stacktrace automatically serialized
Is it ok for you @marandaneto ?
There was a problem hiding this comment.
yeah, sounds good, but then stacktrace should not be mandatory right?
also, if the error has a stacktrace and the user still passed a stacktrace, which one wins? we need to think about that, but it doesn't need to be in this PR then
There was a problem hiding this comment.
only the throwable is a required argument :
Future<SentryId> captureException(
dynamic throwable, {
dynamic stackTrace,
})for the if a stacktrace is passed with an error, it will override the error.stacktrace
There was a problem hiding this comment.
perfect, we'd need to mention this in the docs later.
- feedbacks : remove the IHub interface - client.captureEvent define event as a required param - pass the scope to client.captureEvent(...)
dart/lib/src/scope.dart
Outdated
| @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; |
There was a problem hiding this comment.
mm why do we need this?
There was a problem hiding this comment.
to test scopes equality in tests.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
the "dartiest" way ! 😄
There was a problem hiding this comment.
but needs Dart >=2.6 :(
There was a problem hiding this comment.
I'd also prefer to keep this type of code needed only for tests, on the test packages
There was a problem hiding this comment.
✅ 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 :)
There was a problem hiding this comment.
dart/lib/src/scope.dart
Outdated
| @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; |
There was a problem hiding this comment.
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)
- override SentryId.toString - typos
- add a scope comparaison method in tests - move pkg:collection to dev_depencies - changelog
- add template and params named params to captureMessage - update the Message class constructor
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps