Skip to content

fix(replay): dont set isTerminating on captureReplay for android#3037

Merged
buenaflor merged 16 commits into
mainfrom
fix/dont-set-is-crash-captureReplay-android
Jul 4, 2025
Merged

fix(replay): dont set isTerminating on captureReplay for android#3037
buenaflor merged 16 commits into
mainfrom
fix/dont-set-is-crash-captureReplay-android

Conversation

@buenaflor

Copy link
Copy Markdown
Contributor

📜 Description

💡 Motivation and Context

Fixes a bug where we set the isTerminating parameter on captureReplay to true if an unhandled error happens.

This is a bug because Android replay will stop capturing segments as it expects the app to terminate - so for flutter we potentially run into missing segments

💚 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

@buenaflor
buenaflor changed the base branch from main to fix/jni-proguard-rules July 3, 2025 09:44
@github-actions

github-actions Bot commented Jul 3, 2025

Copy link
Copy Markdown
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/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt

@codecov

codecov Bot commented Jul 3, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.79%. Comparing base (93b7728) to head (fdc97ef).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3037      +/-   ##
==========================================
+ Coverage   87.97%   89.79%   +1.81%     
==========================================
  Files         287       96     -191     
  Lines        9533     3331    -6202     
==========================================
- Hits         8387     2991    -5396     
+ Misses       1146      340     -806     

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

Comment thread flutter/test/replay/replay_event_processor_test.dart Outdated
Comment thread flutter/test/replay/replay_event_processor_test.dart Outdated
Comment thread flutter/test/sentry_native_channel_test.dart
@buenaflor
buenaflor requested a review from denrase July 3, 2025 17:48
@buenaflor

Copy link
Copy Markdown
Contributor Author

@denrase updated it 👍

Base automatically changed from fix/jni-proguard-rules to main July 3, 2025 17:50
@github-actions

github-actions Bot commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.73 ms 1247.21 ms 10.48 ms
Size 7.86 MiB 9.44 MiB 1.58 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c8596a6 1234.11 ms 1241.19 ms 7.08 ms
827bf09 1261.86 ms 1276.41 ms 14.55 ms
2d34233 1258.19 ms 1268.92 ms 10.73 ms
6ba4675 1223.12 ms 1238.17 ms 15.04 ms
93b7728 1247.23 ms 1264.87 ms 17.64 ms

App size

Revision Plain With Sentry Diff
c8596a6 7.86 MiB 9.44 MiB 1.58 MiB
827bf09 7.86 MiB 9.44 MiB 1.58 MiB
2d34233 7.86 MiB 9.44 MiB 1.58 MiB
6ba4675 7.86 MiB 9.44 MiB 1.58 MiB
93b7728 7.86 MiB 9.44 MiB 1.58 MiB

Previous results on branch: fix/dont-set-is-crash-captureReplay-android

Startup times

Revision Plain With Sentry Diff
2d02261 1246.74 ms 1268.15 ms 21.40 ms
3de5a40 1258.85 ms 1270.92 ms 12.07 ms

App size

Revision Plain With Sentry Diff
2d02261 7.86 MiB 9.44 MiB 1.58 MiB
3de5a40 7.86 MiB 9.44 MiB 1.58 MiB

@github-actions

github-actions Bot commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 468.04 ms 537.00 ms 68.96 ms
Size 6.54 MiB 7.69 MiB 1.15 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c8596a6 474.00 ms 492.96 ms 18.96 ms
93b7728 475.28 ms 489.13 ms 13.86 ms
6ba4675 499.80 ms 632.43 ms 132.63 ms
827bf09 475.40 ms 547.14 ms 71.74 ms
2d34233 470.54 ms 558.90 ms 88.36 ms

App size

Revision Plain With Sentry Diff
c8596a6 6.54 MiB 7.53 MiB 1015.27 KiB
93b7728 6.54 MiB 7.69 MiB 1.15 MiB
6ba4675 6.54 MiB 7.53 MiB 1015.26 KiB
827bf09 6.54 MiB 7.53 MiB 1015.27 KiB
2d34233 6.54 MiB 7.55 MiB 1.01 MiB

Previous results on branch: fix/dont-set-is-crash-captureReplay-android

Startup times

Revision Plain With Sentry Diff
3de5a40 472.27 ms 532.69 ms 60.42 ms
2d02261 425.94 ms 494.69 ms 68.76 ms

App size

Revision Plain With Sentry Diff
3de5a40 6.54 MiB 7.55 MiB 1.01 MiB
2d02261 6.54 MiB 7.55 MiB 1.01 MiB

@denrase denrase left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a small suggestion.

return
}
replay!!.captureReplay(isCrash)
replay!!.captureReplay(isTerminating = false)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a small thin, but maybe we could safley unpack the otpional replay. If for some reason this becomes nil, we will crash here.

@buenaflor buenaflor Jul 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if an error happens in the plugin it doesnt crash, Flutter will still catch it and propagate it to the flutter layer (where we catch it with the SentryNativeSafeInvoker

image

@buenaflor
buenaflor merged commit 49e44d2 into main Jul 4, 2025
88 of 92 checks passed
@buenaflor
buenaflor deleted the fix/dont-set-is-crash-captureReplay-android branch July 4, 2025 08:57
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.

2 participants