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

Disable ScreenshotIntegration, WidgetsBindingIntegration and SentryWidget in multi-view apps #2366

Merged
merged 32 commits into from
Feb 12, 2025

Conversation

martinhaintz
Copy link
Contributor

@martinhaintz martinhaintz commented Oct 21, 2024

📜 Description

Disable ScreenshotIntegration, WidgetsBindingIntegration and SentryWidget in multi-view applications.

To find out if we are in a multi-view app, we check the PlatformDispatcher.instance.implicitView return value.

In a non-multiview app PlatformDispatcher.instance.implicitView returns a FlutterView object, in multi-view it returns null. See Flutter Docs for more infos.

💡 Motivation and Context

Based on the discussion here.

💚 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

Copy link
Contributor

github-actions bot commented Oct 21, 2024

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

Generated by 🚫 dangerJS against bb385c5

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.09%. Comparing base (d7dc4e5) to head (bb385c5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ter/lib/src/utils/platform_dispatcher_wrapper.dart 60.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2366      +/-   ##
==========================================
+ Coverage   88.98%   92.09%   +3.10%     
==========================================
  Files         262       91     -171     
  Lines        8906     2946    -5960     
==========================================
- Hits         7925     2713    -5212     
+ Misses        981      233     -748     

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

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Copy link
Contributor

github-actions bot commented Oct 21, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 435.59 ms 501.94 ms 66.35 ms
Size 6.46 MiB 7.48 MiB 1.02 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
905bf99 459.73 ms 505.26 ms 45.53 ms
464b4d0 320.71 ms 380.02 ms 59.31 ms
ea60f10 456.23 ms 494.48 ms 38.25 ms
1b0c8a3 479.78 ms 521.18 ms 41.40 ms
e893df5 310.60 ms 380.58 ms 69.98 ms
0a23f98 377.19 ms 416.18 ms 39.00 ms
691aa3b 370.43 ms 466.28 ms 95.85 ms
3e5ee37 317.56 ms 366.84 ms 49.28 ms
ca7f531 395.69 ms 497.82 ms 102.12 ms
4c13d97 455.34 ms 509.42 ms 54.08 ms

App size

Revision Plain With Sentry Diff
905bf99 6.49 MiB 7.56 MiB 1.07 MiB
464b4d0 6.06 MiB 7.03 MiB 990.27 KiB
ea60f10 6.49 MiB 7.55 MiB 1.07 MiB
1b0c8a3 6.49 MiB 7.57 MiB 1.08 MiB
e893df5 6.06 MiB 7.09 MiB 1.03 MiB
0a23f98 6.06 MiB 7.03 MiB 996.97 KiB
691aa3b 5.94 MiB 6.96 MiB 1.02 MiB
3e5ee37 5.94 MiB 6.92 MiB 1001.19 KiB
ca7f531 6.33 MiB 7.26 MiB 949.75 KiB
4c13d97 6.49 MiB 7.56 MiB 1.07 MiB

Previous results on branch: feat/multiview-aware

Startup times

Revision Plain With Sentry Diff
2baf2a4 414.65 ms 505.75 ms 91.10 ms
be2b2de 411.65 ms 507.57 ms 95.93 ms
df169ab 452.48 ms 559.46 ms 106.98 ms
1d57db2 542.49 ms 649.24 ms 106.75 ms
72556e0 401.20 ms 502.86 ms 101.66 ms

App size

Revision Plain With Sentry Diff
2baf2a4 6.46 MiB 7.48 MiB 1.02 MiB
be2b2de 6.46 MiB 7.48 MiB 1.03 MiB
df169ab 6.46 MiB 7.48 MiB 1.03 MiB
1d57db2 6.46 MiB 7.48 MiB 1.03 MiB
72556e0 6.46 MiB 7.48 MiB 1.02 MiB

Copy link
Contributor

github-actions bot commented Oct 21, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1257.08 ms 1268.57 ms 11.49 ms
Size 8.42 MiB 9.91 MiB 1.49 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
95c69e3 1223.94 ms 1245.29 ms 21.35 ms
09c1f55 1258.11 ms 1280.45 ms 22.34 ms
49a149b 1296.47 ms 1320.20 ms 23.73 ms
bffc2c5 1239.76 ms 1264.67 ms 24.92 ms
b49bf00 1248.00 ms 1260.35 ms 12.35 ms
a609134 1254.50 ms 1265.08 ms 10.58 ms
d1488a1 1234.02 ms 1244.98 ms 10.96 ms
a24a1db 1257.71 ms 1271.57 ms 13.87 ms
905bf99 1240.84 ms 1271.47 ms 30.63 ms
9da696d 1253.91 ms 1267.68 ms 13.77 ms

App size

Revision Plain With Sentry Diff
95c69e3 8.33 MiB 9.64 MiB 1.31 MiB
09c1f55 8.38 MiB 9.74 MiB 1.36 MiB
49a149b 8.15 MiB 9.12 MiB 986.26 KiB
bffc2c5 8.33 MiB 9.40 MiB 1.07 MiB
b49bf00 8.10 MiB 9.08 MiB 1004.36 KiB
a609134 8.16 MiB 9.16 MiB 1.01 MiB
d1488a1 8.42 MiB 9.89 MiB 1.47 MiB
a24a1db 8.42 MiB 9.91 MiB 1.49 MiB
905bf99 8.38 MiB 9.74 MiB 1.36 MiB
9da696d 8.42 MiB 9.91 MiB 1.49 MiB

Previous results on branch: feat/multiview-aware

Startup times

Revision Plain With Sentry Diff
2baf2a4 1255.74 ms 1276.96 ms 21.22 ms
72556e0 1254.58 ms 1269.53 ms 14.95 ms
be2b2de 1263.27 ms 1272.61 ms 9.35 ms
df169ab 1239.88 ms 1259.89 ms 20.02 ms
1d57db2 1245.47 ms 1253.96 ms 8.49 ms

App size

Revision Plain With Sentry Diff
2baf2a4 8.42 MiB 9.91 MiB 1.49 MiB
72556e0 8.42 MiB 9.91 MiB 1.49 MiB
be2b2de 8.42 MiB 9.91 MiB 1.49 MiB
df169ab 8.42 MiB 9.91 MiB 1.49 MiB
1d57db2 8.42 MiB 9.91 MiB 1.49 MiB

@martinhaintz martinhaintz changed the title add multiview helper to make the sentry widget multiview aware. add multiview helper to make the sentry multiview aware. Oct 22, 2024
@martinhaintz martinhaintz marked this pull request as ready for review October 22, 2024 11:47
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.

not sure how it's gonna work but any chance we can set up some simple tests for this?

I wanna make sure that this doesn't regress in future flutter versions

@buenaflor
Copy link
Contributor

I'll add an integration test for this

@buenaflor buenaflor self-assigned this Dec 16, 2024
@buenaflor buenaflor assigned denrase and unassigned buenaflor Jan 22, 2025
@denrase denrase changed the title add multiview helper to make the sentry multiview aware. Disable ScreenshotIntegration, WidgetsBindingIntegration and SentryWidget in multi-view applications. Feb 4, 2025
@denrase denrase changed the title Disable ScreenshotIntegration, WidgetsBindingIntegration and SentryWidget in multi-view applications. Disable ScreenshotIntegration, WidgetsBindingIntegration and SentryWidget in multi-view apps Feb 4, 2025
@denrase denrase marked this pull request as ready for review February 4, 2025 10:50
@denrase denrase requested a review from buenaflor February 4, 2025 10:51
@denrase denrase enabled auto-merge (squash) February 4, 2025 13:00
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! wdyt is the effort of putting in an integration test in example/integration_test? @denrase

@denrase
Copy link
Collaborator

denrase commented Feb 5, 2025

@buenaflor what do you recommend should this test cover?

@buenaflor
Copy link
Contributor

buenaflor commented Feb 5, 2025

@denrase I was thinking about creating a new app that has multiview enabled with an integration test that checks if the relevant sentry integrations are disabled but probably thats too much work, wdyt

otherwise what we could do is add an addition web integration_test case where we check that these integrations ScreenshotIntegration, WidgetsBindingIntegration are not disabled so we prevent potential regressions

I have already set up integration tests that run on web, see web_sdk_test.dart as an example

@denrase denrase merged commit 57323e5 into main Feb 12, 2025
61 checks passed
@denrase denrase deleted the feat/multiview-aware branch February 12, 2025 10:50
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.

None yet

3 participants