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

Fix: Only call method channels on native platforms #1196

Merged
merged 9 commits into from
Jan 9, 2023

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Dec 19, 2022

📜 Description

Closes #1176

💡 Motivation and Context

  • Only call native cannels on native platforms
  • Also call native scope observers on iOS & macOS

💚 How did you test it?

Unit tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2022

Codecov Report

Base: 88.87% // Head: 76.45% // Decreases project coverage by -12.41% ⚠️

Coverage data is based on head (1541762) compared to base (9928a74).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1196       +/-   ##
===========================================
- Coverage   88.87%   76.45%   -12.42%     
===========================================
  Files         121       11      -110     
  Lines        3810      327     -3483     
===========================================
- Hits         3386      250     -3136     
+ Misses        424       77      -347     
Impacted Files Coverage Δ
dart/lib/src/noop_client.dart
...lib/src/environment/_io_environment_variables.dart
dart/lib/src/protocol/sentry_browser.dart
dart/lib/src/platform/_io_platform.dart
dart/lib/src/protocol/span_status.dart
dart/lib/src/utils/iterable_extension.dart
dart/lib/src/sentry_envelope.dart
dart/lib/src/transport/rate_limit.dart
dart/lib/src/sentry_client.dart
dart/lib/src/http_client/breadcrumb_client.dart
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2022

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 320.88 ms 384.14 ms 63.26 ms
Size 5.94 MiB 6.96 MiB 1.02 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ccc09e4 308.21 ms 357.74 ms 49.54 ms
aed5947 295.70 ms 348.18 ms 52.48 ms
fbf42af 323.04 ms 365.09 ms 42.04 ms
cdf7172 348.54 ms 390.81 ms 42.27 ms
2f8f173 323.31 ms 373.29 ms 49.97 ms
3e5ee37 317.56 ms 366.84 ms 49.28 ms
559d28f 302.35 ms 339.53 ms 37.18 ms
72dfc83 298.62 ms 340.14 ms 41.52 ms
ae02632 309.16 ms 348.98 ms 39.82 ms
eb1a7c1 332.98 ms 381.55 ms 48.57 ms

App size

Revision Plain With Sentry Diff
ccc09e4 5.94 MiB 6.95 MiB 1.01 MiB
aed5947 5.94 MiB 6.96 MiB 1.02 MiB
fbf42af 5.94 MiB 6.96 MiB 1.02 MiB
cdf7172 5.94 MiB 6.95 MiB 1.01 MiB
2f8f173 5.94 MiB 6.95 MiB 1.01 MiB
3e5ee37 5.94 MiB 6.92 MiB 1001.19 KiB
559d28f 5.94 MiB 6.92 MiB 1001.70 KiB
72dfc83 5.94 MiB 6.92 MiB 1001.71 KiB
ae02632 5.94 MiB 6.95 MiB 1.01 MiB
eb1a7c1 5.94 MiB 6.92 MiB 1005.76 KiB

Previous results on branch: fix/method-channels-ios

Startup times

Revision Plain With Sentry Diff
50bb8f1 360.83 ms 432.41 ms 71.58 ms
bcb98b2 344.80 ms 383.92 ms 39.12 ms
1856064 314.94 ms 369.42 ms 54.48 ms
f181dc0 340.57 ms 394.32 ms 53.75 ms

App size

Revision Plain With Sentry Diff
50bb8f1 5.94 MiB 6.96 MiB 1.02 MiB
bcb98b2 5.94 MiB 6.96 MiB 1.02 MiB
1856064 5.94 MiB 6.96 MiB 1.02 MiB
f181dc0 5.94 MiB 6.96 MiB 1.02 MiB

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2022

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1257.53 ms 1280.59 ms 23.06 ms
Size 8.16 MiB 9.17 MiB 1.01 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3f23617 1261.93 ms 1286.10 ms 24.17 ms
1131914 1277.20 ms 1300.20 ms 23.00 ms
6325c3b 1266.52 ms 1291.06 ms 24.54 ms
613760b 1263.10 ms 1277.27 ms 14.16 ms
ef2f368 1259.12 ms 1277.04 ms 17.92 ms
eecbbca 1264.90 ms 1286.33 ms 21.43 ms
691aa3b 1265.57 ms 1282.13 ms 16.55 ms
0ceb89c 1252.02 ms 1271.78 ms 19.75 ms
62dde43 1258.43 ms 1276.81 ms 18.38 ms
a609134 1254.50 ms 1265.08 ms 10.58 ms

App size

Revision Plain With Sentry Diff
3f23617 8.16 MiB 9.17 MiB 1.01 MiB
1131914 8.16 MiB 9.17 MiB 1.01 MiB
6325c3b 8.16 MiB 9.17 MiB 1.01 MiB
613760b 8.15 MiB 9.13 MiB 1000.46 KiB
ef2f368 8.15 MiB 9.10 MiB 965.24 KiB
eecbbca 8.15 MiB 9.10 MiB 965.26 KiB
691aa3b 8.16 MiB 9.17 MiB 1.01 MiB
0ceb89c 8.15 MiB 9.12 MiB 989.78 KiB
62dde43 8.16 MiB 9.17 MiB 1.01 MiB
a609134 8.16 MiB 9.16 MiB 1.01 MiB

Previous results on branch: fix/method-channels-ios

Startup times

Revision Plain With Sentry Diff
f181dc0 1271.37 ms 1292.42 ms 21.05 ms
bcb98b2 1266.35 ms 1277.86 ms 11.50 ms
1856064 1273.82 ms 1292.48 ms 18.66 ms
50bb8f1 1264.76 ms 1276.58 ms 11.83 ms

App size

Revision Plain With Sentry Diff
f181dc0 8.16 MiB 9.17 MiB 1.01 MiB
bcb98b2 8.16 MiB 9.17 MiB 1.01 MiB
1856064 8.16 MiB 9.17 MiB 1.01 MiB
50bb8f1 8.16 MiB 9.17 MiB 1.01 MiB

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

Left a comment otherwise LGTM.

@marandaneto
Copy link
Contributor

@denrase have you checked that the scope sync is working for iOS/macOS now? we need to be sure before going ahead.

@marandaneto
Copy link
Contributor

@denrase have you checked that the scope sync is working for iOS/macOS now? we need to be sure before going ahead.

@denrase is this tested? ready to be merged?

@denrase
Copy link
Collaborator Author

denrase commented Jan 9, 2023

@marandaneto Set a sample user through scope sync in the example app and let it crash in swift code, user was synced. On the main branch, there is no user with the same code.

Here

Bildschirmfoto 2023-01-09 um 12 55 40

Main

Bildschirmfoto 2023-01-09 um 13 04 21

@denrase
Copy link
Collaborator Author

denrase commented Jan 9, 2023

@marandaneto Also works on macOS.

Bildschirmfoto 2023-01-09 um 13 14 41

@denrase
Copy link
Collaborator Author

denrase commented Jan 9, 2023

Tested with all methods. Issue with removeContexts and removeTag, which may be an issue in the cocoa SDK.

@denrase denrase merged commit 3141b75 into main Jan 9, 2023
@denrase denrase deleted the fix/method-channels-ios branch January 9, 2023 13:17
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.

MethodChannel is called for platforms that dont have some implementations such as endNativeFrames
3 participants