Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Hub passes the Scope #114

Merged
merged 17 commits into from
Oct 20, 2020
Merged

ref: Hub passes the Scope #114

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8c6727d
init has a private method
marandaneto Oct 19, 2020
7570846
fix merge
marandaneto Oct 19, 2020
c1033b2
draft
marandaneto Oct 19, 2020
c3f453a
fix test
marandaneto Oct 19, 2020
a46e144
hub passes scope to client
marandaneto Oct 19, 2020
8a14620
fix
marandaneto Oct 19, 2020
4c3f323
fix broken tests
marandaneto Oct 19, 2020
35e3072
changelog
marandaneto Oct 19, 2020
30fa7a9
ignore omit_local_variable_types
marandaneto Oct 20, 2020
9088c88
ref
marandaneto Oct 20, 2020
e93f85e
ref imports
marandaneto Oct 20, 2020
7b05b0b
Update dart/lib/src/hub.dart
marandaneto Oct 20, 2020
1f70e4f
Update dart/lib/src/noop_hub.dart
marandaneto Oct 20, 2020
1f4bb44
Merge branch 'ref/hub-pass-scope' of github.com:getsentry/sentry-dart…
marandaneto Oct 20, 2020
d4334a9
ref
marandaneto Oct 20, 2020
d02947f
fix dartfmt
marandaneto Oct 20, 2020
ba721bd
fix merge conflict
marandaneto Oct 20, 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
29 changes: 15 additions & 14 deletions dart/lib/src/hub.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ class Hub {
);
}

final ListQueue<_StackItem> _stack;
final ListQueue<_StackItem> _stack = ListQueue();

/// if stack is empty, it throws IterableElementError.noElement()
_StackItem _peek() => _stack.isNotEmpty ? _stack.first : null;

final SentryOptions _options;

Expand All @@ -32,20 +35,18 @@ class Hub {
return Hub._(options);
}

