ref: Hub passes the Scope #114

marandaneto · 2020-10-19T13:01:07Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

ref: Hub passes the Scope

💡 Motivation and Context

Client needs to have the Scope in order to prepare the event.
Also a few other improvements I've found on the way

💚 How did you test it?

tests

📝 Checklist

I reviewed submitted code
I added tests to verify changes
All tests passing
No breaking changes

🔮 Next steps

dart/lib/src/hub.dart

marandaneto · 2020-10-19T13:03:14Z

dart/lib/src/noop_hub.dart

+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';


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

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

it didnt complain, thats why

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.

nice I created a task for it

marandaneto · 2020-10-19T14:07:55Z

dart/lib/src/hub.dart

@@ -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);


@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

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;

I'm not sure I understand. They are not sync, they return a Future

@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

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 {//...}

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.

marandaneto · 2020-10-19T15:40:48Z

dart/lib/src/protocol/sentry_id.dart


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

-  String get id => _id;
+  // TODO: should we generate the new UUID here with an empty ctor?


if so, we'd need to import the uuid dependency and make the _id of its type

I maybe missed it, but I didn't found a method to generate a Uuid from String in the uuid package

maybe parse and unparse https://github.com/Daegalus/dart-uuid#uuidparsestring-uuid-list-buffer-null-int-offset-0

I created a task for it

dart/lib/src/hub.dart

bruno-garcia · 2020-10-19T15:57:52Z

dart/lib/src/hub.dart

@@ -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);


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;

bruno-garcia · 2020-10-19T16:00:02Z

dart/lib/src/noop_hub.dart

+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';


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

rxlabz · 2020-10-20T09:30:59Z

dart/lib/src/hub.dart

@@ -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);


I'm not sure I understand. They are not sync, they return a Future

rxlabz · 2020-10-20T09:32:38Z

dart/lib/src/protocol/sentry_id.dart


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

-  String get id => _id;
+  // TODO: should we generate the new UUID here with an empty ctor?


I maybe missed it, but I didn't found a method to generate a Uuid from String in the uuid package

dart/lib/src/hub.dart

dart/lib/src/noop_hub.dart

dart/lib/src/sentry.dart

dart/test/hub_test.dart

dart/test/sentry_test.dart

Co-authored-by: Erick Ghaumez <[email protected]>

… into ref/hub-pass-scope

rxlabz

LGTM

marandaneto added 3 commits October 19, 2020 13:20

init has a private method

8c6727d

fix merge

7570846

draft

c1033b2

marandaneto requested a review from bruno-garcia as a code owner October 19, 2020 13:01

marandaneto marked this pull request as draft October 19, 2020 13:01

marandaneto changed the title ~~ref/hub pass scope~~ ref: Hub passes the Scope Oct 19, 2020

marandaneto changed the base branch from main to feature/unified-api October 19, 2020 13:02

marandaneto commented Oct 19, 2020

View reviewed changes

dart/lib/src/hub.dart Outdated Show resolved Hide resolved

marandaneto commented Oct 19, 2020

View reviewed changes

marandaneto added 2 commits October 19, 2020 15:15

fix test

c3f453a

hub passes scope to client

a46e144

marandaneto commented Oct 19, 2020

View reviewed changes

marandaneto added 2 commits October 19, 2020 16:23

fix

8a14620

fix broken tests

4c3f323

marandaneto requested a review from rxlabz October 19, 2020 15:38

marandaneto commented Oct 19, 2020

View reviewed changes

changelog

35e3072

bruno-garcia reviewed Oct 19, 2020

View reviewed changes

ignore omit_local_variable_types

30fa7a9

rxlabz reviewed Oct 20, 2020

View reviewed changes

marandaneto and others added 6 commits October 20, 2020 13:24

ref

9088c88

ref imports

e93f85e

Update dart/lib/src/hub.dart

7b05b0b

Co-authored-by: Erick Ghaumez <[email protected]>

Update dart/lib/src/noop_hub.dart

1f70e4f

Co-authored-by: Erick Ghaumez <[email protected]>

Merge branch 'ref/hub-pass-scope' of github.com:getsentry/sentry-dart…

1f4bb44

… into ref/hub-pass-scope

ref

d4334a9

marandaneto marked this pull request as ready for review October 20, 2020 11:28

fix dartfmt

d02947f

marandaneto requested review from bruno-garcia and rxlabz October 20, 2020 12:50

rxlabz approved these changes Oct 20, 2020

View reviewed changes

fix merge conflict

ba721bd

marandaneto merged commit 2b36bec into feature/unified-api Oct 20, 2020

marandaneto deleted the ref/hub-pass-scope branch October 20, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Hub passes the Scope #114

ref: Hub passes the Scope #114

marandaneto commented Oct 19, 2020 •

edited

Loading

marandaneto Oct 19, 2020

bruno-garcia Oct 19, 2020

marandaneto Oct 20, 2020

rxlabz Oct 20, 2020

marandaneto Oct 20, 2020

marandaneto Oct 19, 2020

bruno-garcia Oct 19, 2020

rxlabz Oct 20, 2020

marandaneto Oct 20, 2020

rxlabz Oct 20, 2020 •

edited

Loading

marandaneto Oct 20, 2020

marandaneto Oct 19, 2020

rxlabz Oct 20, 2020

marandaneto Oct 20, 2020

marandaneto Oct 20, 2020

bruno-garcia Oct 19, 2020

bruno-garcia Oct 19, 2020

rxlabz Oct 20, 2020

rxlabz Oct 20, 2020

rxlabz left a comment

ref: Hub passes the Scope #114

ref: Hub passes the Scope #114

Conversation

marandaneto commented Oct 19, 2020 • edited Loading

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxlabz Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxlabz left a comment

Choose a reason for hiding this comment

marandaneto commented Oct 19, 2020 •

edited

Loading

rxlabz Oct 20, 2020 •

edited

Loading