Feat: Add some more information for exceptions collected via `FlutterError.onError` #538

ueman · 2021-07-24T15:29:28Z

📜 Description

The information in the red square is added by this PR.

💡 Motivation and Context

💚 How did you test it?

Changed test accordingly

📝 Checklist

🔮 Next steps

codecov-commenter · 2021-07-24T15:32:48Z

Codecov Report

Merging #538 (63e5c94) into main (db3cde2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #538   +/-   ##
=======================================
  Coverage   91.55%   91.55%           
=======================================
  Files          74       74           
  Lines        2428     2428           
=======================================
  Hits         2223     2223           
  Misses        205      205

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db3cde2...63e5c94. Read the comment docs.

flutter/lib/src/default_integrations.dart

bruno-garcia

How often can the information field be larger than what Sentry can display properly? It transforms an array into a single line, so it could end up truncated or not fitting the UI very well. Perhaps we need to find another way to present it?

flutter/lib/src/default_integrations.dart

flutter/test/default_integrations_test.dart

ueman · 2021-07-25T16:42:51Z

How often can the information field be larger than what Sentry can display properly? It transforms an array into a single line, so it could end up truncated or not fitting the UI very well.

Hard to say as its origin can be the Flutter framework, libraries and developers themselves.
So it's probably better to assume that it can be very long.

Perhaps we need to find another way to present it?

What would be a better way to display this?
As far as I can tell, the only data which is displayed as a text block is the exception description.

