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(core): Use makeDsn from core to extract the URL from DSN for filtering breadcrumbs #4395

Merged
merged 10 commits into from
Jan 16, 2025

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Dec 19, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Uses makeDsn from sentry/core to extract the URL from DSN for filtering breadcrumbs instead of relying on the implementation of the URL protocol #4240 that may result in errors like the following on some environments:

Failed to extract url from DSN [Error: URL.protocol is not implemented] [Component Stack]

💡 Motivation and Context

See #4375 (comment)

💚 How did you test it?

Manual testing, Existing unit tests

📝 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

@antonis antonis marked this pull request as ready for review December 19, 2024 13:39
Copy link
Contributor

github-actions bot commented Dec 19, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 472.91 ms 456.02 ms -16.89 ms
Size 17.75 MiB 20.11 MiB 2.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e73f4ed+dirty 332.96 ms 354.33 ms 21.37 ms
9cd0e9f 449.65 ms 433.39 ms -16.26 ms
8de2810 430.47 ms 428.72 ms -1.75 ms
946a600 384.53 ms 366.65 ms -17.88 ms
8e80789 430.76 ms 431.45 ms 0.69 ms
70e6261 482.65 ms 495.70 ms 13.05 ms
6e8584e 447.10 ms 474.71 ms 27.61 ms
c830127 407.57 ms 409.50 ms 1.93 ms
db44eaf 437.65 ms 436.06 ms -1.59 ms
700cbf4 425.56 ms 436.26 ms 10.70 ms

App size

Revision Plain With Sentry Diff
e73f4ed+dirty 17.73 MiB 20.04 MiB 2.31 MiB
9cd0e9f 17.74 MiB 20.08 MiB 2.34 MiB
8de2810 17.74 MiB 20.08 MiB 2.34 MiB
946a600 17.74 MiB 20.09 MiB 2.35 MiB
8e80789 17.74 MiB 20.10 MiB 2.36 MiB
70e6261 17.73 MiB 19.94 MiB 2.21 MiB
6e8584e 17.73 MiB 19.86 MiB 2.12 MiB
c830127 17.74 MiB 20.10 MiB 2.36 MiB
db44eaf 17.74 MiB 20.08 MiB 2.35 MiB
700cbf4 17.73 MiB 20.07 MiB 2.33 MiB

Previous results on branch: antonis/extract-url-with-regex

Startup times

Revision Plain With Sentry Diff
bf46362 474.62 ms 461.50 ms -13.12 ms
cf0b7fc 431.94 ms 424.68 ms -7.26 ms
98cfad8 368.76 ms 351.36 ms -17.40 ms
fb2547b 456.13 ms 481.60 ms 25.47 ms

App size

Revision Plain With Sentry Diff
bf46362 17.75 MiB 20.11 MiB 2.36 MiB
cf0b7fc 17.74 MiB 20.10 MiB 2.36 MiB
98cfad8 17.75 MiB 20.11 MiB 2.36 MiB
fb2547b 17.74 MiB 20.10 MiB 2.36 MiB

Copy link
Contributor

github-actions bot commented Dec 19, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 392.98 ms 456.00 ms 63.02 ms
Size 7.15 MiB 8.38 MiB 1.23 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0ebca77+dirty 360.94 ms 402.24 ms 41.30 ms
70caa60+dirty 308.83 ms 393.06 ms 84.23 ms
abb7058+dirty 320.78 ms 324.08 ms 3.30 ms
8fe7c9d+dirty 363.91 ms 429.93 ms 66.03 ms
700cbf4+dirty 411.71 ms 485.52 ms 73.81 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
e22745e+dirty 415.50 ms 448.76 ms 33.26 ms
c639edf+dirty 363.39 ms 414.78 ms 51.39 ms
75774ea+dirty 426.80 ms 455.43 ms 28.62 ms
b6f8ea2+dirty 397.51 ms 457.40 ms 59.88 ms

App size

