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

Add Debug Ids support for JSC and Hermes bundles #3164

Merged
merged 40 commits into from
Oct 5, 2023

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Jul 5, 2023

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Adds Sentry Metro Plugin that injects Debug IDs to bundles.

Adds additional tooling that enables Debug IDs in Hermes source maps.

This PR adds Sentry Metro Serializer, which wraps the default or user-supplied serializer, and adds a Sentry Debug ID module which contains the Debug ID code snipped. The serializer also creates a Debug ID by hashing the content of the intermediate bundle, that is the bundle object before written to disk.

💡 Motivation and Context

💚 How did you test it?

sample app, ci

📝 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
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1284.86 ms 1284.90 ms 0.04 ms
Size 2.36 MiB 2.87 MiB 520.64 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 1293.52 ms 1296.08 ms 2.56 ms
0677344+dirty 1276.70 ms 1300.07 ms 23.37 ms
3853f43+dirty 1221.82 ms 1242.64 ms 20.82 ms
ad6c299+dirty 1244.76 ms 1260.10 ms 15.34 ms
34aba08+dirty 1276.78 ms 1308.52 ms 31.74 ms
d197b5c+dirty 1217.61 ms 1242.66 ms 25.05 ms
9433f35+dirty 1246.94 ms 1271.45 ms 24.52 ms
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
80b2ce3+dirty 1265.92 ms 1268.60 ms 2.69 ms
457e29f+dirty 1253.94 ms 1269.18 ms 15.24 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 2.36 MiB 2.85 MiB 500.03 KiB
0677344+dirty 2.36 MiB 2.85 MiB 496.81 KiB
3853f43+dirty 2.36 MiB 2.85 MiB 499.81 KiB
ad6c299+dirty 2.36 MiB 2.84 MiB 488.85 KiB
34aba08+dirty 2.36 MiB 2.85 MiB 495.32 KiB
d197b5c+dirty 2.36 MiB 2.82 MiB 462.86 KiB
9433f35+dirty 2.36 MiB 2.85 MiB 499.80 KiB
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
80b2ce3+dirty 2.36 MiB 2.84 MiB 486.98 KiB
457e29f+dirty 2.36 MiB 2.87 MiB 520.67 KiB

Previous results on branch: kw-debug-id-metro-plugin

Startup times

Revision Plain With Sentry Diff
6fa4f32+dirty 1225.69 ms 1233.76 ms 8.07 ms
116ff59+dirty 1245.98 ms 1261.74 ms 15.76 ms
7c3bc66+dirty 1252.41 ms 1271.14 ms 18.73 ms

App size

Revision Plain With Sentry Diff
6fa4f32+dirty 2.36 MiB 2.87 MiB 520.52 KiB
116ff59+dirty 2.36 MiB 2.87 MiB 520.56 KiB
7c3bc66+dirty 2.36 MiB 2.87 MiB 520.54 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 364.85 ms 384.02 ms 19.17 ms
Size 7.15 MiB 8.10 MiB 981.29 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
3853f43+dirty 278.12 ms 338.72 ms 60.60 ms
ad6c299+dirty 336.47 ms 362.89 ms 26.42 ms
34aba08+dirty 331.79 ms 376.69 ms 44.91 ms
d197b5c+dirty 258.75 ms 313.61 ms 54.86 ms
9433f35+dirty 265.50 ms 336.08 ms 70.58 ms
76d1baf+dirty 339.02 ms 408.65 ms 69.63 ms
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms
457e29f+dirty 591.49 ms 612.96 ms 21.47 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
3853f43+dirty 7.15 MiB 8.08 MiB 959.34 KiB
ad6c299+dirty 7.15 MiB 8.04 MiB 912.17 KiB
34aba08+dirty 7.15 MiB 8.07 MiB 946.13 KiB
d197b5c+dirty 7.15 MiB 8.09 MiB 962.72 KiB
9433f35+dirty 7.15 MiB 8.08 MiB 959.34 KiB
76d1baf+dirty 7.15 MiB 8.09 MiB 964.41 KiB
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB
457e29f+dirty 7.15 MiB 8.10 MiB 981.29 KiB

Previous results on branch: kw-debug-id-metro-plugin

Startup times

Revision Plain With Sentry Diff
6fa4f32+dirty 351.06 ms 383.24 ms 32.18 ms
116ff59+dirty 322.52 ms 367.76 ms 45.24 ms
7c3bc66+dirty 281.94 ms 337.48 ms 55.54 ms

App size

Revision Plain With Sentry Diff
6fa4f32+dirty 7.15 MiB 8.10 MiB 980.72 KiB
116ff59+dirty 7.15 MiB 8.10 MiB 980.72 KiB
7c3bc66+dirty 7.15 MiB 8.10 MiB 980.72 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1254.76 ms 1273.63 ms 18.87 ms
Size 2.92 MiB 3.43 MiB 524.74 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
27ef4ee+dirty 1236.41 ms 1244.90 ms 8.49 ms
0677344+dirty 1252.52 ms 1254.08 ms 1.56 ms
3853f43+dirty 1271.74 ms 1278.04 ms 6.30 ms
ad6c299+dirty 1248.50 ms 1248.88 ms 0.38 ms
34aba08+dirty 1268.58 ms 1276.80 ms 8.22 ms
d197b5c+dirty 1234.80 ms 1249.20 ms 14.40 ms
9433f35+dirty 1232.24 ms 1232.74 ms 0.50 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
80b2ce3+dirty 1245.12 ms 1262.04 ms 16.92 ms
457e29f+dirty 1256.71 ms 1258.50 ms 1.79 ms

