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

denrase · 2024-10-14T14:17:42Z

📜 Description

SDK removes sentry frames if the SDK falls back to StackTrace.current

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

Before

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

💡 Motivation and Context

Relates to #2268

💚 How did you test it?

Unit tests

📝 Checklist

I reviewed submitted code
I added tests to verify changes
No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
I updated the docs if needed
All tests passing
No breaking changes

🔮 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 · 2024-10-14T14:18:37Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 5ddd2af

codecov · 2024-10-14T14:19:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-10-14T14:26:32Z

Android Performance metrics 🚀

	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
`e5b744f`	302.70 ms	342.17 ms	39.47 ms
`0ac1eed`	370.60 ms	441.54 ms	70.94 ms
`90a08ea`	477.25 ms	534.10 ms	56.85 ms
`0db91cc`	327.85 ms	387.31 ms	59.46 ms
`061fed2`	434.11 ms	506.49 ms	72.38 ms
`4829ad3`	381.55 ms	455.45 ms	73.90 ms
`519423f`	357.00 ms	415.77 ms	58.77 ms
`ddc97ad`	331.45 ms	384.06 ms	52.61 ms
`e3ef570`	389.71 ms	459.16 ms	69.45 ms
`2d3b03d`	309.53 ms	353.40 ms	43.87 ms

App size

Revision	Plain	With Sentry	Diff
`e5b744f`	6.06 MiB	7.09 MiB	1.03 MiB
`0ac1eed`	6.06 MiB	7.03 MiB	990.44 KiB
`90a08ea`	6.49 MiB	7.55 MiB	1.06 MiB
`0db91cc`	5.94 MiB	6.95 MiB	1.01 MiB
`061fed2`	6.52 MiB	7.59 MiB	1.06 MiB
`4829ad3`	6.33 MiB	7.26 MiB	943.11 KiB
`519423f`	6.06 MiB	7.03 MiB	989.24 KiB
`ddc97ad`	6.16 MiB	7.14 MiB	1003.75 KiB
`e3ef570`	6.33 MiB	7.26 MiB	950.38 KiB
`2d3b03d`	6.06 MiB	7.09 MiB	1.03 MiB

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

Startup times

Revision	Plain	With Sentry	Diff
`48c36f9`	455.06 ms	498.96 ms	43.89 ms
`eef0828`	454.86 ms	479.50 ms	24.64 ms
`6a2fb35`	452.60 ms	471.21 ms	18.61 ms
`e9b16e8`	465.89 ms	532.40 ms	66.51 ms
`89ff369`	449.88 ms	501.09 ms	51.21 ms
`89f72ba`	690.31 ms	750.86 ms	60.55 ms
`b74ef0c`	457.74 ms	504.35 ms	46.61 ms
`be807ef`	447.39 ms	509.94 ms	62.55 ms
`adf8466`	449.17 ms	503.16 ms	54.00 ms
`0ca1761`	498.72 ms	550.92 ms	52.20 ms

App size

Revision	Plain	With Sentry	Diff
`48c36f9`	6.49 MiB	7.57 MiB	1.08 MiB
`eef0828`	6.49 MiB	7.57 MiB	1.08 MiB
`6a2fb35`	6.49 MiB	7.57 MiB	1.08 MiB
`e9b16e8`	6.49 MiB	7.57 MiB	1.08 MiB
`89ff369`	6.49 MiB	7.57 MiB	1.08 MiB
`89f72ba`	6.49 MiB	7.57 MiB	1.08 MiB
`b74ef0c`	6.49 MiB	7.57 MiB	1.08 MiB
`be807ef`	6.49 MiB	7.57 MiB	1.08 MiB
`adf8466`	6.49 MiB	7.57 MiB	1.08 MiB
`0ca1761`	6.49 MiB	7.57 MiB	1.08 MiB

github-actions · 2024-10-14T14:31:23Z

iOS Performance metrics 🚀

	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
`d4d0807`	1246.94 ms	1260.69 ms	13.75 ms
`f9d18f3`	1240.20 ms	1242.78 ms	2.57 ms
`04bd9e6`	1230.78 ms	1250.71 ms	19.94 ms
`051e97a`	1245.94 ms	1249.51 ms	3.57 ms
`b8562d0`	1249.92 ms	1267.56 ms	17.64 ms
`affcf07`	1240.61 ms	1266.49 ms	25.88 ms
`2e8b1e1`	1247.45 ms	1263.67 ms	16.22 ms
`0f067d3`	1245.71 ms	1269.59 ms	23.88 ms
`f1314d5`	1246.46 ms	1270.92 ms	24.46 ms
`e239c83`	1248.40 ms	1269.28 ms	20.89 ms

App size