Revision Plain With Sentry Diff
0ebca77+dirty 7.15 MiB 8.22 MiB 1.07 MiB
70caa60+dirty 7.15 MiB 8.03 MiB 901.79 KiB
abb7058+dirty 7.15 MiB 8.10 MiB 980.40 KiB
8fe7c9d+dirty 7.15 MiB 8.38 MiB 1.23 MiB
700cbf4+dirty 7.15 MiB 8.34 MiB 1.19 MiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
e22745e+dirty 7.15 MiB 8.35 MiB 1.20 MiB
c639edf+dirty 7.15 MiB 8.35 MiB 1.20 MiB
75774ea+dirty 7.15 MiB 8.36 MiB 1.21 MiB
b6f8ea2+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Previous results on branch: antonis/extract-url-with-regex

Startup times

Revision Plain With Sentry Diff
bf46362+dirty 367.76 ms 416.91 ms 49.15 ms
cf0b7fc+dirty 485.25 ms 471.55 ms -13.70 ms
fb2547b+dirty 388.51 ms 464.65 ms 76.14 ms
98cfad8+dirty 382.38 ms 418.00 ms 35.62 ms

App size

Revision Plain With Sentry Diff
bf46362+dirty 7.15 MiB 8.38 MiB 1.23 MiB
cf0b7fc+dirty 7.15 MiB 8.38 MiB 1.23 MiB
fb2547b+dirty 7.15 MiB 8.38 MiB 1.23 MiB
98cfad8+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Copy link
Contributor

github-actions bot commented Dec 19, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.37 ms 1234.25 ms 3.88 ms
Size 3.19 MiB 4.25 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
db44eaf+dirty 1238.49 ms 1236.56 ms -1.93 ms
70e6261+dirty 1224.90 ms 1231.02 ms 6.12 ms
e2b64fe+dirty 1285.78 ms 1297.56 ms 11.78 ms
1d86dd6+dirty 1289.25 ms 1293.36 ms 4.11 ms
9cd0e9f+dirty 1244.61 ms 1247.43 ms 2.82 ms
1332acb+dirty 1243.98 ms 1241.12 ms -2.86 ms
18ce5e8+dirty 1244.67 ms 1242.96 ms -1.72 ms
c398f67+dirty 1227.31 ms 1230.00 ms 2.69 ms
5bb8d5f+dirty 1215.04 ms 1217.52 ms 2.48 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
db44eaf+dirty 2.92 MiB 3.66 MiB 761.15 KiB
70e6261+dirty 2.92 MiB 3.59 MiB 686.11 KiB
e2b64fe+dirty 2.92 MiB 3.41 MiB 499.97 KiB
1d86dd6+dirty 2.92 MiB 3.44 MiB 538.27 KiB
9cd0e9f+dirty 2.92 MiB 3.64 MiB 741.23 KiB
1332acb+dirty 2.92 MiB 3.67 MiB 772.45 KiB
18ce5e8+dirty 2.92 MiB 3.69 MiB 789.94 KiB
c398f67+dirty 2.92 MiB 3.60 MiB 701.89 KiB
5bb8d5f+dirty 2.92 MiB 3.48 MiB 575.85 KiB

Previous results on branch: antonis/extract-url-with-regex

Startup times

Revision Plain With Sentry Diff
fb2547b+dirty 1242.14 ms 1240.33 ms -1.82 ms
cf0b7fc+dirty 1238.58 ms 1224.85 ms -13.73 ms
bf46362+dirty 1239.57 ms 1244.50 ms 4.93 ms
98cfad8+dirty 1225.21 ms 1220.22 ms -4.98 ms

App size

Revision Plain With Sentry Diff
fb2547b+dirty 2.92 MiB 3.69 MiB 790.20 KiB
cf0b7fc+dirty 2.92 MiB 3.69 MiB 790.66 KiB
bf46362+dirty 3.19 MiB 4.25 MiB 1.06 MiB
98cfad8+dirty 3.19 MiB 4.25 MiB 1.06 MiB

Copy link
Contributor