App size

Revision Plain With Sentry Diff
27ef4ee+dirty 2.92 MiB 3.41 MiB 503.72 KiB
0677344+dirty 2.92 MiB 3.41 MiB 500.94 KiB
3853f43+dirty 2.92 MiB 3.41 MiB 503.54 KiB
ad6c299+dirty 2.92 MiB 3.40 MiB 494.12 KiB
34aba08+dirty 2.92 MiB 3.41 MiB 499.03 KiB
d197b5c+dirty 2.92 MiB 3.37 MiB 464.41 KiB
9433f35+dirty 2.92 MiB 3.41 MiB 503.55 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
80b2ce3+dirty 2.92 MiB 3.40 MiB 492.75 KiB
457e29f+dirty 2.92 MiB 3.43 MiB 524.75 KiB

Previous results on branch: kw-debug-id-metro-plugin

Startup times

Revision Plain With Sentry Diff
6fa4f32+dirty 1238.94 ms 1242.34 ms 3.40 ms
116ff59+dirty 1311.49 ms 1313.53 ms 2.04 ms
7c3bc66+dirty 1268.16 ms 1283.79 ms 15.63 ms

App size

Revision Plain With Sentry Diff
6fa4f32+dirty 2.92 MiB 3.43 MiB 524.68 KiB
116ff59+dirty 2.92 MiB 3.43 MiB 524.57 KiB
7c3bc66+dirty 2.92 MiB 3.43 MiB 524.52 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2023

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 437.96 ms 459.06 ms 21.10 ms
Size 17.73 MiB 19.84 MiB 2.10 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3ffcddd 302.92 ms 315.80 ms 12.88 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
0db0c72 372.12 ms 386.00 ms 13.88 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
abb7058 370.27 ms 389.58 ms 19.31 ms
76d1baf+dirty 335.72 ms 355.52 ms 19.80 ms
0677344 327.74 ms 337.14 ms 9.40 ms
9c48b2c 349.24 ms 385.96 ms 36.72 ms
457e29f 398.10 ms 421.39 ms 23.29 ms
8900e1a+dirty 430.68 ms 456.13 ms 25.44 ms

App size

Revision Plain With Sentry Diff
3ffcddd 17.73 MiB 19.75 MiB 2.02 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
0db0c72 17.73 MiB 19.75 MiB 2.02 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
abb7058 17.73 MiB 19.83 MiB 2.10 MiB
76d1baf+dirty 17.73 MiB 20.04 MiB 2.31 MiB
0677344 17.73 MiB 19.81 MiB 2.07 MiB
9c48b2c 17.73 MiB 19.80 MiB 2.07 MiB
457e29f 17.73 MiB 19.84 MiB 2.10 MiB
8900e1a+dirty 17.73 MiB 19.75 MiB 2.01 MiB

Previous results on branch: kw-debug-id-metro-plugin

Startup times

Revision Plain With Sentry Diff
7c3bc66 384.26 ms 411.58 ms 27.32 ms
116ff59 431.96 ms 442.77 ms 10.81 ms
6fa4f32 336.15 ms 352.29 ms 16.14 ms

App size

Revision Plain With Sentry Diff
7c3bc66 17.73 MiB 19.83 MiB 2.10 MiB
116ff59 17.73 MiB 19.83 MiB 2.10 MiB
6fa4f32 17.73 MiB 19.83 MiB 2.10 MiB

src/js/tools/sentryMetroSerializer.ts Dismissed Show dismissed Hide dismissed
@krystofwoldrich krystofwoldrich marked this pull request as ready for review September 26, 2023 09:57
@krystofwoldrich
Copy link
Member Author

iOS e2e tests are not happy as they need getsentry/sentry-cli#1754 release to work.

Besides that this RP is ready for a review.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

1st pass, excited for this!

typings/metro.d.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
@krystofwoldrich
Copy link
Member Author

@AbhiPrasad Thanks for the comments, I've shaken things up, should be better now.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Really good work @krystofwoldrich

scripts/has-sourcemap-debugid.js Outdated Show resolved Hide resolved
const sourceMapPath = process.argv[2];

if (!sourceMapPath) {
console.log('Add source map path as first argument of the script.');
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 console.warn or console.error these? Ditto for the console logs that are below this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that's more correct, but I had issues reading std err output in https://github.com/getsentry/sentry-react-native/pull/3164/files#diff-01c94c11105e37de1b11a82973b605f625fff5959b0386c32fd1e42a14325208R125-R126

So that's why I kept it console.log. More important is that we have the log.

src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
src/js/tools/sentryMetroSerializer.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

The part related to cocoa LGTM.

@krystofwoldrich krystofwoldrich merged commit 185debc into main Oct 5, 2023
51 checks passed
@krystofwoldrich krystofwoldrich deleted the kw-debug-id-metro-plugin branch October 5, 2023 12:45
Comment on lines +130 to +132
extraArgs.addAll(notIncludeRelease ? [] : [
"--release", releaseName,
"--dist", versionCode
Copy link

Choose a reason for hiding this comment

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

@krystofwoldrich Why are release and dist not being included if Debug ID are included. Why one or the other vs both?

Copy link
Member Author

Choose a reason for hiding this comment

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

Answered in #3474

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.

4 participants