Skip to content
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

Feat/only send debug images referenced in the stacktrace for events #2329

Conversation

martinhaintz
Copy link
Collaborator

@martinhaintz martinhaintz commented Oct 7, 2024

📜 Description

Instead of sending all debug images in case of errors, we only want to send the referenced debug images of the stacktrace to be sent.

💡 Motivation and Context

close #1755

💚 How did you test it?

build local with flutter build macos --release --obfuscate --split-debug-info=./ and looked at sentry for the imageadresses. Then run flutter packages pub run sentry_dart_plugin and looked for further info in sentry.

📝 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
Contributor

github-actions bot commented Oct 7, 2024

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

Generated by 🚫 dangerJS against 1c07b01

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.89%. Comparing base (18e6fd7) to head (1c07b01).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
flutter/lib/src/native/sentry_native_channel.dart 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2329      +/-   ##
==========================================
+ Coverage   85.06%   85.89%   +0.83%     
==========================================
  Files         257       81     -176     
  Lines        9191     2872    -6319     
==========================================
- Hits         7818     2467    -5351     
+ Misses       1373      405     -968     

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

@martinhaintz
Copy link
Collaborator Author

@buenaflor any ideas, what would be the best way to test this? print and log are disabled in Flutter for release builds.

Copy link
Contributor

github-actions bot commented Oct 7, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 500.45 ms 519.46 ms 19.01 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2966d88 365.71 ms 422.50 ms 56.79 ms
affcf07 446.19 ms 476.32 ms 30.13 ms
fcd1ee4 298.96 ms 376.04 ms 77.09 ms
b47809a 333.42 ms 368.36 ms 34.95 ms
c1bb00f 303.77 ms 371.88 ms 68.11 ms
f770c4c 385.88 ms 449.86 ms 63.98 ms
3d305b9 403.55 ms 469.76 ms 66.20 ms
ea60f10 456.23 ms 494.48 ms 38.25 ms
8609bd8 359.76 ms 437.40 ms 77.64 ms
7748b7c 356.37 ms 426.70 ms 70.33 ms

App size

Revision Plain With Sentry Diff
2966d88 6.33 MiB 7.27 MiB 954.11 KiB
affcf07 6.52 MiB 7.59 MiB 1.06 MiB
fcd1ee4 6.16 MiB 7.13 MiB 1000.85 KiB
b47809a 5.94 MiB 6.96 MiB 1.02 MiB
c1bb00f 6.06 MiB 7.09 MiB 1.03 MiB
f770c4c 6.33 MiB 7.26 MiB 950.37 KiB
3d305b9 6.35 MiB 7.35 MiB 1021.16 KiB
ea60f10 6.49 MiB 7.55 MiB 1.07 MiB
8609bd8 6.27 MiB 7.20 MiB 959.07 KiB
7748b7c 6.27 MiB 7.20 MiB 959.09 KiB

Previous results on branch: feat/only-send-debug-images-referenced-in-the-stacktrace-for-events

Startup times

Revision Plain With Sentry Diff
18890a0 500.08 ms 537.72 ms 37.64 ms
fa97c64 472.71 ms 530.63 ms 57.92 ms
2cc98f6 424.36 ms 472.90 ms 48.54 ms
0fef294 469.37 ms 509.82 ms 40.45 ms
bad0c67 463.64 ms 517.10 ms 53.46 ms
d90c4e9 468.52 ms 508.76 ms 40.24 ms
ad79f89 503.92 ms 507.65 ms 3.74 ms

App size

Revision Plain With Sentry Diff
18890a0 6.49 MiB 7.57 MiB 1.08 MiB
fa97c64 6.49 MiB 7.56 MiB 1.07 MiB
2cc98f6 6.49 MiB 7.57 MiB 1.08 MiB
0fef294 6.49 MiB 7.56 MiB 1.07 MiB
bad0c67 6.49 MiB 7.56 MiB 1.07 MiB
d90c4e9 6.49 MiB 7.56 MiB 1.07 MiB
ad79f89 6.49 MiB 7.56 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Oct 7, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1248.06 ms 1277.54 ms 29.48 ms
Size 8.38 MiB 9.77 MiB 1.39 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f0fcbe1 1238.08 ms 1243.44 ms 5.36 ms
0a23f98 1252.98 ms 1276.76 ms 23.78 ms
c978477 1228.43 ms 1249.17 ms 20.74 ms
a40bb7c 1222.75 ms 1240.88 ms 18.13 ms
6034b0a 1244.89 ms 1270.22 ms 25.33 ms
ef2f368 1259.12 ms 1277.04 ms 17.92 ms
ecb4003 1266.94 ms 1290.24 ms 23.31 ms
e0ba81f 1240.27 ms 1260.15 ms 19.88 ms
d5fb969 1228.79 ms 1256.17 ms 27.38 ms
7ec9238 1259.69 ms 1281.59 ms 21.90 ms

App size

Revision Plain With Sentry Diff
f0fcbe1 8.29 MiB 9.38 MiB 1.09 MiB
0a23f98 8.10 MiB 9.18 MiB 1.08 MiB
c978477 8.32 MiB 9.50 MiB 1.18 MiB
a40bb7c 8.38 MiB 9.73 MiB 1.35 MiB
6034b0a 8.33 MiB 9.40 MiB 1.07 MiB
ef2f368 8.15 MiB 9.10 MiB 965.24 KiB
ecb4003 8.09 MiB 9.07 MiB 1000.82 KiB
e0ba81f 8.38 MiB 9.71 MiB 1.34 MiB
d5fb969 8.33 MiB 9.64 MiB 1.31 MiB
7ec9238 8.34 MiB 9.65 MiB 1.31 MiB