github-actions bot commented Dec 19, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1229.02 ms 1227.53 ms -1.49 ms
Size 2.63 MiB 3.69 MiB 1.05 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
700cbf4+dirty 1234.59 ms 1227.71 ms -6.88 ms
43e66e0+dirty 1226.82 ms 1225.92 ms -0.90 ms
c830127+dirty 1227.06 ms 1225.19 ms -1.88 ms
3ffcddd+dirty 1244.47 ms 1264.14 ms 19.67 ms
0677344+dirty 1276.70 ms 1300.07 ms 23.37 ms
a989877+dirty 1228.56 ms 1227.71 ms -0.85 ms
6e8584e+dirty 1274.50 ms 1296.82 ms 22.32 ms
07e58c9+dirty 1226.02 ms 1228.35 ms 2.33 ms
e22745e+dirty 1222.73 ms 1224.98 ms 2.25 ms
8c88ac7+dirty 1205.13 ms 1218.87 ms 13.74 ms

App size

Revision Plain With Sentry Diff
700cbf4+dirty 2.36 MiB 3.08 MiB 734.22 KiB
43e66e0+dirty 2.36 MiB 3.11 MiB 759.78 KiB
c830127+dirty 2.36 MiB 3.12 MiB 778.80 KiB
3ffcddd+dirty 2.36 MiB 2.84 MiB 489.60 KiB
0677344+dirty 2.36 MiB 2.85 MiB 496.81 KiB
a989877+dirty 2.36 MiB 3.10 MiB 752.40 KiB
6e8584e+dirty 2.36 MiB 2.88 MiB 533.17 KiB
07e58c9+dirty 2.36 MiB 3.10 MiB 752.28 KiB
e22745e+dirty 2.36 MiB 3.10 MiB 752.32 KiB
8c88ac7+dirty 2.36 MiB 3.10 MiB 752.63 KiB

Previous results on branch: antonis/extract-url-with-regex

Startup times

Revision Plain With Sentry Diff
fb2547b+dirty 1232.60 ms 1244.00 ms 11.40 ms
cf0b7fc+dirty 1226.64 ms 1229.61 ms 2.97 ms
bf46362+dirty 1223.02 ms 1229.31 ms 6.28 ms
98cfad8+dirty 1231.73 ms 1225.02 ms -6.71 ms

App size

Revision Plain With Sentry Diff
fb2547b+dirty 2.36 MiB 3.12 MiB 778.85 KiB
cf0b7fc+dirty 2.36 MiB 3.12 MiB 779.48 KiB
bf46362+dirty 2.63 MiB 3.68 MiB 1.05 MiB
98cfad8+dirty 2.63 MiB 3.68 MiB 1.05 MiB

Comment on lines 70 to 74
const regex = /^(https?):\/\/(?:[^@]+@)?([^/]+)(?:\/.*)?$/;
const matches = dsn.match(regex);

if (matches) {
const [, protocol, host] = matches;
Copy link
Member

Choose a reason for hiding this comment

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

I've missed that in the original review as well, but let's use the makeDsn function from sentry/core. It already uses regex and is used in the client impl, so it 100% works with RN.

https://github.com/getsentry/sentry-javascript/blob/6f56d063e479f90ce07a5ddc0636dc0bc3b3bb2e/packages/core/src/utils-hoist/dsn.ts#L122

Copy link
Collaborator Author

@antonis antonis Jan 10, 2025

Choose a reason for hiding this comment

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

Good idea @krystofwoldrich 👍
Updated with a1ecb49 to use the makeDsn function.

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Jan 9, 2025

Just for completeness this is the default RN implementation.

https://github.com/facebook/react-native/blob/51ffc5cc12fb476a16af30d9aaadd34a6b625439/packages/react-native/Libraries/Blob/URL.js#L142

But it seem like some dependency polyfills it in our sample apps.

@antonis antonis changed the title fix(core): Use regex to extract URL from DSN for filtering breadcrumbs fix(core): Use makeDsn from core to extract the URL from DSN for filtering breadcrumbs Jan 10, 2025
@krystofwoldrich
Copy link
Member

Looks good, just a changelog entry is missing.

@antonis
Copy link
Collaborator Author

antonis commented Jan 16, 2025

Thank you for reviewing @krystofwoldrich 🙇
Added a changelog with 32943e9

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@krystofwoldrich krystofwoldrich merged commit a38594f into main Jan 16, 2025
70 of 71 checks passed
@krystofwoldrich krystofwoldrich deleted the antonis/extract-url-with-regex branch January 16, 2025 12:32
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.

3 participants