Revision	Plain	With Sentry	Diff
`d4d0807`	8.33 MiB	9.64 MiB	1.31 MiB
`f9d18f3`	8.29 MiB	9.36 MiB	1.07 MiB
`04bd9e6`	8.33 MiB	9.61 MiB	1.27 MiB
`051e97a`	8.28 MiB	9.34 MiB	1.06 MiB
`b8562d0`	8.33 MiB	9.54 MiB	1.22 MiB
`affcf07`	8.38 MiB	9.70 MiB	1.33 MiB
`2e8b1e1`	8.33 MiB	9.64 MiB	1.31 MiB
`0f067d3`	8.32 MiB	9.52 MiB	1.20 MiB
`f1314d5`	8.10 MiB	9.08 MiB	1004.30 KiB
`e239c83`	8.38 MiB	9.74 MiB	1.36 MiB

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

Startup times

Revision	Plain	With Sentry	Diff
`48c36f9`	1249.63 ms	1264.59 ms	14.96 ms
`be807ef`	1238.78 ms	1250.96 ms	12.18 ms
`89f72ba`	1253.04 ms	1285.69 ms	32.65 ms
`0ca1761`	1243.71 ms	1272.46 ms	28.74 ms
`adf8466`	1233.16 ms	1243.47 ms	10.31 ms
`6a2fb35`	1237.61 ms	1245.08 ms	7.47 ms
`eef0828`	1250.47 ms	1271.85 ms	21.38 ms
`89ff369`	1238.06 ms	1254.94 ms	16.88 ms
`b74ef0c`	1256.04 ms	1281.37 ms	25.33 ms

App size

Revision	Plain	With Sentry	Diff
`48c36f9`	8.38 MiB	9.75 MiB	1.37 MiB
`be807ef`	8.38 MiB	9.75 MiB	1.37 MiB
`89f72ba`	8.38 MiB	9.75 MiB	1.37 MiB
`0ca1761`	8.38 MiB	9.75 MiB	1.37 MiB
`adf8466`	8.38 MiB	9.75 MiB	1.37 MiB
`6a2fb35`	8.38 MiB	9.75 MiB	1.37 MiB
`eef0828`	8.38 MiB	9.75 MiB	1.37 MiB
`89ff369`	8.38 MiB	9.75 MiB	1.37 MiB
`b74ef0c`	8.38 MiB	9.75 MiB	1.37 MiB

buenaflor · 2024-10-15T10:58:50Z

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 · 2024-10-17T11:27:03Z

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

dart/test/sentry_exception_factory_test.dart

buenaflor · 2024-10-17T12:11:18Z

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 · 2024-10-21T09:19:33Z

@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 · 2024-10-21T09:23:32Z

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 · 2024-10-21T10:14:30Z

@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 · 2024-10-28T10:10:38Z

@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.

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

denrase · 2024-10-28T10:15:13Z

@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 · 2024-10-28T10:23:48Z

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 · 2024-10-29T13:20:45Z

dart/lib/src/sentry_client.dart