Previous results on branch: feat/only-send-debug-images-referenced-in-the-stacktrace-for-events

Startup times

Revision Plain With Sentry Diff
ad79f89 1258.51 ms 1281.02 ms 22.51 ms
bad0c67 1252.14 ms 1262.27 ms 10.13 ms
18890a0 1252.71 ms 1274.23 ms 21.51 ms
fa97c64 1264.96 ms 1289.26 ms 24.30 ms
0fef294 1228.18 ms 1237.88 ms 9.69 ms
2cc98f6 1250.93 ms 1273.89 ms 22.96 ms
d90c4e9 1258.81 ms 1284.04 ms 25.23 ms

App size

Revision Plain With Sentry Diff
ad79f89 8.38 MiB 9.75 MiB 1.37 MiB
bad0c67 8.38 MiB 9.74 MiB 1.36 MiB
18890a0 8.38 MiB 9.75 MiB 1.37 MiB
fa97c64 8.38 MiB 9.74 MiB 1.36 MiB
0fef294 8.38 MiB 9.74 MiB 1.36 MiB
2cc98f6 8.38 MiB 9.75 MiB 1.37 MiB
d90c4e9 8.38 MiB 9.77 MiB 1.39 MiB

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.

I took a quick look and fixed it locally. here's several things that went wrong:

  • platform channels do not support Set, read here about the supported types, which means we need to use List to pass the instruction addresses from flutter to native. We can continue using sets when parsing the addresses but when communicating between platform layers these need to be converted to List. generally this may be something that we can improve for type safety when interacting through platform channels. I was surprised that this didn't trigger any error or warning
  • UInt64(instructionAddr) is not correct because the base it uses to convert the String is base 10 but the instruction addresses we are dealing with are hex so we need to adjust the radix/base to 16 => UInt64(instructionAddr, radix: 16) but this won't still work because the instruction addresses we pass in contain the 0x prefix which we also need to remove before we convert it to UInt64
  • In the native channel loadDebugImages function you changed Map<dynamic, dynamic> to Map<dynamic, Set<String>> which doesn't work because DebugMeta fields won't match this
  • if the debug images that we parsed from the stacktrace is empty, we should always return all debug images with PrivateSentrySDKOnly.getDebugImages(). This did not work because it was only triggered by the condition if the passed instruction addresses are null or empty and not if the parsed images afterwards are empty

flutter/ios/Classes/SentryFlutterPluginApple.swift Outdated Show resolved Hide resolved
flutter/ios/Classes/SentryFlutterPluginApple.swift Outdated Show resolved Hide resolved
flutter/ios/Classes/SentryFlutterPluginApple.swift Outdated Show resolved Hide resolved
flutter/lib/src/native/sentry_native_channel.dart Outdated Show resolved Hide resolved
flutter/lib/src/native/sentry_native_binding.dart Outdated Show resolved Hide resolved
martinhaintz and others added 7 commits October 11, 2024 11:05
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
Copy link
Contributor

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

  • flutter/ios/Classes/SentryFlutterPluginApple.swift

@martinhaintz martinhaintz enabled auto-merge (squash) October 11, 2024 16:58
@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@getsentry getsentry deleted a comment from github-actions bot Oct 11, 2024
@buenaflor
Copy link
Contributor

buenaflor commented Oct 14, 2024

as per this comment we will not use getDebugImagesForAddresses due to performance reasons.

Instead we should use SentryDebugImageProvider.getDebugImagesFromCacheForFrames or SentryDebugImageProvider. getDebugImagesFromCacheForThreads, however I think the former is the one more suited for us which will be part of Sentry Cocoa 8.39.0.

This PR is blocked until cocoa 8.39.0 is released

@buenaflor buenaflor marked this pull request as draft October 30, 2024 01:24
auto-merge was automatically disabled October 30, 2024 01:24

Pull request was converted to draft

@martinhaintz martinhaintz marked this pull request as ready for review November 13, 2024 13:06
@martinhaintz
Copy link
Collaborator Author

Changed the SentryDebugImageProvider.getDebugImages to SentryDebugImageProvider.getDebugImagesForImageAddressesFromCache and tested it via sentry.

@martinhaintz martinhaintz enabled auto-merge (squash) November 13, 2024 13:08
@getsentry getsentry deleted a comment from github-actions bot Nov 14, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 14, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 14, 2024
@buenaflor
Copy link
Contributor

lgtm now 👍 only one small change left

@buenaflor buenaflor removed the Blocked label Nov 14, 2024
@getsentry getsentry deleted a comment from github-actions bot Nov 15, 2024
martinhaintz and others added 2 commits November 18, 2024 09:41
Co-authored-by: Giancarlo Buenaflor <giancarlo_buenaflor@yahoo.com>
@martinhaintz martinhaintz merged commit 13c8257 into main Nov 18, 2024
48 checks passed
@martinhaintz martinhaintz deleted the feat/only-send-debug-images-referenced-in-the-stacktrace-for-events branch November 18, 2024 10:44
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.

Only send debug images referenced in the stacktrace for events
3 participants