Skip to content

v9: Parent-child relationship for the PlatformExceptions and Cause #2803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
Apr 10, 2025

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Mar 18, 2025

📜 Description

  • Introduce child relationships (exceptions) in SentryExceptions, so we can preserve nested relationships.
  • Flatten exceptions into list with ids and parent references according to RFC.

💡 Motivation and Context

Closes #1859

💚 How did you test it?

📝 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

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 87.63%. Comparing base (3f8357e) to head (80e7f9e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...or/android_platform_exception_event_processor.dart 62.50% 18 Missing ⚠️
...cessor/exception/io_exception_event_processor.dart 73.33% 4 Missing ⚠️
dart/lib/src/protocol/sentry_exception.dart 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2803      +/-   ##
==========================================
- Coverage   87.78%   87.63%   -0.16%     
==========================================
  Files         269      270       +1     
  Lines        8971     9032      +61     
==========================================
+ Hits         7875     7915      +40     
- Misses       1096     1117      +21     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented Mar 18, 2025

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 509.69 ms 574.29 ms 64.60 ms
Size 6.44 MiB 7.43 MiB 1010.28 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
873fb42 352.10 ms 397.43 ms 45.33 ms
d1488a1 516.78 ms 585.04 ms 68.27 ms
84c28cd 415.11 ms 549.44 ms 134.33 ms
94757e3 480.21 ms 515.28 ms 35.07 ms
dd5521e 454.51 ms 538.29 ms 83.78 ms
d189e01 328.67 ms 397.12 ms 68.45 ms
be173fa 375.94 ms 458.36 ms 82.42 ms
3751dbc 486.20 ms 538.68 ms 52.48 ms
d53c6fa 282.83 ms 344.00 ms 61.17 ms
1b66358 314.96 ms 363.39 ms 48.43 ms

App size

Revision Plain With Sentry Diff
873fb42 5.94 MiB 6.96 MiB 1.02 MiB
d1488a1 6.46 MiB 7.48 MiB 1.03 MiB
84c28cd 6.49 MiB 7.56 MiB 1.07 MiB
94757e3 6.49 MiB 7.57 MiB 1.08 MiB
dd5521e 6.44 MiB 7.50 MiB 1.06 MiB
d189e01 6.16 MiB 7.14 MiB 1009.90 KiB
be173fa 6.35 MiB 7.33 MiB 1005.54 KiB
3751dbc 6.49 MiB 7.55 MiB 1.06 MiB
d53c6fa 6.16 MiB 7.14 MiB 1011.18 KiB
1b66358 6.06 MiB 7.09 MiB 1.03 MiB

Previous results on branch: enha/rel-platfomr-cause

Startup times

Revision Plain With Sentry Diff
9a23c4a 453.27 ms 521.45 ms 68.18 ms
e26f4d4 461.52 ms 526.23 ms 64.71 ms
b950043 444.28 ms 505.20 ms 60.91 ms
972a9b7 444.48 ms 488.96 ms 44.48 ms
83081be 452.27 ms 517.54 ms 65.27 ms
2e189f2 526.22 ms 550.76 ms 24.54 ms
4adb170 485.06 ms 528.74 ms 43.68 ms

App size

Revision Plain With Sentry Diff
9a23c4a 6.44 MiB 7.56 MiB 1.12 MiB
e26f4d4 6.44 MiB 7.43 MiB 1010.29 KiB
b950043 6.44 MiB 7.56 MiB 1.12 MiB
972a9b7 6.44 MiB 7.43 MiB 1010.42 KiB
83081be 6.44 MiB 7.57 MiB 1.13 MiB
2e189f2 6.44 MiB 7.56 MiB 1.12 MiB
4adb170 6.44 MiB 7.43 MiB 1011.07 KiB

Copy link
Contributor

github-actions bot commented Mar 18, 2025

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.12 ms 1257.04 ms 12.92 ms
Size 8.43 MiB 10.00 MiB 1.57 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f25f207 1233.44 ms 1260.73 ms 27.30 ms
280ab9f 1255.71 ms 1262.78 ms 7.06 ms
ef2f368 1259.12 ms 1277.04 ms 17.92 ms
ed2ae08 1222.10 ms 1226.57 ms 4.47 ms
bd69c1b 1258.09 ms 1282.46 ms 24.37 ms
33d0587 1262.16 ms 1270.50 ms 8.34 ms
bc29768 1247.25 ms 1268.63 ms 21.38 ms
3e5078c 1239.73 ms 1248.69 ms 8.96 ms
90a08ea 1260.45 ms 1285.24 ms 24.80 ms
c19bfb6 1236.06 ms 1259.57 ms 23.51 ms

App size

Revision Plain With Sentry Diff
f25f207 8.32 MiB 9.38 MiB 1.05 MiB
280ab9f 8.29 MiB 9.39 MiB 1.10 MiB
ef2f368 8.15 MiB 9.10 MiB 965.24 KiB
ed2ae08 8.28 MiB 9.34 MiB 1.06 MiB
bd69c1b 8.43 MiB 9.98 MiB 1.55 MiB
33d0587 8.29 MiB 9.38 MiB 1.09 MiB
bc29768 8.32 MiB 9.38 MiB 1.05 MiB
3e5078c 8.28 MiB 9.34 MiB 1.06 MiB
90a08ea 8.38 MiB 9.73 MiB 1.36 MiB
c19bfb6 8.34 MiB 9.65 MiB 1.31 MiB

Previous results on branch: enha/rel-platfomr-cause

Startup times

Revision Plain With Sentry Diff
b950043 1257.96 ms 1280.29 ms 22.33 ms
9a23c4a 1260.02 ms 1282.49 ms 22.47 ms
972a9b7 1265.10 ms 1278.61 ms 13.51 ms
e26f4d4 1253.73 ms 1263.14 ms 9.41 ms
4adb170 1247.20 ms 1251.44 ms 4.23 ms
2e189f2 1250.57 ms 1277.29 ms 26.71 ms
83081be 1265.35 ms 1278.00 ms 12.65 ms

App size

Revision Plain With Sentry Diff
b950043 8.43 MiB 9.98 MiB 1.55 MiB
9a23c4a 8.43 MiB 9.98 MiB 1.55 MiB
972a9b7 8.43 MiB 10.00 MiB 1.56 MiB
e26f4d4 8.43 MiB 10.00 MiB 1.57 MiB
4adb170 8.43 MiB 10.00 MiB 1.57 MiB
2e189f2 8.43 MiB 9.98 MiB 1.55 MiB
83081be 8.43 MiB 9.99 MiB 1.56 MiB

@denrase denrase marked this pull request as ready for review March 27, 2025 11:10
Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! only got couple questionos

@buenaflor
Copy link
Contributor

Couple of merge conflicts are here, prolly because of the mutable data PR

# Conflicts:
#	dart/lib/src/event_processor/exception/io_exception_event_processor.dart
#	dart/lib/src/protocol/sentry_exception.dart
#	dart/lib/src/sentry_client.dart
#	flutter/example/integration_test/integration_test.dart
#	flutter/lib/src/event_processor/android_platform_exception_event_processor.dart
@denrase
Copy link
Collaborator Author

denrase commented Apr 7, 2025

@buenaflor So i tested on Android emulator. While both exceptions are shown, the order is reversed. Which would suggest that the relationships build here are ignored, no? The json has the references in the mechanism, so this should all be correct. Is this feature activated on sentry.io?

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

Bildschirmfoto 2025-04-07 um 14 08 28 Bildschirmfoto 2025-04-07 um 14 10 06

@denrase denrase requested a review from buenaflor April 7, 2025 12:16
@buenaflor
Copy link
Contributor

buenaflor commented Apr 8, 2025

@denrase not sure how the order is supposed to be

but regarding the UI, it should look like this (or used to)

image

I triggered the platform exception with the implementation of this PR #1998

Not sure if they removed this UI change

@buenaflor
Copy link
Contributor

buenaflor commented Apr 8, 2025

From the docs: https://develop.sentry.dev/sdk/data-model/event-payloads/exception/
Multiple values represent chained exceptions and should be sorted oldest to newest so I believe the second picture is correct? runtime exception -> platformexception

@denrase there's also a couple dartt analyzer warnings

@denrase
Copy link
Collaborator Author

denrase commented Apr 8, 2025

@buenaflor So the first picture is according to the RFC, where the root is the last in the list.

@buenaflor
Copy link
Contributor

as discussed let's check the mentioned PR if we added something wrong to make the UI look like in my comment

@denrase
Copy link
Collaborator Author

denrase commented Apr 9, 2025

"exception":
{
    "values":
    [
        {
            "type": "PlatformException",
            "value": "PlatformException(error, Catch this platform exception!, null, java.lang.RuntimeException: Catch this platform exception!\n\tat io.sentry.samples.flutter.MainActivity$configureFlutterEngine$1.onMethodCall(MainActivity.kt:38)\n\tat io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler.onMessage(MethodChannel.java:267)\n\tat io.flutter.embedding.engine.dart.DartMessenger.invokeHandler(DartMessenger.java:292)\n\tat io.flutter.embedding.engine.dart.DartMessenger.lambda$dispatchMessageToQueue$0$io-flutter-embedding-engine-dart-DartMessenger(DartMessenger.java:319)\n\tat io.flutter.embedding.engine.dart.DartMessenger$$ExternalSyntheticLambda0.run(Unknown Source:12)\n\tat android.os.Handler.handleCallback(Handler.java:942)\n\tat android.os.Handler.dispatchMessage(Handler.java:99)\n\tat android.os.Looper.loopOnce(Looper.java:201)\n\tat android.os.Looper.loop(Looper.java:288)\n\tat android.app.ActivityThread.main(ActivityThread.java:7924)\n\tat java.lang.reflect.Method.invoke(Native Method)\n\tat com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)\n\tat com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)\n)",
            "mechanism":
            {
                "type": "chained",
                "exception_id": 1,
                "parent_id": 0,
            }
        },
        {
            "type": "RuntimeException",
            "value": "Catch this platform exception!",
            "mechanism":
            {
                "type": "sentry-flutter",
                "is_exception_group": true,
                "exception_id": 0
            }
        }
    ]
},

Above is what we get from the PR of @ueman. We have a PlatformExcetion, which contains an exception that occured in Android. We parse this either from details or stackTrace:

PlatformException(
  details: null,
  stackTrace: RuntimeException(...)
)

The RuntimeException is the innermost, so in this example it is the root and last in order.

PlatformException(id: 1, parent: 0), RuntimeException(id: 0, parent: null),

What does not make sense to me is is_exception_group: true in this example, as there are not multiple children on the same level. But only if we set this value, we have the UI which shows hierarchy.

Bildschirmfoto 2025-04-09 um 14 23 21

In tests we have the case where both details and stackTrace:

PlatformException(
  details: SomeException(...),
  stackTrace: RuntimeException(...)
)

I'm not sure that this is a case that occures, but since we have a test case for it, I'm assuming that we need to handle it.

But what would in this case the structure look like? Initially, I have implemented it the other way around, where PlatformException is the root and also a group, as it contains two children on the same level:

RuntimeException(id: 2, parent: 0), SomeException(id: 1, parent: 0), PlatformException(id: 0, parent: null),

But seeing the other PR, I don't think that is correct.

So in summary:

@buenaflor
Copy link
Contributor

buenaflor commented Apr 9, 2025

based on JS and Python impl the root / parent should be marked with is_exception_group: true, in this case the PlatformException

Comment on lines +91 to +92
stackTrace: stackTrace ?? this.stackTrace?.copyWith(),
mechanism: mechanism ?? this.mechanism?.copyWith(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the copyWith?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the tests, yeah. There we use copy with and without this, we'd just share the same instance of stackTrace/mecahnism between multiple tests, now that it's immutable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to fix this? since copyWith is deprecated we will remove it at some point

but if it's too much work it's fine, we can do it when we eventually remove it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it for now and handle this some other time

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got 1 last question but lgtm 👍

remove newline

Co-authored-by: Giancarlo Buenaflor <[email protected]>
@denrase denrase requested a review from buenaflor April 10, 2025 11:22
@ueman
Copy link
Collaborator

ueman commented Apr 10, 2025

But if so, i don't know how to build the hierarchy with both details and stackTrace populated with an error.

FWIW, that's not a case I've ever seen in production. It's either in details or in stackTrace, but not both. I would suggest to not handle the case where both contain an exception and wait until someone complains and then care about it.

@buenaflor buenaflor merged commit 6edf8b8 into main Apr 10, 2025
150 of 154 checks passed
@buenaflor buenaflor deleted the enha/rel-platfomr-cause branch April 10, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v9] Parent-child relationship for the PlatformExceptions and Cause
3 participants