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: Adds SAGP as an experimental expo plugin feature #4440

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Jan 13, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Adds SAGP as an experimental expo plugin when the enableAndroidGradlePlugin is enabled in the experimental_android section of the @sentry/react-native/expo plugin.
The supported options are the following:

  • autoUploadProguardMapping
  • includeProguardMapping
  • dexguardEnabled
  • uploadNativeSymbols
  • autoUploadNativeSymbols
  • includeNativeSources
  • includeSourceContext

💡 Motivation and Context

Fixes #4400

💚 How did you test it?

CI, Manual testing:

📝 Checklist

  • 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.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

See getsentry/sentry-android-gradle-plugin#809

Copy link
Contributor

github-actions bot commented Jan 13, 2025

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

Generated by 🚫 dangerJS against 2302390

Copy link
Contributor

github-actions bot commented Jan 13, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.27 ms 1230.63 ms -4.63 ms
Size 3.19 MiB 4.25 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
12427f4+dirty 1224.90 ms 1231.40 ms 6.50 ms
de59d3a+dirty 1241.17 ms 1249.16 ms 8.00 ms
63ed251+dirty 1223.27 ms 1222.94 ms -0.33 ms
5a22220+dirty 1246.18 ms 1249.61 ms 3.43 ms
416f465+dirty 1232.48 ms 1233.26 ms 0.78 ms
c398f67+dirty 1227.31 ms 1230.00 ms 2.69 ms
1332acb+dirty 1243.98 ms 1241.12 ms -2.86 ms
8fe7c9d+dirty 1227.63 ms 1245.28 ms 17.65 ms
5f03ae9+dirty 1237.79 ms 1241.02 ms 3.23 ms
9cab16b+dirty 1236.10 ms 1247.16 ms 11.06 ms

App size

Revision Plain With Sentry Diff
12427f4+dirty 2.92 MiB 3.44 MiB 533.29 KiB
de59d3a+dirty 2.92 MiB 3.67 MiB 772.59 KiB
63ed251+dirty 2.92 MiB 3.66 MiB 757.10 KiB
5a22220+dirty 2.92 MiB 3.48 MiB 575.81 KiB
416f465+dirty 2.92 MiB 3.67 MiB 772.44 KiB
c398f67+dirty 2.92 MiB 3.60 MiB 701.89 KiB
1332acb+dirty 2.92 MiB 3.67 MiB 772.45 KiB
8fe7c9d+dirty 3.19 MiB 4.24 MiB 1.06 MiB
5f03ae9+dirty 3.19 MiB 4.25 MiB 1.06 MiB
9cab16b+dirty 2.92 MiB 3.64 MiB 743.06 KiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
c5d03fc+dirty 1230.90 ms 1235.12 ms 4.22 ms
28fa62b+dirty 1214.88 ms 1214.82 ms -0.06 ms
e165be8+dirty 1228.63 ms 1245.41 ms 16.78 ms
a05231a+dirty 1225.98 ms 1234.84 ms 8.86 ms
fc3cd14+dirty 1252.76 ms 1248.33 ms -4.43 ms

App size

Revision Plain With Sentry Diff
c5d03fc+dirty 3.19 MiB 4.25 MiB 1.06 MiB
28fa62b+dirty 3.19 MiB 4.25 MiB 1.06 MiB
e165be8+dirty 3.19 MiB 4.25 MiB 1.06 MiB
a05231a+dirty 3.19 MiB 4.25 MiB 1.06 MiB
fc3cd14+dirty 3.19 MiB 4.25 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Jan 13, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.34 ms 1213.90 ms -6.44 ms
Size 2.63 MiB 3.69 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
12427f4+dirty 1267.15 ms 1271.30 ms 4.15 ms
de59d3a+dirty 1223.73 ms 1236.28 ms 12.55 ms
63ed251+dirty 1232.55 ms 1238.77 ms 6.22 ms
5a22220+dirty 1209.49 ms 1220.94 ms 11.45 ms
416f465+dirty 1221.56 ms 1226.48 ms 4.92 ms
c398f67+dirty 1219.67 ms 1225.66 ms 5.99 ms
1332acb+dirty 1230.53 ms 1234.54 ms 4.01 ms
8fe7c9d+dirty 1241.83 ms 1244.35 ms 2.51 ms
5f03ae9+dirty 1232.29 ms 1230.92 ms -1.37 ms
9cab16b+dirty 1237.76 ms 1234.00 ms -3.76 ms