Hub._(SentryOptions options)
: _options = options,
_stack = ListQueue() {
Hub._(SentryOptions options) : _options = options {
_stack.add(_StackItem(_getClient(fromOptions: options), Scope(_options)));
_isEnabled = true;
}

static void _validateOptions(SentryOptions options) {
if (options == null) {
throw ArgumentError.notNull('options');
throw ArgumentError.notNull('SentryOptions is required.');
}

if (options.dsn == null) {
throw ArgumentError.notNull('options.dsn');
if (options.dsn == null || options.dsn.isEmpty) {
throw ArgumentError.notNull('DSN is required.');
}
}

Expand All @@ -54,7 +55,7 @@ class Hub {
/// Check if the Hub is enabled/active.
bool get isEnabled => _isEnabled;

SentryId _lastEventId;
SentryId _lastEventId = SentryId.empty();

/// Last event id recorded in the current scope
SentryId get lastEventId => _lastEventId;
Expand All @@ -74,7 +75,7 @@ class Hub {
'captureEvent called with null parameter.',
);
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
try {
sentryId = await item.client.captureEvent(event, scope: item.scope);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rxlabz I noticed that all the methods are sync now because of await

await: You can use the await keyword to get the completed result of an asynchronous expression. The await keyword only works within an async function.

should we aim for sync calls or keep it async?

@bruno-garcia

Copy link
Member

Choose a reason for hiding this comment

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

While we send the envelope to the client/transport to be sent to the network directly, this should be async.
We could make it sync if we, inside the client, pass the envelope to an Isolate. And there we could use async to send it to the network or write to disk.

I'd propose we stick to async for now, user code already expects do await on capture methods, and allows us to interface with the native layer by purely writing an envelope to disk (which is async)

if you're not using async/await on the method body, just remove the keywords and return futureObj;

Copy link
Contributor

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 understand. They are not sync, they return a Future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rxlabz item.client.captureEvent returns a future, but you call this method using await
if you use the await keyboard, means you the future to be completed, so its a sync call, not async.
or I am not understanding the await keyword in Dart, which is also possible

Copy link
Contributor

@rxlabz rxlabz Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

await is just a "syntactic sugar", it doesn't change the nature of the code. Maybe https://dart.dev/codelabs/async-await will be clearer than me.
But I noticed that this method is asynchronous when it calls item.client.captureEvent and synchronous if the hub is not enabled or if the event is null, we probably should change the signature to

FutureOr<SentryId> captureEvent(Event event) async {//...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not a big fan because of:

Note: the FutureOr type is interpreted as dynamic in non strong-mode.

but I got it I guess:
await item.client.captureEvent makes a sync call, but the signature of the method
hub.captureEvent is also async so it's fine.

let's keep as it is till we have a concrete implementation of the transport layer, so we'd know what looks better.

Expand Down Expand Up @@ -115,7 +116,7 @@ class Hub {
'captureException called with null parameter.',
);
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
try {
// TODO pass the scope
Expand Down Expand Up @@ -162,7 +163,7 @@ class Hub {
'captureMessage called with null parameter.',
);
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
try {
// TODO pass the scope
Expand Down Expand Up @@ -196,7 +197,7 @@ class Hub {
_options.logger(SeverityLevel.warning,
"Instance is disabled and this 'bindClient' call is a no-op.");
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
if (client != null) {
_options.logger(SeverityLevel.debug, 'New client bound to scope.');
Expand Down Expand Up @@ -234,7 +235,7 @@ class Hub {
"Instance is disabled and this 'close' call is a no-op.",
);
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
try {
item.client.close();
Expand Down Expand Up @@ -262,7 +263,7 @@ class Hub {
"Instance is disabled and this 'configureScope' call is a no-op.",
);
} else {
final item = _stack.first;
final item = _peek();
if (item != null) {
try {
callback(item.scope);
Expand Down
8 changes: 8 additions & 0 deletions dart/lib/src/noop_client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ import 'client.dart';
import 'protocol.dart';

class NoOpSentryClient implements SentryClient {
NoOpSentryClient._();

static final NoOpSentryClient _instance = NoOpSentryClient._();

factory NoOpSentryClient() {
return _instance;
}

@override
User userContext;

Expand Down
51 changes: 51 additions & 0 deletions dart/lib/src/noop_hub.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import 'dart:async';

import 'package:sentry/src/client.dart';
import 'package:sentry/src/hub.dart';
import 'package:sentry/src/protocol/sentry_id.dart';
import 'package:sentry/src/protocol/level.dart';
import 'package:sentry/src/protocol/event.dart';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDE does the imports like this but feels weird, whats the proper way?

Copy link
Member

Choose a reason for hiding this comment

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

the linter should be pointing us to the right way (flutter analyze). I believe this is the way to go but I can't recall now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it didnt complain, thats why

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally in Dart, relative imports should be preferred https://fuchsia.dev/fuchsia-src/development/languages/dart/style#do_import_all_files_within_a_package_using_relative_paths
( but I know that the Flutter team doesn't follow all the Dart styleguide :) )
In IntelliJ, the autocompletion often shows both the relative and package: imports. I don't know if VSCode can be configured is this way.
We could also activate this lint https://dart-lang.github.io/linter/lints/prefer_relative_imports.html which seems to not be activated with pedantic to enforce the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice I created a task for it


import 'hub.dart';

class NoOpHub implements Hub {
NoOpHub._();

static final NoOpHub _instance = NoOpHub._();

factory NoOpHub() {
return _instance;
}

@override
void bindClient(SentryClient client) {}

@override
Future<SentryId> captureEvent(Event event) => Future.value(SentryId.empty());

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

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

@override
Hub clone() => this;

@override
void close() {}

@override
void configureScope(callback) {}

@override
bool get isEnabled => false;

@override
SentryId get lastEventId => SentryId.empty();
}
6 changes: 2 additions & 4 deletions dart/lib/src/protocol/sentry_id.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
/// Sentry response id

class SentryId {
static const String emptyId = '00000000-0000-0000-0000-000000000000';
static const String _emptyId = '00000000-0000-0000-0000-000000000000';

/// The ID Sentry.io assigned to the submitted event for future reference.
final String _id;

String get id => _id;

const SentryId(this._id);

factory SentryId.empty() => SentryId(emptyId);
factory SentryId.empty() => SentryId(_emptyId);

@override
String toString() => _id.replaceAll('-', '');
Expand Down
42 changes: 37 additions & 5 deletions dart/lib/src/sentry.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,72 @@ import 'client.dart';
import 'hub.dart';
import 'protocol.dart';
import 'sentry_options.dart';
import 'noop_hub.dart';

/// Configuration options callback
typedef OptionsConfiguration = void Function(SentryOptions);

/// Sentry SDK main entry point
///
class Sentry {
static Hub _hub;
static Hub _hub = NoOpHub();

Sentry._();

/// Returns the current hub
static Hub get currentHub => _hub;

/// Initializes the SDK
static void init(OptionsConfiguration optionsConfiguration) {
final options = SentryOptions();
optionsConfiguration(options);
_init(options);
}

/// Initializes the SDK
static void _init(SentryOptions options) {
if (isEnabled) {
options.logger(
SeverityLevel.warning,
'Sentry has been already initialized. Previous configuration will be overwritten.',
);
}

final hub = currentHub;
_hub = Hub(options);
hub.close();
}

/// Reports an [event] to Sentry.io.
static Future<SentryId> captureEvent(Event event) async {
return _hub.captureEvent(event);
return currentHub.captureEvent(event);
}

/// Reports the [exception] and optionally its [stackTrace] to Sentry.io.
static Future<SentryId> captureException(
dynamic error, {
dynamic stackTrace,
}) async {
return _hub.captureException(error, stackTrace: stackTrace);
return currentHub.captureException(error, stackTrace: stackTrace);
}

Future<SentryId> captureMessage(
String message, {
SeverityLevel level,
String template,
List<dynamic> params,
}) async {
return currentHub.captureMessage(message,
level: level, template: template, params: params);
}

/// Close the client SDK
static Future<void> close() async => _hub.close();
static Future<void> close() async => currentHub.close();

/// Check if the current Hub is enabled/active.
static bool get isEnabled => currentHub.isEnabled;

/// client injector only use for testing
@visibleForTesting
static void initClient(SentryClient client) => _hub.bindClient(client);
static void initClient(SentryClient client) => currentHub.bindClient(client);
}
4 changes: 2 additions & 2 deletions dart/test/test_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Future testCaptureException(
} catch (error, stackTrace) {
final sentryId =
await client.captureException(error, stackTrace: stackTrace);
expect('${sentryId.id}', 'test-event-id');
expect('${sentryId.toString()}', 'test-event-id');
}

expect(postUri, client.postUri);
Expand Down Expand Up @@ -302,7 +302,7 @@ void runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false}) {
} catch (error, stackTrace) {
final sentryId =
await client.captureException(error, stackTrace: stackTrace);
expect('${sentryId.id}', SentryId.emptyId);
expect('$sentryId', SentryId.empty());
}

await client.close();
Expand Down