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(integrations): Add Spotlight integration #3550

Merged
merged 8 commits into from
Jan 29, 2024
Merged

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Jan 25, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Attempts to match Browser Integration as close as possible

This PR adds Spotlight integration for React Native and Expo applications.

The integration auto-detects the dev server hostname and expects spotlight/sidecar to be available on the same hostname.

Screenshot 2024-01-25 at 00 01 32 Screenshot 2024-01-25 at 00 01 20 Screenshot 2024-01-25 at 00 00 33

💚 How did you test it?

sample apps

📝 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

  • Add tests to verify the original envelope is not changed

@krystofwoldrich krystofwoldrich changed the title feat(integrations): Add Spotlight integration with auto dev server hostname detection feat(integrations): Add Spotlight integration Jan 25, 2024
Copy link
Contributor

github-actions bot commented Jan 25, 2024

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

Generated by 🚫 dangerJS against 0c2640c

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 444.00 ms 482.92 ms 38.92 ms
Size 17.73 MiB 19.85 MiB 2.11 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
12427f4 393.69 ms 414.84 ms 21.14 ms
5446992 403.40 ms 426.70 ms 23.30 ms
dadc233+dirty 333.78 ms 343.94 ms 10.16 ms
22e31b6 396.48 ms 419.64 ms 23.16 ms
f06c879 408.41 ms 424.54 ms 16.13 ms
b1e8712 462.11 ms 465.71 ms 3.60 ms
34aba08 328.10 ms 342.84 ms 14.74 ms
70caa60+dirty 299.00 ms 321.02 ms 22.02 ms
e73f4ed+dirty 332.96 ms 354.33 ms 21.37 ms
2534337 394.15 ms 415.12 ms 20.97 ms

App size

Revision Plain With Sentry Diff
12427f4 17.73 MiB 19.85 MiB 2.12 MiB
5446992 17.73 MiB 19.85 MiB 2.12 MiB
dadc233+dirty 17.73 MiB 19.75 MiB 2.02 MiB
22e31b6 17.73 MiB 19.84 MiB 2.10 MiB
f06c879 17.73 MiB 19.85 MiB 2.12 MiB
b1e8712 17.73 MiB 19.75 MiB 2.02 MiB
34aba08 17.73 MiB 19.80 MiB 2.07 MiB
70caa60+dirty 17.73 MiB 19.75 MiB 2.01 MiB
e73f4ed+dirty 17.73 MiB 20.04 MiB 2.31 MiB
2534337 17.73 MiB 19.84 MiB 2.11 MiB

Previous results on branch: kw-add-spotlight

Startup times

Revision Plain With Sentry Diff
c05d12c 416.23 ms 425.20 ms 8.97 ms

App size

Revision Plain With Sentry Diff
c05d12c 17.73 MiB 19.85 MiB 2.11 MiB

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 366.91 ms 388.49 ms 21.58 ms
Size 7.15 MiB 8.12 MiB 990.81 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
abb7058+dirty 320.78 ms 324.08 ms 3.30 ms
dadc233+dirty 363.19 ms 370.37 ms 7.18 ms
ad6c299+dirty 336.47 ms 362.89 ms 26.42 ms
d361d38+dirty 257.72 ms 318.76 ms 61.04 ms
e2b64fe+dirty 258.82 ms 304.26 ms 45.44 ms
e5c9b8b+dirty 335.40 ms 360.06 ms 24.67 ms
457e29f+dirty 591.49 ms 612.96 ms 21.47 ms
2534337+dirty 597.14 ms 665.04 ms 67.90 ms
70caa60+dirty 308.83 ms 393.06 ms 84.23 ms
5446992+dirty 371.61 ms 390.00 ms 18.39 ms

App size

Revision Plain With Sentry Diff
abb7058+dirty 7.15 MiB 8.10 MiB 980.40 KiB
dadc233+dirty 7.15 MiB 8.04 MiB 910.84 KiB
ad6c299+dirty 7.15 MiB 8.04 MiB 912.17 KiB
d361d38+dirty 7.15 MiB 8.08 MiB 959.34 KiB
e2b64fe+dirty 7.15 MiB 8.07 MiB 947.16 KiB
e5c9b8b+dirty 7.15 MiB 8.10 MiB 980.41 KiB
457e29f+dirty 7.15 MiB 8.10 MiB 981.29 KiB
2534337+dirty 7.15 MiB 8.11 MiB 988.68 KiB
70caa60+dirty 7.15 MiB 8.03 MiB 901.79 KiB
5446992+dirty 7.15 MiB 8.12 MiB 999.45 KiB

Previous results on branch: kw-add-spotlight

Startup times

Revision Plain With Sentry Diff
c05d12c+dirty 358.84 ms 390.00 ms 31.16 ms

App size

Revision Plain With Sentry Diff
c05d12c+dirty 7.15 MiB 8.12 MiB 990.81 KiB

Copy link
Contributor