App size

Revision Plain With Sentry Diff
12427f4+dirty 2.36 MiB 2.88 MiB 530.38 KiB
de59d3a+dirty 2.36 MiB 3.11 MiB 760.16 KiB
63ed251+dirty 2.36 MiB 3.10 MiB 752.55 KiB
5a22220+dirty 2.36 MiB 2.92 MiB 570.21 KiB
416f465+dirty 2.36 MiB 3.11 MiB 759.80 KiB
c398f67+dirty 2.36 MiB 3.04 MiB 696.27 KiB
1332acb+dirty 2.36 MiB 3.11 MiB 759.86 KiB
8fe7c9d+dirty 2.63 MiB 3.68 MiB 1.04 MiB
5f03ae9+dirty 2.63 MiB 3.68 MiB 1.05 MiB
9cab16b+dirty 2.36 MiB 3.08 MiB 737.23 KiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
c5d03fc+dirty 1214.23 ms 1214.45 ms 0.21 ms
28fa62b+dirty 1222.57 ms 1223.91 ms 1.34 ms
e165be8+dirty 1226.43 ms 1225.46 ms -0.97 ms
a05231a+dirty 1232.37 ms 1242.12 ms 9.75 ms
fc3cd14+dirty 1220.68 ms 1209.53 ms -11.15 ms

App size

Revision Plain With Sentry Diff
c5d03fc+dirty 2.63 MiB 3.69 MiB 1.05 MiB
28fa62b+dirty 2.63 MiB 3.69 MiB 1.05 MiB
e165be8+dirty 2.63 MiB 3.69 MiB 1.05 MiB
a05231a+dirty 2.63 MiB 3.68 MiB 1.05 MiB
fc3cd14+dirty 2.63 MiB 3.69 MiB 1.05 MiB

Copy link
Contributor

github-actions bot commented Jan 13, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 393.94 ms 461.88 ms 67.94 ms
Size 7.15 MiB 8.38 MiB 1.23 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
416f465+dirty 407.26 ms 451.31 ms 44.05 ms
9f0f6c8+dirty 365.53 ms 419.16 ms 53.63 ms
70e6261+dirty 395.08 ms 408.12 ms 13.04 ms
d43a46b+dirty 417.65 ms 472.98 ms 55.33 ms
2ec71da+dirty 375.64 ms 431.59 ms 55.95 ms
12427f4+dirty 379.48 ms 400.92 ms 21.44 ms
5446992+dirty 371.61 ms 390.00 ms 18.39 ms
baa882f+dirty 449.30 ms 540.40 ms 91.10 ms
d0bf494+dirty 253.73 ms 308.23 ms 54.49 ms
9a3ca65+dirty 344.96 ms 358.92 ms 13.96 ms

App size

Revision Plain With Sentry Diff
416f465+dirty 7.15 MiB 8.37 MiB 1.22 MiB
9f0f6c8+dirty 7.15 MiB 8.37 MiB 1.22 MiB
70e6261+dirty 7.15 MiB 8.21 MiB 1.07 MiB
d43a46b+dirty 7.15 MiB 8.34 MiB 1.19 MiB
2ec71da+dirty 7.15 MiB 8.38 MiB 1.23 MiB
12427f4+dirty 7.15 MiB 8.12 MiB 997.78 KiB
5446992+dirty 7.15 MiB 8.12 MiB 999.45 KiB
baa882f+dirty 7.15 MiB 8.34 MiB 1.19 MiB
d0bf494+dirty 7.15 MiB 8.04 MiB 910.85 KiB
9a3ca65+dirty 7.15 MiB 8.09 MiB 962.83 KiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
a05231a+dirty 410.49 ms 464.94 ms 54.45 ms
e165be8+dirty 403.17 ms 433.17 ms 30.00 ms
28fa62b+dirty 467.78 ms 451.33 ms -16.45 ms
c5d03fc+dirty 400.53 ms 450.84 ms 50.31 ms
fc3cd14+dirty 409.87 ms 463.46 ms 53.59 ms

