refactor: move all native calls to SentryNativeBinding #2097

vaind · 2024-06-11T19:59:01Z

This finishes the transition to SentryNativeBinding. Previously, there was still some code directly using MethodChannels. Merging this PR will open way to integrating sentry-native via FFI (e.g. for desktop platforms), which is what I originally set out to do before finding out these blockers.

Basically, this PR:

moves the remaining native calls captureEnvelope(), loadContexts and loadDebugImages to SentryNativeBinding
removes SentryNative class that was just a wrapper around SentryNativeBinding, with some quirks:
- error handling moved to SentryNativeChannel via the SentryNativeSafeInvoker mixin
- native app start status variables moved to NativeAppStartIntegration
updates classes that depended on SentryNative to instead use any implementation of SentryNativeBinding
updates tests to accomodate these chanhes

I know this PR looks pretty large but although it doesn't seem like it, I did try to keep it to the minimum necessary. Hiding whitespace changes helps a little (especially in tests where a foreach loop may have been added/removed):

#skip-changelog because these are all internal changes

github-actions · 2024-06-11T20:10:17Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	393.11 ms	482.51 ms	89.40 ms
Size	6.35 MiB	7.34 MiB	1008.32 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`8f88a49`	364.98 ms	413.78 ms	48.80 ms
`daa1b33`	366.98 ms	451.59 ms	84.61 ms
`04db237`	330.16 ms	428.38 ms	98.22 ms
`afa6e2a`	349.73 ms	428.48 ms	78.75 ms
`e2d89fc`	323.84 ms	376.23 ms	52.39 ms
`86d4841`	286.35 ms	372.43 ms	86.08 ms
`8609bd8`	359.76 ms	437.40 ms	77.64 ms
`72dfc83`	298.62 ms	340.14 ms	41.52 ms
`870f5eb`	329.45 ms	369.29 ms	39.84 ms
`f79eecf`	341.35 ms	388.98 ms	47.63 ms

App size

Revision	Plain	With Sentry	Diff
`8f88a49`	6.34 MiB	7.30 MiB	979.60 KiB
`daa1b33`	6.27 MiB	7.20 MiB	956.36 KiB
`04db237`	5.94 MiB	6.95 MiB	1.01 MiB
`afa6e2a`	6.27 MiB	7.20 MiB	955.69 KiB
`e2d89fc`	6.06 MiB	7.03 MiB	989.37 KiB
`86d4841`	6.15 MiB	7.13 MiB	1000.49 KiB
`8609bd8`	6.27 MiB	7.20 MiB	959.07 KiB
`72dfc83`	5.94 MiB	6.92 MiB	1001.71 KiB
`870f5eb`	5.94 MiB	6.92 MiB	1005.77 KiB
`f79eecf`	6.15 MiB	7.13 MiB	1000.07 KiB

Previous results on branch: refactor/native-bindings

Startup times

Revision	Plain	With Sentry	Diff
`d780005`	361.80 ms	439.98 ms	78.18 ms
`69d7517`	449.02 ms	521.77 ms	72.75 ms
`298d05e`	424.94 ms	470.20 ms	45.26 ms
`7259c82`	412.69 ms	479.35 ms	66.67 ms
`829ca2d`	391.83 ms	468.26 ms	76.43 ms

App size

Revision	Plain	With Sentry	Diff
`d780005`	6.35 MiB	7.34 MiB	1007.94 KiB
`69d7517`	6.35 MiB	7.34 MiB	1007.93 KiB
`298d05e`	6.35 MiB	7.34 MiB	1007.73 KiB
`7259c82`	6.35 MiB	7.34 MiB	1007.95 KiB
`829ca2d`	6.35 MiB	7.34 MiB	1007.95 KiB

github-actions · 2024-06-11T20:15:51Z

iOS Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1232.24 ms	1255.52 ms	23.28 ms
Size	8.33 MiB	9.58 MiB	1.25 MiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`3f23617`	1261.93 ms	1286.10 ms	24.17 ms
`ca7f531`	1242.51 ms	1282.61 ms	40.10 ms
`d783424`	1265.71 ms	1277.00 ms	11.29 ms
`754cdbe`	1261.67 ms	1280.88 ms	19.20 ms
`a7acb24`	1296.71 ms	1317.69 ms	20.98 ms
`f2db4ec`	1244.14 ms	1259.79 ms	15.65 ms
`25e9b59`	1289.76 ms	1295.27 ms	5.51 ms
`683fd34`	1239.83 ms	1259.08 ms	19.25 ms
`b8562d0`	1249.92 ms	1267.56 ms	17.64 ms
`3e9fb0e`	1262.49 ms	1280.65 ms	18.16 ms

App size

Revision	Plain	With Sentry	Diff
`3f23617`	8.16 MiB	9.17 MiB	1.01 MiB
`ca7f531`	8.32 MiB	9.38 MiB	1.06 MiB
`d783424`	8.29 MiB	9.38 MiB	1.09 MiB
`754cdbe`	8.29 MiB	9.37 MiB	1.08 MiB
`a7acb24`	8.16 MiB	9.17 MiB	1.01 MiB
`f2db4ec`	8.10 MiB	9.16 MiB	1.07 MiB
`25e9b59`	8.16 MiB	9.15 MiB	1021.15 KiB
`683fd34`	8.28 MiB	9.34 MiB	1.06 MiB
`b8562d0`	8.33 MiB	9.54 MiB	1.22 MiB
`3e9fb0e`	8.15 MiB	9.12 MiB	989.77 KiB

Previous results on branch: refactor/native-bindings