github-actions bot commented Jan 25, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1204.63 ms 1205.39 ms 0.76 ms
Size 2.36 MiB 2.88 MiB 526.76 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9433f35+dirty 1246.94 ms 1271.45 ms 24.52 ms
9c48b2c+dirty 1246.96 ms 1255.73 ms 8.77 ms
1d86dd6+dirty 1249.71 ms 1279.16 ms 29.45 ms
d361d38+dirty 1246.04 ms 1267.12 ms 21.08 ms
12427f4+dirty 1267.15 ms 1271.30 ms 4.15 ms
6e8584e+dirty 1274.50 ms 1296.82 ms 22.32 ms
3ffcddd+dirty 1244.47 ms 1264.14 ms 19.67 ms
22e31b6+dirty 1253.62 ms 1265.96 ms 12.34 ms
0677344+dirty 1276.70 ms 1300.07 ms 23.37 ms
3853f43+dirty 1221.82 ms 1242.64 ms 20.82 ms

App size

Revision Plain With Sentry Diff
9433f35+dirty 2.36 MiB 2.85 MiB 499.80 KiB
9c48b2c+dirty 2.36 MiB 2.85 MiB 495.77 KiB
1d86dd6+dirty 2.36 MiB 2.89 MiB 535.43 KiB
d361d38+dirty 2.36 MiB 2.85 MiB 499.84 KiB
12427f4+dirty 2.36 MiB 2.88 MiB 530.38 KiB
6e8584e+dirty 2.36 MiB 2.88 MiB 533.17 KiB
3ffcddd+dirty 2.36 MiB 2.84 MiB 489.60 KiB
22e31b6+dirty 2.36 MiB 2.87 MiB 520.67 KiB
0677344+dirty 2.36 MiB 2.85 MiB 496.81 KiB
3853f43+dirty 2.36 MiB 2.85 MiB 499.81 KiB

@krystofwoldrich krystofwoldrich requested a review from Lms24 January 25, 2024 14:05
Copy link
Contributor

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.96 ms 1215.83 ms 2.88 ms
Size 2.92 MiB 3.44 MiB 531.17 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9433f35+dirty 1232.24 ms 1232.74 ms 0.50 ms
9c48b2c+dirty 1253.39 ms 1256.30 ms 2.91 ms
1d86dd6+dirty 1289.25 ms 1293.36 ms 4.11 ms
d361d38+dirty 1272.96 ms 1291.70 ms 18.74 ms
12427f4+dirty 1224.90 ms 1231.40 ms 6.50 ms
6e8584e+dirty 1271.71 ms 1281.26 ms 9.55 ms
3ffcddd+dirty 1272.22 ms 1273.98 ms 1.76 ms
22e31b6+dirty 1276.55 ms 1278.12 ms 1.57 ms
0677344+dirty 1252.52 ms 1254.08 ms 1.56 ms
3853f43+dirty 1271.74 ms 1278.04 ms 6.30 ms

App size

Revision Plain With Sentry Diff
9433f35+dirty 2.92 MiB 3.41 MiB 503.55 KiB
9c48b2c+dirty 2.92 MiB 3.41 MiB 499.97 KiB
1d86dd6+dirty 2.92 MiB 3.44 MiB 538.27 KiB
d361d38+dirty 2.92 MiB 3.41 MiB 503.57 KiB
12427f4+dirty 2.92 MiB 3.44 MiB 533.29 KiB
6e8584e+dirty 2.92 MiB 3.44 MiB 536.52 KiB
3ffcddd+dirty 2.92 MiB 3.40 MiB 494.39 KiB
22e31b6+dirty 2.92 MiB 3.43 MiB 524.74 KiB
0677344+dirty 2.92 MiB 3.41 MiB 500.94 KiB
3853f43+dirty 2.92 MiB 3.41 MiB 503.54 KiB

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice! I assume the usage instructions are gonna be similar to web frontend in the sense that we tell users to add the integration in dev mode? Can RN tree-shake the integration out in prod builds?

Other than that, LGTM!

Comment on lines +50 to +53
const spotlightEnvelope: Envelope = [...originalEnvelope];
const envelopeItems = [...originalEnvelope[1]].filter(
item => typeof item[0].content_type !== 'string' || !item[0].content_type.startsWith('image'),
);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this is a good point. I think we just support text data at the moment.

I think we should handle this in the sidecar somehow because we don't want SDKs to have too much of that logic. Ideally, Spotlight (similarly to Relay lol) accepts all kinds of payloads and just deals with removing incompatible stuff on its end.

Additionally, WDYT, would it be valuable to render the images in the Spotlight UI?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, no change required from my end. The workaround is fine for now, I just think we should handle this in the spotlight sidecar in the future

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 think rendering the screenshots would be especially useful for crashes/uncaught errors. So the developer doesn't have to try to crash the app again and carefully watch what was on the screen at that moment (or create a screenrecording).

src/js/integrations/spotlight.ts Show resolved Hide resolved
@krystofwoldrich krystofwoldrich marked this pull request as ready for review January 26, 2024 22:47
@krystofwoldrich krystofwoldrich merged commit eb53e59 into main Jan 29, 2024
48 of 51 checks passed
@krystofwoldrich krystofwoldrich deleted the kw-add-spotlight branch January 29, 2024 12:16
@krystofwoldrich
Copy link
Member Author

Yes, we tell users to enable it in dev builds, the same as on JS. Sadly RN only removes unimported files, so the integration code stays. This is something to revisit when RN supports tree shaking.

@kahest kahest mentioned this pull request Feb 12, 2024
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.

2 participants