App size

Revision Plain With Sentry Diff
a05231a+dirty 7.15 MiB 8.38 MiB 1.23 MiB
e165be8+dirty 7.15 MiB 8.38 MiB 1.23 MiB
28fa62b+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c5d03fc+dirty 7.15 MiB 8.38 MiB 1.23 MiB
fc3cd14+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 405.46 ms 423.43 ms 17.97 ms
Size 17.75 MiB 20.11 MiB 2.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
cdf2f33 469.46 ms 462.17 ms -7.29 ms
7bc4d75 488.76 ms 473.28 ms -15.48 ms
e22745e 462.66 ms 458.10 ms -4.56 ms
f06c879 408.41 ms 424.54 ms 16.13 ms
e2b64fe 316.88 ms 330.23 ms 13.35 ms
c6f01ea 486.20 ms 486.98 ms 0.77 ms
34aba08 328.10 ms 342.84 ms 14.74 ms
b95b8af 454.05 ms 454.53 ms 0.48 ms
d8e8c67 448.79 ms 438.70 ms -10.09 ms
52c0562 453.04 ms 434.71 ms -18.33 ms

App size

Revision Plain With Sentry Diff
cdf2f33 17.74 MiB 20.08 MiB 2.34 MiB
7bc4d75 17.74 MiB 20.08 MiB 2.34 MiB
e22745e 17.74 MiB 20.08 MiB 2.34 MiB
f06c879 17.73 MiB 19.85 MiB 2.12 MiB
e2b64fe 17.73 MiB 19.80 MiB 2.07 MiB
c6f01ea 17.74 MiB 20.10 MiB 2.36 MiB
34aba08 17.73 MiB 19.80 MiB 2.07 MiB
b95b8af 17.73 MiB 20.11 MiB 2.37 MiB
d8e8c67 17.74 MiB 20.10 MiB 2.36 MiB
52c0562 17.73 MiB 20.11 MiB 2.38 MiB

Previous results on branch: antonis/experimental-expo-sagp

Startup times

Revision Plain With Sentry Diff
fc3cd14 453.14 ms 472.16 ms 19.02 ms
c5d03fc 422.04 ms 418.83 ms -3.21 ms
a05231a 459.71 ms 465.70 ms 5.99 ms
28fa62b 420.92 ms 423.72 ms 2.80 ms
e165be8 451.06 ms 466.90 ms 15.84 ms

App size

Revision Plain With Sentry Diff
fc3cd14 17.75 MiB 20.11 MiB 2.37 MiB
c5d03fc 17.75 MiB 20.11 MiB 2.37 MiB
a05231a 17.75 MiB 20.11 MiB 2.36 MiB
28fa62b 17.75 MiB 20.11 MiB 2.37 MiB
e165be8 17.75 MiB 20.11 MiB 2.37 MiB

@antonis antonis marked this pull request as ready for review January 14, 2025 07:36
import { withAppBuildGradle, withProjectBuildGradle } from '@expo/config-plugins';

