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.

Sign up for GitHub

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

Jump to bottom

fix(frames): remove non absolute non url frame.abs_path coming from default stack parser #2891

Merged
merged 6 commits into from
Mar 14, 2023
Merged

fix(frames): remove non absolute non url frame.abs_path coming from default stack parser #2891

merged 6 commits into from
Mar 14, 2023

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Mar 13, 2023 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

In JS SDK frame.abs_path was added and that caused the from.abs_path to be relative. This lead to source maps failure in the product.

related changes in JS getsentry/sentry-javascript#7167

💡 Motivation and Context

fixes: #2879

💚 How did you test it?

📝 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 test so this won't happen again

krystofwoldrich
krystofwoldrich commented Mar 13, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
marandaneto
marandaneto reviewed Mar 13, 2023
View reviewed changes
src/js/sdk.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 423.74 ms 455.24 ms 31.50 ms
Size 17.73 MiB 20.04 MiB 2.31 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 335.72 ms 355.52 ms 19.80 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
52a8031+dirty 311.55 ms 321.37 ms 9.82 ms
9a3ca65+dirty 326.93 ms 330.14 ms 3.21 ms
d197b5c+dirty 338.94 ms 354.87 ms 15.93 ms
e73f4ed+dirty 332.96 ms 354.33 ms 21.37 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 17.73 MiB 20.04 MiB 2.31 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
52a8031+dirty 17.73 MiB 20.04 MiB 2.31 MiB
9a3ca65+dirty 17.73 MiB 20.04 MiB 2.31 MiB
d197b5c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
e73f4ed+dirty 17.73 MiB 20.04 MiB 2.31 MiB

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1263.32 ms 1279.75 ms 16.43 ms
Size 2.36 MiB 2.83 MiB 474.50 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
86d6d2c+dirty 1267.55 ms 1286.21 ms 18.66 ms
52a8031+dirty 1280.88 ms 1289.78 ms 8.90 ms
9a3ca65+dirty 1247.06 ms 1274.58 ms 27.52 ms
d197b5c+dirty 1217.61 ms 1242.66 ms 25.05 ms
e73f4ed+dirty 1243.27 ms 1244.52 ms 1.25 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
86d6d2c+dirty 2.36 MiB 2.82 MiB 462.82 KiB
52a8031+dirty 2.36 MiB 2.82 MiB 469.44 KiB
9a3ca65+dirty 2.36 MiB 2.82 MiB 462.89 KiB
d197b5c+dirty 2.36 MiB 2.82 MiB 462.86 KiB
e73f4ed+dirty 2.36 MiB 2.82 MiB 469.44 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1269.78 ms 1277.70 ms 7.92 ms
Size 2.92 MiB 3.39 MiB 481.60 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
86d6d2c+dirty 1291.62 ms 1296.80 ms 5.18 ms
52a8031+dirty 1255.96 ms 1273.00 ms 17.04 ms
9a3ca65+dirty 1276.40 ms 1279.14 ms 2.74 ms
d197b5c+dirty 1234.80 ms 1249.20 ms 14.40 ms
e73f4ed+dirty 1282.90 ms 1309.30 ms 26.40 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
86d6d2c+dirty 2.92 MiB 3.37 MiB 464.31 KiB
52a8031+dirty 2.92 MiB 3.38 MiB 475.71 KiB
9a3ca65+dirty 2.92 MiB 3.37 MiB 464.32 KiB
d197b5c+dirty 2.92 MiB 3.37 MiB 464.41 KiB
e73f4ed+dirty 2.92 MiB 3.38 MiB 475.71 KiB

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 260.79 ms 309.59 ms 48.80 ms
Size 7.15 MiB 8.09 MiB 966.13 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
76d1baf+dirty 339.02 ms 408.65 ms 69.63 ms
86d6d2c+dirty 267.21 ms 325.24 ms 58.04 ms
52a8031+dirty 330.72 ms 358.76 ms 28.03 ms
9a3ca65+dirty 344.96 ms 358.92 ms 13.96 ms
d197b5c+dirty 258.75 ms 313.61 ms 54.86 ms
e73f4ed+dirty 262.98 ms 311.02 ms 48.04 ms

App size

Revision Plain With Sentry Diff
76d1baf+dirty 7.15 MiB 8.09 MiB 964.41 KiB
86d6d2c+dirty 7.15 MiB 8.09 MiB 962.69 KiB
52a8031+dirty 7.15 MiB 8.09 MiB 965.95 KiB
9a3ca65+dirty 7.15 MiB 8.09 MiB 962.83 KiB
d197b5c+dirty 7.15 MiB 8.09 MiB 962.72 KiB
e73f4ed+dirty 7.15 MiB 8.09 MiB 965.94 KiB

@krystofwoldrich krystofwoldrich marked this pull request as ready for review March 13, 2023 20:48
@krystofwoldrich
Copy link
Member Author

@marandaneto I've added tests for the rewrite frames. Could you review it too?

marandaneto
marandaneto approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

I liked the approach of splitting up the RewriteFrames and making it testable + adding unit tests for multiple cases, thank you!

@krystofwoldrich krystofwoldrich mentioned this pull request Mar 14, 2023
Closed
@krystofwoldrich krystofwoldrich merged commit 15c80ab into main Mar 14, 2023
@krystofwoldrich krystofwoldrich deleted the kw-fix-source-maps-5-1-0 branch March 14, 2023 09:22
@Eden-Eliel Eden-Eliel mentioned this pull request Mar 23, 2023
Closed
11 tasks
@krystofwoldrich krystofwoldrich mentioned this pull request Jun 14, 2023
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marandaneto marandaneto marandaneto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The abs_path of the stack frame is index.android.bundle which is not a valid URL
2 participants
@krystofwoldrich @marandaneto