Startup times

Revision	Plain	With Sentry	Diff
`7259c82`	1228.08 ms	1254.48 ms	26.40 ms
`829ca2d`	1217.73 ms	1229.41 ms	11.67 ms
`d780005`	1225.86 ms	1248.30 ms	22.44 ms
`69d7517`	1225.92 ms	1246.33 ms	20.41 ms

App size

Revision	Plain	With Sentry	Diff
`7259c82`	8.33 MiB	9.59 MiB	1.25 MiB
`829ca2d`	8.33 MiB	9.59 MiB	1.25 MiB
`d780005`	8.33 MiB	9.59 MiB	1.25 MiB
`69d7517`	8.33 MiB	9.59 MiB	1.25 MiB

vaind · 2024-06-13T10:40:47Z

flutter/test/load_image_list_test.dart

@@ -1,183 +0,0 @@
-@TestOn('vm')


this was duplicate of integration/load_image_list_integration.test

vaind · 2024-06-13T10:43:42Z

flutter/test/sentry_native_channel_test.dart

+  for (var platform in [
+    MockPlatform.android(),
+    MockPlatform.iOs(),
+    MockPlatform.macOs()
+  ]) {


this is basically the only addition here - running the test for all implementations of SentryNativeBinding.

The diff is a bit messed up here because of whitespace

codecov · 2024-06-13T10:49:54Z

Codecov Report

Attention: Patch coverage is 97.03704% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.43%. Comparing base (c3b8a98) to head (994cdfb).

Files	Patch %	Lines
...tter/lib/src/native/cocoa/sentry_native_cocoa.dart	60.00%	2 Missing ⚠️
...src/integrations/native_app_start_integration.dart	75.00%	1 Missing ⚠️
flutter/lib/src/native/factory_real.dart	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2097      +/-   ##
==========================================
+ Coverage   95.25%   95.43%   +0.17%     
==========================================
  Files          54       57       +3     
  Lines        1791     1751      -40     
==========================================
- Hits         1706     1671      -35     
+ Misses         85       80       -5

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

buenaflor

nice! 👍

buenaflor · 2024-06-19T10:48:05Z

flutter/lib/src/sentry_flutter.dart

+    final native = _native;
+    if (native != null) {
+      integrations.add(NativeSdkIntegration(native));
+      integrations.add(LoadContextsIntegration(native));


ah looks much nicer now 😄

if I get it right we only create the native binding for android, ios, macos right so there's no need for the checks we had before?

yes, same checks were applied above when actually asigning the variable so it's easier this way

buenaflor · 2024-06-19T11:04:44Z

btw sdk metrics is failing currently, the app size needs to be adjusted again, not related to this pr though

refactor: move all native calls to SentryNativeBinding [wip]

829ca2d

vaind added 2 commits June 13, 2024 09:15

refactor tests

7259c82

fix tests

e79278c

vaind commented Jun 13, 2024

View reviewed changes

cleanup

d780005

vaind added 3 commits June 13, 2024 13:11

fix tests

69d7517

fix tests

ef05592

code coverage

298d05e

vaind changed the title ~~refactor: move all native calls to SentryNativeBinding [wip]~~ refactor: move all native calls to SentryNativeBinding Jun 13, 2024

vaind marked this pull request as ready for review June 13, 2024 13:01

vaind requested review from krystofwoldrich, stefanosiano and buenaflor as code owners June 13, 2024 13:01

vaind and others added 2 commits June 18, 2024 14:19

Merge branch 'main' into refactor/native-bindings

c1f2357

fix tests post merge

994cdfb

buenaflor approved these changes Jun 19, 2024

View reviewed changes

vaind merged commit beeb73d into main Jun 19, 2024
46 of 47 checks passed

vaind deleted the refactor/native-bindings branch June 19, 2024 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move all native calls to SentryNativeBinding #2097

refactor: move all native calls to SentryNativeBinding #2097

vaind commented Jun 11, 2024 •

edited

github-actions bot commented Jun 11, 2024 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/native-bindings

Startup times

App size

github-actions bot commented Jun 11, 2024 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/native-bindings

Startup times

App size

vaind Jun 13, 2024

vaind Jun 13, 2024 •

edited

codecov bot commented Jun 13, 2024 •

edited

buenaflor left a comment

buenaflor Jun 19, 2024

vaind Jun 19, 2024

buenaflor commented Jun 19, 2024 •

edited

refactor: move all native calls to SentryNativeBinding #2097

refactor: move all native calls to SentryNativeBinding #2097

Conversation

vaind commented Jun 11, 2024 • edited

github-actions bot commented Jun 11, 2024 • edited

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/native-bindings

Startup times

App size

github-actions bot commented Jun 11, 2024 • edited

iOS Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: refactor/native-bindings

Startup times

App size

vaind Jun 13, 2024

Choose a reason for hiding this comment

vaind Jun 13, 2024 • edited

Choose a reason for hiding this comment

codecov bot commented Jun 13, 2024 • edited

Codecov Report

buenaflor left a comment

Choose a reason for hiding this comment

buenaflor Jun 19, 2024

Choose a reason for hiding this comment

vaind Jun 19, 2024

Choose a reason for hiding this comment

buenaflor commented Jun 19, 2024 • edited

vaind commented Jun 11, 2024 •

edited

github-actions bot commented Jun 11, 2024 •

edited

github-actions bot commented Jun 11, 2024 •

edited

vaind Jun 13, 2024 •

edited

codecov bot commented Jun 13, 2024 •

edited

buenaflor commented Jun 19, 2024 •

edited