The events data could be another place for this, but then the information would be visually disconnected in the Sentry UI :(

Co-authored-by: Bruno Garcia <[email protected]>

ueman · 2021-07-26T06:47:53Z

It doesn't play nicely if it's really long :( I raised an issue for this little UI bug here: getsentry/sentry#27737

marandaneto · 2021-07-27T07:47:15Z

flutter/example/lib/main.dart

+            RaisedButton(
+              child: const Text('Capture from FlutterError.onError'),
+              onPressed: () {
+                // modeled after a real exception
+                FlutterError.onError?.call(FlutterErrorDetails(
+                  exception: Exception('A really bad exception'),
+                  silent: false,
+                  context: DiagnosticsNode.message('while handling a gesture'),
+                  library: 'gesture',
+                  informationCollector: () => [
+                    DiagnosticsNode.message(
+                        'Handler: "onTap" Recognizer: TapGestureRecognizer'),
+                    DiagnosticsNode.message(
+                        'Handler: "onTap" Recognizer: TapGestureRecognizer'),
+                    DiagnosticsNode.message(
+                        'Handler: "onTap" Recognizer: TapGestureRecognizer'),
+                  ],
+                ));
+              },
+            ),


the example Flutter error : Scaffold.of() already triggers a FlutterError.onError error, isnt that enough? is it common that people trigger FlutterError.onError manually too?

the example Flutter error : Scaffold.of() already triggers a FlutterError.onError error, isnt that enough?

This is just to test how it looks in the Sentry UI when it has a long trail of information, which Flutter error : Scaffold.of() doesn't have (as seen in the screenshot of the PR description). We can remove it before merging.

is it common that people trigger FlutterError.onError manually too?

No, but it's not uncommon in libraries.

marandaneto · 2021-07-29T07:08:42Z

@ueman is this ready to be merged or the multiple line thing should be tested?

ueman · 2021-07-29T10:27:13Z

@ueman is this ready to be merged or the multiple line thing should be tested?

It works and is tested but it could be that it doesn't look that good in the Sentry UI for long information. So unless you or @bruno-garcia has a better idea where to put the information this is good to go.

I tried to add it to the events extra data, but then this information kinda gets lost.

marandaneto · 2021-07-29T11:52:54Z

I tried to add it to the events extra data, but then this information kinda gets lost.

what do you mean by kinda get lost? does it make to Sentry or not easy to find since the information is down there in the additional data?
just trying to understand if we have a bug or not.

ueman · 2021-07-29T13:32:20Z

I tried to add it to the events extra data, but then this information kinda gets lost.

what do you mean by kinda get lost? does it make to Sentry or not easy to find since the information is down there in the additional data?
just trying to understand if we have a bug or not.

It gets visually lost when it at the same place as all the device, os and app information. It does make to Sentry so it's not a bug.

I'll add some pictures later to clarify what I mean :D

ueman · 2021-07-29T16:19:28Z

This event has the data added to the mechanism and as the crash_context. At the top I think it's really useful, though it might not look good for long text, just like @bruno-garcia said. At the bottom, the information gets kinda visually lost next to all the meta data.

bruno-garcia · 2021-07-29T16:28:35Z

I'm a bit wary of adding data that'll be potentially large in that field in this form. Perhaps best we find a way to break in up in more 'key/value' pairs

bagedevimo · 2021-08-04T21:01:34Z

This is looks amazing for us, as we have 3 different flutter apps all using this context data and we've only just realized it's missing. Very keen to help out in any way that I can.

ueman · 2021-08-05T05:48:42Z

This is looks amazing for us, as we have 3 different flutter apps all using this context data and we've only just realized it's missing. Very keen to help out in any way that I can.

Thanks! That's really appreciated.
Do you mind answering the following questions?

How long are your context and information values?
What length of these fields have you seen in the wild?
How would you like to see these fields in the Sentry UI?

bagedevimo · 2021-08-05T19:06:39Z

Our context fields are typically 3-4 words, similar length to your example. We have none longer than 36 characters. I'm afraid we've never seen information values in practice, as Sentry isn't capturing them :P. Looking at the definition of FlutterErrorDetails, it seems reasonable to suggest these should be key/value pairs, but as far as I can tell you could only split on the newlines that may or may not exist. Seems fine for most use-cases? I don't know a lot about the Sentry UI though sorry. A neat UX might be that if a tag is too long up the top, it is cut off and can be clicked to scroll down to the full value.

ueman · 2021-08-07T08:51:12Z

Okay, I've changed it so that the mechanism has a tag which tells the user to look for a flutter_error_details context down below. Down there, everything displays well. I hope that is satisfying for all :D

ueman added 2 commits July 24, 2021 17:24

WIP

51719f4

Changelog

e22e346

ueman requested a review from marandaneto as a code owner July 24, 2021 15:29

changelog

fdf86e9

ueman commented Jul 25, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Outdated Show resolved Hide resolved

bruno-garcia reviewed Jul 25, 2021

View reviewed changes

flutter/lib/src/default_integrations.dart Outdated Show resolved Hide resolved

flutter/test/default_integrations_test.dart Show resolved Hide resolved

Update flutter/lib/src/default_integrations.dart

745c1f3

Co-authored-by: Bruno Garcia <[email protected]>

wip

7de2d6d

marandaneto reviewed Jul 27, 2021

View reviewed changes

ueman added 3 commits August 7, 2021 09:47

More Tests, other UI

5e52b92

Merge remote-tracking branch 'origin/main' into feat/improve_on_error

8d9a47a

fix changelog

63e5c94

marandaneto approved these changes Aug 11, 2021

View reviewed changes

ueman merged commit a5127ce into main Aug 11, 2021

ueman deleted the feat/improve_on_error branch August 11, 2021 12:43

Uh oh!

Feat: Add some more information for exceptions collected via FlutterError.onError #538

Feat: Add some more information for exceptions collected via FlutterError.onError #538

Uh oh!

Conversation

ueman commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

codecov-commenter commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ueman commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueman commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marandaneto Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

ueman Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Jul 29, 2021

Uh oh!

ueman commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marandaneto commented Jul 29, 2021

Uh oh!

ueman commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueman commented Jul 29, 2021

Uh oh!

bruno-garcia commented Jul 29, 2021

Uh oh!

bagedevimo commented Aug 4, 2021

Uh oh!

ueman commented Aug 5, 2021

Uh oh!

bagedevimo commented Aug 5, 2021

Uh oh!

ueman commented Aug 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Feat: Add some more information for exceptions collected via `FlutterError.onError` #538

Feat: Add some more information for exceptions collected via `FlutterError.onError` #538

ueman commented Jul 24, 2021 •

edited

Loading

codecov-commenter commented Jul 24, 2021 •

edited

Loading

ueman commented Jul 25, 2021 •

edited

Loading

ueman commented Jul 26, 2021 •

edited

Loading

ueman commented Jul 29, 2021 •

edited

Loading

ueman commented Jul 29, 2021 •

edited

Loading