@@ -280,8 +284,14 @@ class SentryClient {
    // therefore add it to the threads.
    // https://develop.sentry.dev/sdk/event-payloads/stacktrace/
    if (stackTrace != null || _options.attachStacktrace) {


I know this is not part of this PR but isn't the attachStacktrace option basically useless because it will still add teh stacktrace if there is one even though a user might've set attachStacktrace to false.

We don't have to address this here but I just find it strange

The check was introduced here.

And with the change in #524 the fallback thread was moved from the event to the threads.

Reading the discription for attachThreads, I think we should only attach it here if it is set to true. It's a breaking change, as it affects grouping. This feature is also opt-out, so it will not affect as many users.

Seeing that @rxlabz and @ueman were involved, maybe they have some additional insight for us?

buenaflor · 2024-10-29T13:23:38Z

dart/lib/src/sentry_stack_trace_factory.dart

+          if (removeSentryFrames == true &&
+              (stackTraceFrame.package == 'sentry' ||
+                  stackTraceFrame.package == 'sentry_flutter')) {
+            continue;


it's not only limited to 'sentry' and 'sentry_flutter'

see here: https://github.com/getsentry/sentry-dart/blob/f754e867516d18d38272967acc03289a4ff83c0c/dart/lib/src/sentry_stack_trace_factory.dart#L18C1-L29C1 (this has been removed in the current code, this links to an older commit)

In this case only those are relevant, as we just want to remove those frames that originate from creating the fallback stack-trace, no?

buenaflor · 2024-10-29T13:26:59Z

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 · 2024-10-31T09:46:28Z

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 · 2024-11-11T12:38:35Z

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

…github.com/getsentry/sentry-dart into feat/redact-screenshots-via-view-hierarchy * 'feat/redact-screenshots-via-view-hierarchy' of https://github.com/getsentry/sentry-dart: build(deps): bump ruby/setup-ruby from 1.199.0 to 1.202.0 (#2403) Remove `sentry` frames if SDK falls back to current stack trace (#2351) fix: load contexts not setting the user for transactions (#2395) feat: improve frame tracking accuracy (#2372) build(sample): update kotlin version (#2398)

denrase added 2 commits October 14, 2024 16:14

Add stackFrameExcludes to SentryOptions

7ce2108

Merge branch 'main' into feat/stack-frame-excludes

adf8466

update cl

45a99cb

denrase marked this pull request as ready for review October 14, 2024 14:35

denrase requested review from krystofwoldrich, stefanosiano, buenaflor and martinhaintz as code owners October 14, 2024 14:35

fix failing test expectations

be807ef

denrase added 2 commits October 17, 2024 13:12

Remove sentry frames if SDK falls back to currentStackTrace

7690750

Merge branch 'main' into feat/stack-frame-excludes

89f72ba

denrase changed the title ~~Add stackFrameExcludes to SentryOptions~~ Remove sentry frames if SDK falls back to currentStackTrace Oct 17, 2024

update cl

3e0b272

denrase changed the title ~~Remove sentry frames if SDK falls back to currentStackTrace~~ Remove sentry frames if SDK falls back to current stack trace Oct 17, 2024

fix cl

0054c9d

denrase commented Oct 17, 2024

View reviewed changes

dart/test/sentry_exception_factory_test.dart Show resolved Hide resolved

Add hint in flutter error integration

6a2fb35

update test expectations

b74ef0c

denrase added 2 commits October 21, 2024 13:43

cleanup

89ff369

fix import

48c36f9

buenaflor and others added 2 commits October 24, 2024 13:17

Merge branch 'main' into feat/stack-frame-excludes

0ca1761

Merge branch 'main' into feat/stack-frame-excludes

eef0828

buenaflor reviewed Oct 29, 2024

View reviewed changes

denrase and others added 2 commits November 5, 2024 11:15

Merge branch 'main' into feat/stack-frame-excludes

e9b16e8

Merge branch 'main' into feat/stack-frame-excludes

6bb1d53

buenaflor and others added 5 commits November 11, 2024 13:56

Update CHANGELOG.md

d72452a

Update CHANGELOG.md

47e1472

Merge branch 'main' into feat/stack-frame-excludes

9cd7166

Merge branch 'main' into feat/stack-frame-excludes

65e93be

update cl

5ddd2af

denrase requested a review from buenaflor November 11, 2024 15:44

buenaflor approved these changes Nov 11, 2024

View reviewed changes

buenaflor merged commit 78baa40 into main Nov 11, 2024
135 of 136 checks passed

buenaflor deleted the feat/stack-frame-excludes branch November 11, 2024 22:53

buenaflor mentioned this pull request Nov 12, 2024

stacktrace_utils.dart 位置 getCurrentStackTrace 行 10 在 sentry #2268

Closed

buenaflor mentioned this pull request Dec 10, 2024

FormatException: getCurrentStackTrace throws inside the package #2468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

denrase commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

github-actions bot commented Oct 14, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

buenaflor commented Oct 15, 2024 •

edited

Loading

denrase commented Oct 17, 2024

buenaflor commented Oct 17, 2024

denrase commented Oct 21, 2024

denrase commented Oct 21, 2024

buenaflor commented Oct 21, 2024

denrase commented Oct 28, 2024

denrase commented Oct 28, 2024 •

edited

Loading

buenaflor commented Oct 28, 2024

buenaflor Oct 29, 2024

denrase Oct 30, 2024

buenaflor Oct 29, 2024 •

edited

Loading

denrase Oct 30, 2024

buenaflor commented Oct 29, 2024 •

edited

Loading

kahest commented Oct 31, 2024

buenaflor commented Nov 11, 2024

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

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

Conversation

denrase commented Oct 14, 2024 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 14, 2024 • edited Loading

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 14, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

github-actions bot commented Oct 14, 2024 • edited Loading

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

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

Startup times

App size

buenaflor commented Oct 15, 2024 • edited Loading

denrase commented Oct 17, 2024

buenaflor commented Oct 17, 2024

denrase commented Oct 21, 2024

denrase commented Oct 21, 2024

buenaflor commented Oct 21, 2024

denrase commented Oct 28, 2024

denrase commented Oct 28, 2024 • edited Loading

buenaflor commented Oct 28, 2024

buenaflor Oct 29, 2024

Choose a reason for hiding this comment

denrase Oct 30, 2024

Choose a reason for hiding this comment

buenaflor Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

denrase Oct 30, 2024

Choose a reason for hiding this comment

buenaflor commented Oct 29, 2024 • edited Loading

kahest commented Oct 31, 2024

buenaflor commented Nov 11, 2024

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

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

denrase commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

buenaflor commented Oct 15, 2024 •

edited

Loading

denrase commented Oct 28, 2024 •

edited

Loading

buenaflor Oct 29, 2024 •

edited

Loading

buenaflor commented Oct 29, 2024 •

edited

Loading