Sentry - Support browser by lejard-h · Pull Request #22 · getsentry/sentry-dart

lejard-h · 2018-08-01T09:40:06Z

Refactor to support browser
- SentryClient from package:sentry/sentry.dart with conditional import
- SentryBrowserClient for web from package:sentry/browser_client.dart
- SentryIOClient for VM and Flutter from package:sentry/io_client.dart
Write test for browser
Working Angular Sentry packages => https://github.com/leftyio/angular_sentry

Main implementation difference are:

gzip compressing not available on browser
javascript stacktrace need to be prefix with window.location.origin to be resolve by sentry

yjbanov · 2018-08-01T15:53:05Z

Thanks for the PR! A couple of things before we can move forward. This PR needs two reviews, one to cover the dart-for-web side of things and one to cover the Flutter/VM parts. We have a few people on the Flutter team (including myself) who can provide the latter, but not the former. Therefore we need to find someone on our team who would be willing to own the Web functionality for flutter/sentry.

@kevmoo, @matanlurey, can you help with this?

If you need to move fast and can't wait for these organizational/ownership issues to be resolved, one option is to fork this library (Go open source!).

lejard-h · 2018-08-01T16:10:46Z

I already have a fork that we use in production and that is used by angular_sentry for a few month now.

I don't need to move fast, the coming release of dart 2 and angular 5 seems to be a good opportunity to build a strong library for sentry.

matanlurey · 2018-08-01T16:13:44Z

It is unlikely my team will be able to own this anytime soon @yjbanov

lejard-h · 2018-08-06T12:50:32Z

angular implementation => leftyio/angular_sentry#1

Hixie · 2018-08-28T23:29:15Z

@yjbanov Are you able to continue to shepherd this through?

yjbanov · 2018-08-29T00:45:04Z

Unfortunately, no.

eseidelGoogle · 2018-09-12T02:23:20Z

https://github.com/getsentry maintains official Sentry plugins, it's possible they would be interested in taking on a fuller-featured sentry plugin for Dart server, browser and flutter.

Hixie · 2018-10-30T23:58:15Z

@lejard-h We've moved this to the packages repo. I'm not sure we have anyone who would be able to review the change, still, but in the meantime, would you be willing to resubmit the PR to the other repo?

lejard-h · 2018-11-06T08:26:48Z

I will do that 👍

This reverts commit 2344a24.

asked here getsentry/sentry-dart#22 (comment) move the current sentry packages without modifications, except fixing warnings due to new analysis_options.

listepo · 2019-06-09T16:00:13Z

Any news? (Flutter for a web already presented)

yjbanov · 2019-06-13T14:40:27Z

Any news? (Flutter for a web already presented)

Yes, this will have to align with the Flutter for web efforts, likely after we merge the Web support into Flutter proper.

Standaa · 2019-10-22T16:21:28Z

Any updates as to when this PR will be accepted ?

yjbanov · 2019-10-22T17:20:51Z

As of September 10, 2019, Flutter for web merged into the mainline Flutter SDK, so we can certainly reopen this discussion.

lejard-h · 2019-10-22T21:18:01Z

I fixed the conflicts.

Test are passing but I still need to check my angular_sentry implementation and run additional tests on real env.

I also added conditional import support on package:sentry/sentry.dart so it should not be breaking change.

jaumard · 2019-11-03T12:56:57Z

I've added sentry without checking first if it's support web :( shame on me lool can't run on web anymore.

But now I'm interested to see this merged ^^

yjbanov · 2019-11-04T20:56:21Z

I'm reviewing this PR right now in between other high-priority tasks that have been assigned to me, but it's big! :) So it will take some time.

yjbanov · 2019-10-24T05:44:49Z

+    Map<String, dynamic> data,
+    Map<String, String> headers,
+  ) =>
+      // Gzip compression is not available on browser


To be more precise gzip is available in web browsers but it is done implicitly.

yjbanov · 2019-11-04T18:28:28Z

  return '${frame.uri}';
 }
