search

getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
766 stars 239 forks source link

Remove `sentry` frames if SDK falls back to current stack trace #2351

Closed denrase closed 2 weeks ago

denrase commented 1 month ago

:scroll: Description

So we don't say that the app crashed in stacktrace_utils.dart anymore.

Before

Bildschirmfoto 2024-10-14 um 14 39 16

After (More frames are hidden in this screenshot, but it's identical to before)

Bildschirmfoto 2024-10-14 um 16 05 59

:bulb: Motivation and Context

Relates to #2268

:green_heart: How did you test it?

:pencil: Checklist

:crystal_ball: Next steps

Will this affect grouping as well?

Grouping in Sentry is different for events with stack traces and without. As a result, you will get new groups as you enable or disable this flag for certain events.

Will this affect SDK crash detection, provided we have this functionality on sentry dart/flutter?

github-actions[bot] commented 1 month ago
Messages
:book: Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by :no_entry_sign: dangerJS against 5ddd2af1b1844a65fb15b62cc7496c57d88e1400

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.05%. Comparing base (6d5e62f) to head (5ddd2af). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2351 +/- ## ========================================== + Coverage 85.02% 85.05% +0.03% ========================================== Files 257 257 Lines 9175 9190 +15 ========================================== + Hits 7801 7817 +16 + Misses 1374 1373 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 1 month ago

Android Performance metrics :rocket:

  Plain With Sentry Diff
Startup time 471.67 ms 507.40 ms 35.73 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e5b744f308ef88a70f679a622a88ade69705613c 302.70 ms 342.17 ms 39.47 ms
0ac1eed9198ed3ff6a9da538a7e34be4807120c3 370.60 ms 441.54 ms 70.94 ms
90a08ea59ea9a5f3aed6058b400411496b29aa91 477.25 ms 534.10 ms 56.85 ms
0db91ccbfaec953eb629db000f6a6034aa04e2c8 327.85 ms 387.31 ms 59.46 ms
061fed2925fdc038a8b4ed8f7905e646d7c0a887 434.11 ms 506.49 ms 72.38 ms
4829ad36de3c9b0c6cd7c1d030ee2987d7bb7347 381.55 ms 455.45 ms 73.90 ms
519423f251b6c787fd39742528f413087966d476 357.00 ms 415.77 ms 58.77 ms
ddc97adb5918b9f9eda573656e6edd9401b17801 331.45 ms 384.06 ms 52.61 ms
e3ef57098845edc69a00ce9b4845b54351beccbf 389.71 ms 459.16 ms 69.45 ms
2d3b03d17e0b28a698d4de81ab6ac6b586c4533a 309.53 ms 353.40 ms 43.87 ms

App size

Revision Plain With Sentry Diff
e5b744f308ef88a70f679a622a88ade69705613c 6.06 MiB 7.09 MiB 1.03 MiB
0ac1eed9198ed3ff6a9da538a7e34be4807120c3 6.06 MiB 7.03 MiB 990.44 KiB
90a08ea59ea9a5f3aed6058b400411496b29aa91 6.49 MiB 7.55 MiB 1.06 MiB
0db91ccbfaec953eb629db000f6a6034aa04e2c8 5.94 MiB 6.95 MiB 1.01 MiB
061fed2925fdc038a8b4ed8f7905e646d7c0a887 6.52 MiB 7.59 MiB 1.06 MiB
4829ad36de3c9b0c6cd7c1d030ee2987d7bb7347 6.33 MiB 7.26 MiB 943.11 KiB
519423f251b6c787fd39742528f413087966d476 6.06 MiB 7.03 MiB 989.24 KiB
ddc97adb5918b9f9eda573656e6edd9401b17801 6.16 MiB 7.14 MiB 1003.75 KiB
e3ef57098845edc69a00ce9b4845b54351beccbf 6.33 MiB 7.26 MiB 950.38 KiB
2d3b03d17e0b28a698d4de81ab6ac6b586c4533a 6.06 MiB 7.09 MiB 1.03 MiB

Previous results on branch: feat/stack-frame-excludes

Startup times

Revision Plain With Sentry Diff
48c36f9a1dd0b04cdfa20c05451a7e7511ba4bb4 455.06 ms 498.96 ms 43.89 ms
eef08283b9d6b298955040c28c9a16ae12993314 454.86 ms 479.50 ms 24.64 ms
6a2fb35c8a6b0b085859e1c4a5b15e86978560f2 452.60 ms 471.21 ms 18.61 ms
e9b16e8e8b1129df9f7d6b429a29d3f2ecfc6ea3 465.89 ms 532.40 ms 66.51 ms
89ff369cde4636c0b01906169f4e01586fac204c 449.88 ms 501.09 ms 51.21 ms
89f72ba3c257819e4e71e92789584550ba2c8735 690.31 ms 750.86 ms 60.55 ms
b74ef0c5eca3e9a19e4c83743dcd4e0bcc4143ce 457.74 ms 504.35 ms 46.61 ms
be807efe66b0d18f7dff8e83e16e57140e1ef4a3 447.39 ms 509.94 ms 62.55 ms
adf8466cece26342b98c577f08f354960eefb035 449.17 ms 503.16 ms 54.00 ms
0ca1761f1f8d1563394a77cf5d336fe19fe4ab4a 498.72 ms 550.92 ms 52.20 ms

App size

Revision Plain With Sentry Diff
48c36f9a1dd0b04cdfa20c05451a7e7511ba4bb4 6.49 MiB 7.57 MiB 1.08 MiB
eef08283b9d6b298955040c28c9a16ae12993314 6.49 MiB 7.57 MiB 1.08 MiB
6a2fb35c8a6b0b085859e1c4a5b15e86978560f2 6.49 MiB 7.57 MiB 1.08 MiB
e9b16e8e8b1129df9f7d6b429a29d3f2ecfc6ea3 6.49 MiB 7.57 MiB 1.08 MiB
89ff369cde4636c0b01906169f4e01586fac204c 6.49 MiB 7.57 MiB 1.08 MiB
89f72ba3c257819e4e71e92789584550ba2c8735 6.49 MiB 7.57 MiB 1.08 MiB
b74ef0c5eca3e9a19e4c83743dcd4e0bcc4143ce 6.49 MiB 7.57 MiB 1.08 MiB
be807efe66b0d18f7dff8e83e16e57140e1ef4a3 6.49 MiB 7.57 MiB 1.08 MiB
adf8466cece26342b98c577f08f354960eefb035 6.49 MiB 7.57 MiB 1.08 MiB
0ca1761f1f8d1563394a77cf5d336fe19fe4ab4a 6.49 MiB 7.57 MiB 1.08 MiB
github-actions[bot] commented 1 month ago

iOS Performance metrics :rocket:

  Plain With Sentry Diff
Startup time 1246.48 ms 1262.27 ms 15.79 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d4d08071f7cfa6a1fb4b2a27151bb30a36d607f8 1246.94 ms 1260.69 ms 13.75 ms
f9d18f3411c23172de9bbb769b49dae01834f5e6 1240.20 ms 1242.78 ms 2.57 ms
04bd9e665bd9e8a5f2d1b16c55e4e3fdd18f9b6d 1230.78 ms 1250.71 ms 19.94 ms
051e97a45b67f5c431e8c540585c1786c42670f9 1245.94 ms 1249.51 ms 3.57 ms
b8562d01376a7efe9af802ab0abd9d8226eb4bd7 1249.92 ms 1267.56 ms 17.64 ms
affcf078275df70b443b3150a5ae60951bfa5faa 1240.61 ms 1266.49 ms 25.88 ms
2e8b1e182237a501baa42811cc2cbe335a7bee1b 1247.45 ms 1263.67 ms 16.22 ms
0f067d38500c0ab6b9e85d0b19690ba915d4fea1 1245.71 ms 1269.59 ms 23.88 ms
f1314d57fef996236ceca00725cc9c3acce16d85 1246.46 ms 1270.92 ms 24.46 ms
e239c830ce5177e7392e665600b16745141b4762 1248.40 ms 1269.28 ms 20.89 ms

App size

Revision Plain With Sentry Diff
d4d08071f7cfa6a1fb4b2a27151bb30a36d607f8 8.33 MiB 9.64 MiB 1.31 MiB
f9d18f3411c23172de9bbb769b49dae01834f5e6 8.29 MiB 9.36 MiB 1.07 MiB
04bd9e665bd9e8a5f2d1b16c55e4e3fdd18f9b6d 8.33 MiB 9.61 MiB 1.27 MiB
051e97a45b67f5c431e8c540585c1786c42670f9 8.28 MiB 9.34 MiB 1.06 MiB
b8562d01376a7efe9af802ab0abd9d8226eb4bd7 8.33 MiB 9.54 MiB 1.22 MiB
affcf078275df70b443b3150a5ae60951bfa5faa 8.38 MiB 9.70 MiB 1.33 MiB
2e8b1e182237a501baa42811cc2cbe335a7bee1b 8.33 MiB 9.64 MiB 1.31 MiB
0f067d38500c0ab6b9e85d0b19690ba915d4fea1 8.32 MiB 9.52 MiB 1.20 MiB
f1314d57fef996236ceca00725cc9c3acce16d85 8.10 MiB 9.08 MiB 1004.30 KiB
e239c830ce5177e7392e665600b16745141b4762 8.38 MiB 9.74 MiB 1.36 MiB

Previous results on branch: feat/stack-frame-excludes

Startup times

Revision Plain With Sentry Diff
48c36f9a1dd0b04cdfa20c05451a7e7511ba4bb4 1249.63 ms 1264.59 ms 14.96 ms
be807efe66b0d18f7dff8e83e16e57140e1ef4a3 1238.78 ms 1250.96 ms 12.18 ms
89f72ba3c257819e4e71e92789584550ba2c8735 1253.04 ms 1285.69 ms 32.65 ms
0ca1761f1f8d1563394a77cf5d336fe19fe4ab4a 1243.71 ms 1272.46 ms 28.74 ms
adf8466cece26342b98c577f08f354960eefb035 1233.16 ms 1243.47 ms 10.31 ms
6a2fb35c8a6b0b085859e1c4a5b15e86978560f2 1237.61 ms 1245.08 ms 7.47 ms
eef08283b9d6b298955040c28c9a16ae12993314 1250.47 ms 1271.85 ms 21.38 ms
89ff369cde4636c0b01906169f4e01586fac204c 1238.06 ms 1254.94 ms 16.88 ms
b74ef0c5eca3e9a19e4c83743dcd4e0bcc4143ce 1256.04 ms 1281.37 ms 25.33 ms

App size

Revision Plain With Sentry Diff
48c36f9a1dd0b04cdfa20c05451a7e7511ba4bb4 8.38 MiB 9.75 MiB 1.37 MiB
be807efe66b0d18f7dff8e83e16e57140e1ef4a3 8.38 MiB 9.75 MiB 1.37 MiB
89f72ba3c257819e4e71e92789584550ba2c8735 8.38 MiB 9.75 MiB 1.37 MiB
0ca1761f1f8d1563394a77cf5d336fe19fe4ab4a 8.38 MiB 9.75 MiB 1.37 MiB
adf8466cece26342b98c577f08f354960eefb035 8.38 MiB 9.75 MiB 1.37 MiB
6a2fb35c8a6b0b085859e1c4a5b15e86978560f2 8.38 MiB 9.75 MiB 1.37 MiB
eef08283b9d6b298955040c28c9a16ae12993314 8.38 MiB 9.75 MiB 1.37 MiB
89ff369cde4636c0b01906169f4e01586fac204c 8.38 MiB 9.75 MiB 1.37 MiB
b74ef0c5eca3e9a19e4c83743dcd4e0bcc4143ce 8.38 MiB 9.75 MiB 1.37 MiB
buenaflor commented 1 month ago

what about sdk crashes/errors? If a real sdk crash/error happened we wouldn't know because the frames would be removed, no?

the main reason why we re-enabled it a couple months ago was to catch sdk errors

denrase commented 1 month ago

@buenaflor We should somehow verify that we are not involuntarily breaking SDK crash detection, do you have any idea how?

buenaflor commented 1 month ago

don't think we can directly test it but we can do a sanity check: https://github.com/getsentry/sentry/blob/master/src/sentry/utils/sdk_crashes/sdk_crash_detection_config.py#L288C1-L302C1 if these paths are contained in one of the stacktraces we ingest it as a sdk crash/error

one edge case is if we throw an unhandled error from the sdk and for some reason flutter doesn't provide a stacktrace, we try to capture a stacktrace with StackTrace.current and then remove all sentry frames, then we wouldn't be able to capture the crash detection in that case.

denrase commented 1 month ago

@buenaflor Checked wit some flutter sdk errors. Works as expected, sentry frames are removed.

FlutterError.reportError(
  FlutterErrorDetails(exception: FlutterError.fromParts(
    <DiagnosticsNode>[
      ErrorSummary(
        'The Element for ${toStringShort()} cannot be inserted into slot "$slot" of its ancestor. ',
      ),
      ErrorDescription(
        'The ownership chain for the Element in question was:\n  ${debugGetCreatorChain(10)}',
      ),
      ErrorDescription(
        'This Element allows the creation of multiple independent render trees, which cannot '
        'be attached to an ancestor in an existing render tree. However, an ancestor RenderObject '
        'is expecting that a child will be attached.'
      ),
      ErrorHint(
        'Try moving the subtree that contains the ${toStringShort()} widget into the '
        'view property of a ViewAnchor widget or to the root of the widget tree, where '
        'it is not expected to attach its RenderObject to its ancestor.',
      ),
    ],
  )),
);

https://github.com/flutter/flutter/blob/47035a0102339c0464b1866a09cd6a25e86922d7/packages/flutter/lib/src/widgets/view.dart#L784

https://sentry-sdks.sentry.io/issues/3856107391/events/latest/?project=5428562&query=&referrer=latest-event&sort=date&statsPeriod=1h&stream_index=0

FlutterError.reportError(
  const FlutterErrorDetails(
    exception:
      'Both Router.navigate and Router.neglect have been called in this '
      'build cycle, and the Router cannot decide whether to report the '
      'route information. Please make sure only one of them is called '
      'within the same build cycle.',
  ),
);

https://github.com/flutter/flutter/blob/47035a0102339c0464b1866a09cd6a25e86922d7/packages/flutter/lib/src/widgets/router.dart#L674

https://sentry-sdks.sentry.io/issues/6008675625/?project=5428562&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=1

FlutterError.reportError(
      FlutterErrorDetails(
        exception: FlutterError('A $runtimeType overflowed by $overflowText.'),
        library: 'rendering library',
        context: ErrorDescription('during layout'),
        informationCollector: () => <DiagnosticsNode>[
          // debugCreator should only be set in DebugMode, but we want the
          // treeshaker to know that.
          if (kDebugMode && debugCreator != null)
            DiagnosticsDebugCreator(debugCreator!),
          ...overflowHints!,
          describeForError('The specific $runtimeType in question is'),
          // TODO(jacobr): this line is ascii art that it would be nice to
          // handle a little more generically in GUI debugging clients in the
          // future.
          DiagnosticsNode.message('◢◤' * (FlutterError.wrapWidth ~/ 2), allowWrap: false),
        ],
      ),
    );

https://github.com/flutter/flutter/blob/47035a0102339c0464b1866a09cd6a25e86922d7/packages/flutter/lib/src/rendering/debug_overflow_indicator.dart#L246

https://sentry-sdks.sentry.io/issues/3856107391/events/latest/?project=5428562&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=latest-event&sort=date&statsPeriod=1h&stream_index=0

denrase commented 1 month ago

one edge case is if we throw an unhandled error from the sdk and for some reason flutter doesn't provide a stacktrace, we try to capture a stacktrace with StackTrace.current and then remove all sentry frames, then we wouldn't be able to capture the crash detection in that case.

We could only avoid this if we only remove the stacktraces if the errors originiated in our error integrations, and not for any error where we fall back to StackTrace.current.

buenaflor commented 1 month ago

@denrase if we throw an unhandled error within the SDK it is forwarded to our FlutterError.onError integration and I'm not sure if there are cases where flutter does not provide a stacktrace, I'll also check out if there are such cases in our SDK

And for the rest where we fall back to Stacktrace.current, those happen if a user tries to do captureException or captureEvent without a stacktrace or captureMessage e.g

// no stacktrace attached so we fallback to Stacktrace.current in stacktrace factory
Sentry.captureException(exception)
Sentry.captureMessage('msg')

and that's also where the sentry frames are added that are misleading because it captures all sdk function calls until Stacktrace.current

denrase commented 1 month ago

@buenaflor Threw an exception in sentry. It does get caught in on_error_integration.dart and has a stack trace. Since the callback of this integration always has a non-null stack trace, it should at least always be there, but we don't knwo if this could also be StackTrace.empty.

Bildschirmfoto 2024-10-28 um 10 57 56

The case where users call Sentry.captureException and Sentry.captureMessage, we remove sentry frames as well, so this case is covered.

denrase commented 1 month ago

@buenaflor We also catch errors thrown in the SDK in various parts of the SDK and call the logger with them, so they will never reach our backend anyway AFAIK.

try {
  sentryId = await item.client.captureFeedback(
    feedback,
    hint: hint,
    scope: scope,
  );
} catch (exception, stacktrace) {
  _options.logger(
    SentryLevel.error,
    'Error while capturing feedback',
    exception: exception,
    stackTrace: stacktrace,
  );
}
buenaflor commented 1 month ago

yeah, tbh I think this is fine.

I'll do some final tests and then we can wrap it up if it looks good

buenaflor commented 1 month ago

my only concern is: there will be users complaining that they now have an empty stacktrace for unhandled errors where FlutterError.onError doesn't provide one OR the stacktrace provided is not very in-depth because we synthetically create it. I somehow want to make it clear that this is intended behaviour because Flutter doesn't always provide a stacktrace (most users don't know this) and we are doing best effort to provide useful info. does it make sense to add some kind of context to the event to clarify this? or something similar.

imo that would be useful and helps the user understand why this is happening and we don't have to explain it to them all the time

cc @kahest maybe you have insight or an idea how we could surface this to the user clearly

kahest commented 1 month ago

cc @kahest maybe you have insight or an idea how we could surface this to the user clearly

We can show something directly on the issue though not sure what's involved to do that. But maybe calling this out in release notes and docs is enough?

buenaflor commented 2 weeks ago

Let's make it clear in the changelog here and update the sentry-docs in flutter/troubleshooting

I'll take care of that since I'm currently updating some docs anyway