export interface SentryAndroidGradlePluginOptions {
sentryAndroidGradlePluginVersion?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Let's bake in a version that's compatible with current RN SDK and keep it our responsibility instead of users having to pick SAGP version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated with 2539a24 to hardcode the SAGP version and use enableAndroidGradlePlugin as a flag to enable.

Comment on lines 19 to 28
export function withSentryAndroidGradlePlugin(config: any, options: SentryAndroidGradlePluginOptions = {}): any {
const version = options.sentryAndroidGradlePluginVersion;
const includeProguardMapping = options.includeProguardMapping ?? true;
const dexguardEnabled = options.dexguardEnabled ?? false;
const autoUploadProguardMapping = options.autoUploadProguardMapping ?? true;
const uploadNativeSymbols = options.uploadNativeSymbols ?? true;
const autoUploadNativeSymbols = options.autoUploadNativeSymbols ?? true;
const includeNativeSources = options.includeNativeSources ?? true;
const includeSourceContext = options.includeSourceContext ?? false;
const autoInstallationEnabled = options.autoInstallationEnabled ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

We can set the defaults when deconstructing the options argument, it makes it easier to read and removes the names repetition.

Suggested change
export function withSentryAndroidGradlePlugin(config: any, options: SentryAndroidGradlePluginOptions = {}): any {
const version = options.sentryAndroidGradlePluginVersion;
const includeProguardMapping = options.includeProguardMapping ?? true;
const dexguardEnabled = options.dexguardEnabled ?? false;
const autoUploadProguardMapping = options.autoUploadProguardMapping ?? true;
const uploadNativeSymbols = options.uploadNativeSymbols ?? true;
const autoUploadNativeSymbols = options.autoUploadNativeSymbols ?? true;
const includeNativeSources = options.includeNativeSources ?? true;
const includeSourceContext = options.includeSourceContext ?? false;
const autoInstallationEnabled = options.autoInstallationEnabled ?? false;
export function withSentryAndroidGradlePlugin(
config: any,
{
sentryAndroidGradlePluginVersion,
includeProguardMapping = true,
dexguardEnabled = false,
autoUploadProguardMapping = true,
uploadNativeSymbols = true,
autoUploadNativeSymbols = true,
includeNativeSources = true,
includeSourceContext = false,
autoInstallationEnabled = false
}: SentryAndroidGradlePluginOptions = {}
): any {
const version = sentryAndroidGradlePluginVersion;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea 🙇
Updated with 59e54ce

const withSentryAppBuildGradle = (config: any): any => {
return withAppBuildGradle(config, (config: any) => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
if (config.modResults.language === 'groovy') {
Copy link
Member

@krystofwoldrich krystofwoldrich Jan 16, 2025

Choose a reason for hiding this comment

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

Let's invert this and use it as a guard !== groovy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense 👍 Updated with 7b053f5

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
config.modResults.contents = contents;
} else {
throw new Error('Cannot configure Sentry in android/app/build.gradle because it is not in Groovy.');
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the warnOnce helper instead of throw. It will make the message more readable for the users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea 👍
Updated with b1230bd


// Modify android/build.gradle
const withSentryProjectBuildGradle = (config: any): any => {
return withProjectBuildGradle(config, (projectBuildGradle: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check if the lang is groovy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point 👍 Added a check with 9d6d9d2

Comment on lines 43 to 46
projectBuildGradle.modResults.contents = projectBuildGradle.modResults.contents.replace(
/dependencies\s*{/,
`dependencies {\n ${dependency}`,
);
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add a warning if we could not add the dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added with cf55ce4 and f04a2c7

@krystofwoldrich
Copy link
Member

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

@antonis
Copy link
Collaborator Author

antonis commented Jan 16, 2025

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

Makes sense 👍 I followed up with a docs PR for this.

@lucas-zimerman
Copy link
Collaborator

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

It would be great if we could merge this Page and make it only available to people accessing it only by direct link, so it is easier to validate the URLs and also keep it hidden from the users while an official release isn't released

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

I see no issues with the PR, LGTM!

@antonis
Copy link
Collaborator Author

antonis commented Jan 30, 2025

When this is merged and release we should also update the docs.

Maybe a sub page to https://docs.sentry.io/platforms/react-native/manual-setup/expo/

(not to interrupt the default setup flow with the gradle options).

It would be great if we could merge this Page and make it only available to people accessing it only by direct link, so it is easier to validate the URLs and also keep it hidden from the users while an official release isn't released

The current doc PR adds it as a subpage with no link from the parent (other than the navigation links). I'm ok with hiding it. Wdyt @krystofwoldrich?

ps. If I'm not mistaken hiding the page can be achieved by setting draft and noindex to true

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.

Proguard Mappings are not uploaded using expo plugin
3 participants