+
+class EmptyStacktraceException implements Exception {


Let's document all public API, including the class and public members.

Actually It was not used, It just removed it

yjbanov · 2019-11-04T18:29:07Z

 const String sdkName = 'dart';

 /// The name of the SDK platform reported to Sentry.io in the submitted events.
+/// Used for IO version


Separate paragraphs with one blank line. End the sentence with a period.

yjbanov · 2019-11-04T18:29:16Z

+/// Used for IO version
 const String sdkPlatform = 'dart';
+
+/// Used to report browser Stacktrace to sentry


End the sentence with a period.

yjbanov · 2019-11-04T18:34:39Z

-import 'src/utils.dart';
-import 'src/version.dart';
-
+export 'src/base.dart';


Please keep the license header.

yjbanov · 2019-11-04T18:36:15Z

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/// A pure Dart client for Sentry.io crash reporting.


This comment would be most useful in lib/sentry.dart than here. Let's move it there.

yjbanov · 2019-11-10T00:10:13Z

@@ -1,7 +0,0 @@
-#!/bin/sh


Nice clean-up! Thanks.

yjbanov · 2019-11-10T00:12:10Z

@@ -1,5 +1,12 @@
 # package:sentry changelog

+## 2.4.0


How about making it a 3.0.0-dev0, given how big and probably subtly breaking this change is?

yjbanov · 2019-11-10T00:16:39Z

+import 'client_stub.dart'
+    // ignore: uri_does_not_exist
+    if (dart.library.html) 'browser.dart'
+    // ignore: uri_does_not_exist


Why are ignores necessary?

yjbanov

Do browser_client.dart and io_client.dart add much value on top of the configurable imports?

yjbanov · 2019-11-10T00:18:09Z

+const String _testDsnWithPort =
+    'https://public:secret@sentry.example.com:8888/1';
+void main() {
+  group('$SentryBrowserClient', () {


You can also do group(SentryBrowserClient, ...

yjbanov · 2019-11-10T00:20:39Z

@@ -0,0 +1,425 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.


It looks like a lot of the test code is identical with the io version. Would it be possible to have shared test code configurable via a typedef SentryClientFactory that provides either io or browser implementation of the client??

The conditional import made it really simple to do, I moved all common and configurable tests under the test_utils.dart file using a single function

runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false})

Also moved Event test under event_test.dart file.

jaumard · 2019-11-26T11:56:36Z

All good now @yjbanov ? :)

yjbanov · 2019-11-26T14:59:52Z

Thanks for the ping @jaumard.

@lejard-h Is this ready for another pass? This looks almost there.

yjbanov · 2019-11-26T14:55:02Z

@@ -1,653 +1,8 @@
-// Copyright 2017 The Chromium Authors. All rights reserved.
+// Copyright 2019 The Chromium Authors. All rights reserved.


Let's keep the original year.

yjbanov · 2019-11-26T14:55:42Z

@@ -0,0 +1,16 @@
+import 'package:http/http.dart';


This needs a license header.

should be good now

yjbanov · 2019-11-28T04:20:00Z

@lejard-h I can't thank you enough for your boundless patience! As I read through the history of this PR, I'm amazed at how things have changed since August 2018, when it was first filed. This PR survived tectonic shifts in Flutter and Dart.

Support browser

338f436

lejard-h changed the title ~~Support browser~~ Sentry: Support browser Aug 1, 2018

lejard-h changed the title ~~Sentry: Support browser~~ WIP Sentry - Support browser Aug 1, 2018

lejard-h mentioned this pull request Aug 1, 2018

[sentry][web] sourcemap not used flutter/flutter#16883

Closed

lejard-h added 2 commits August 1, 2018 11:42

Delete sentry.iml

9688ee2

remove unnecessary docs

709db34

lejard-h added 4 commits August 5, 2018 13:54

add test

7f813c0

Event.replace method

703842e

fix replace

417e60f

fix browser test

481b905

lejard-h mentioned this pull request Aug 6, 2018

Update leftyio/angular_sentry#1

Merged

lejard-h changed the title ~~WIP Sentry - Support browser~~ Sentry - Support browser Aug 6, 2018

lejard-h added 2 commits August 6, 2018 15:07

remove dart2_test, use pub run

d84caa4

add chrome to travis

31b276a

lejard-h added 2 commits December 10, 2018 16:38

fix org-dartlang-sdk path on browser

2344a24

Revert "fix org-dartlang-sdk path on browser"

ef78e25

This reverts commit 2344a24.

lejard-h added a commit to lejard-h/packages that referenced this pull request Mar 17, 2019

move sentry packages

d516378

asked here getsentry/sentry-dart#22 (comment) move the current sentry packages without modifications, except fixing warnings due to new analysis_options.

lejard-h mentioned this pull request Mar 17, 2019

move sentry packages flutter/packages#17

Closed

try to debug unparsed stacktrace

3f154b5

lejard-h mentioned this pull request Oct 22, 2019

Request: AngularDart with Sentry.io (or similar service). angulardart/angular#413

Closed

lejard-h added 4 commits October 22, 2019 21:57

Merge branch 'master' into web

59c8942

fix clients

7efc5b9

Merge remote-tracking branch 'flutter/master' into web

3ca525d

Format

4a78ddf

Event.copyWith

295a68f

yjbanov reviewed Nov 10, 2019

View reviewed changes

lejard-h added 4 commits November 13, 2019 21:48

Fix license ad comments

c8330a9

Cleanup and pass test

7873e8a

fix mock client

962b4c5

format

9101a68

yjbanov reviewed Nov 26, 2019

View reviewed changes

fix license header

64a607e

yjbanov merged commit 46e6287 into getsentry:master Nov 28, 2019

This was referenced Dec 24, 2021

Error in console when calling Sentry.captureException #680

Closed

Expandos are not allowed on strings, numbers, booleans or null): #681

Closed

		@@ -0,0 +1,425 @@
		// Copyright 2017 The Chromium Authors. All rights reserved.

		@@ -1,653 +1,8 @@
		// Copyright 2017 The Chromium Authors. All rights reserved.
		// Copyright 2019 The Chromium Authors. All rights reserved.

Uh oh!

Uh oh!

Conversation

lejard-h commented Aug 1, 2018 • edited by yjbanov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yjbanov commented Aug 1, 2018

Uh oh!

lejard-h commented Aug 1, 2018

Uh oh!

matanlurey commented Aug 1, 2018

Uh oh!

lejard-h commented Aug 6, 2018

Uh oh!

Hixie commented Aug 28, 2018

Uh oh!

yjbanov commented Aug 29, 2018

Uh oh!

eseidelGoogle commented Sep 12, 2018

Uh oh!

Hixie commented Oct 30, 2018

Uh oh!

lejard-h commented Nov 6, 2018

Uh oh!

listepo commented Jun 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yjbanov commented Jun 13, 2019

Uh oh!

Standaa commented Oct 22, 2019

Uh oh!

yjbanov commented Oct 22, 2019

Uh oh!

lejard-h commented Oct 22, 2019

Uh oh!

jaumard commented Nov 3, 2019

Uh oh!

yjbanov commented Nov 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lejard-h Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaumard commented Nov 26, 2019

Uh oh!

yjbanov commented Nov 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

lejard-h commented Aug 1, 2018 •

edited by yjbanov

Loading

listepo commented Jun 9, 2019 •

edited

Loading

lejard-h Nov 13, 2019 •

edited

Loading

yjbanov Nov 26, 2019 •

edited

Loading