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): Breadcrumbs added on forked context are now captured #4124

Merged
merged 34 commits into from
Nov 18, 2024

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Sep 27, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Uses breadcrumb.origin field to prevent exception capture context from being overwritten by native scope sync.

Depends on:

💡 Motivation and Context

Fixes #2146
The discussion in getsentry/sentry-capacitor#629 (comment) adds some context on the need of a new field to be used internally to solve a technical problem.

💚 How did you test it?

Manual testing with the Capture exception with breadcrumb button, CI

Before After iOS After Android
main ios android

📝 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

Copy link
Contributor

github-actions bot commented Oct 8, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.45 ms 1234.88 ms 13.43 ms
Size 2.36 MiB 3.10 MiB 752.43 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
62a750b+dirty 1216.60 ms 1229.14 ms 12.54 ms
a989877+dirty 1228.56 ms 1227.71 ms -0.85 ms
484813b+dirty 1222.45 ms 1220.79 ms -1.66 ms
700cbf4+dirty 1234.59 ms 1227.71 ms -6.88 ms
e73d82f+dirty 1207.52 ms 1216.73 ms 9.21 ms
8900e1a+dirty 1210.27 ms 1218.66 ms 8.39 ms
dadc233+dirty 1223.20 ms 1236.88 ms 13.68 ms
d7401ac+dirty 1252.38 ms 1275.04 ms 22.66 ms
b1e8712+dirty 1256.02 ms 1265.14 ms 9.12 ms
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms

App size

Revision Plain With Sentry Diff
62a750b+dirty 2.36 MiB 2.92 MiB 570.00 KiB
a989877+dirty 2.36 MiB 3.10 MiB 752.40 KiB
484813b+dirty 2.36 MiB 3.08 MiB 734.18 KiB
700cbf4+dirty 2.36 MiB 3.08 MiB 734.22 KiB
e73d82f+dirty 2.36 MiB 3.08 MiB 734.23 KiB
8900e1a+dirty 2.36 MiB 2.83 MiB 479.25 KiB
dadc233+dirty 2.36 MiB 2.84 MiB 486.85 KiB
d7401ac+dirty 2.36 MiB 2.83 MiB 481.14 KiB
b1e8712+dirty 2.36 MiB 2.84 MiB 488.84 KiB
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB

Previous results on branch: antonis/add-breadcrumb-origin

Startup times

Revision Plain With Sentry Diff
9cee3c7+dirty 1211.82 ms 1220.43 ms 8.61 ms
093a11f+dirty 1212.48 ms 1229.21 ms 16.73 ms
e045c61+dirty 1236.80 ms 1238.87 ms 2.08 ms
e62bf30+dirty 1238.96 ms 1243.39 ms 4.43 ms
2bd46a7+dirty 1218.10 ms 1220.43 ms 2.32 ms
f7ad13b+dirty 1217.18 ms 1220.83 ms 3.65 ms
c84e457+dirty 1218.30 ms 1224.08 ms 5.78 ms
168e871+dirty 1205.87 ms 1221.37 ms 15.50 ms
17fc2f7+dirty 1216.04 ms 1220.39 ms 4.35 ms

App size

Revision Plain With Sentry Diff
9cee3c7+dirty 2.36 MiB 3.08 MiB 737.39 KiB
093a11f+dirty 2.36 MiB 3.10 MiB 752.44 KiB
e045c61+dirty 2.36 MiB 3.10 MiB 753.19 KiB
e62bf30+dirty 2.36 MiB 3.15 MiB 802.94 KiB
2bd46a7+dirty 2.36 MiB 3.10 MiB 752.72 KiB
f7ad13b+dirty 2.36 MiB 3.10 MiB 753.14 KiB
c84e457+dirty 2.36 MiB 3.10 MiB 751.68 KiB
168e871+dirty 2.36 MiB 3.08 MiB 736.99 KiB
17fc2f7+dirty 2.36 MiB 3.08 MiB 737.22 KiB

Copy link
Contributor

github-actions bot commented Oct 8, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.69 ms 1227.90 ms 5.21 ms
Size 2.92 MiB 3.66 MiB 757.00 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
62a750b+dirty 1228.12 ms 1230.53 ms 2.41 ms
a989877+dirty 1222.90 ms 1219.89 ms -3.00 ms
484813b+dirty 1225.07 ms 1221.00 ms -4.07 ms
700cbf4+dirty 1233.96 ms 1228.27 ms -5.69 ms
e73d82f+dirty 1231.20 ms 1228.81 ms -2.40 ms
8900e1a+dirty 1268.36 ms 1273.04 ms 4.68 ms
dadc233+dirty 1266.52 ms 1282.55 ms 16.03 ms
d7401ac+dirty 1288.10 ms 1289.54 ms 1.44 ms
b1e8712+dirty 1284.11 ms 1297.82 ms 13.71 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms

App size

Revision Plain With Sentry Diff
62a750b+dirty 2.92 MiB 3.48 MiB 575.59 KiB
a989877+dirty 2.92 MiB 3.66 MiB 757.66 KiB
484813b+dirty 2.92 MiB 3.64 MiB 740.56 KiB
700cbf4+dirty 2.92 MiB 3.64 MiB 740.57 KiB
e73d82f+dirty 2.92 MiB 3.64 MiB 740.56 KiB
8900e1a+dirty 2.92 MiB 3.39 MiB 485.96 KiB
dadc233+dirty 2.92 MiB 3.40 MiB 492.53 KiB
d7401ac+dirty 2.92 MiB 3.40 MiB 488.06 KiB
b1e8712+dirty 2.92 MiB 3.40 MiB 494.15 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB

Previous results on branch: antonis/add-breadcrumb-origin

Startup times

Revision Plain With Sentry Diff
9cee3c7+dirty 1237.08 ms 1232.16 ms -4.92 ms
093a11f+dirty 1238.33 ms 1237.33 ms -1.01 ms
e045c61+dirty 1225.21 ms 1230.33 ms 5.12 ms
e62bf30+dirty 1239.42 ms 1242.14 ms 2.73 ms
2bd46a7+dirty 1230.79 ms 1232.40 ms 1.61 ms
f7ad13b+dirty 1237.70 ms 1239.14 ms 1.44 ms
c84e457+dirty 1229.50 ms 1226.76 ms -2.74 ms
168e871+dirty 1234.31 ms 1227.67 ms -6.64 ms
17fc2f7+dirty 1234.25 ms 1234.78 ms 0.53 ms

App size

Revision Plain With Sentry Diff
9cee3c7+dirty 2.92 MiB 3.64 MiB 743.04 KiB
093a11f+dirty 2.92 MiB 3.66 MiB 757.73 KiB
e045c61+dirty 2.92 MiB 3.66 MiB 758.40 KiB
e62bf30+dirty 2.92 MiB 3.71 MiB 808.09 KiB
2bd46a7+dirty 2.92 MiB 3.66 MiB 757.44 KiB
f7ad13b+dirty 2.92 MiB 3.66 MiB 758.41 KiB
c84e457+dirty 2.92 MiB 3.66 MiB 756.01 KiB
168e871+dirty 2.92 MiB 3.64 MiB 742.68 KiB
17fc2f7+dirty 2.92 MiB 3.64 MiB 743.07 KiB

Copy link
Contributor

github-actions bot commented Oct 10, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 369.18 ms 398.42 ms 29.24 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

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
f06c879+dirty 361.27 ms 407.88 ms 46.61 ms
8900e1a+dirty 371.40 ms 377.70 ms 6.31 ms
e540498+dirty 408.56 ms 480.00 ms 71.44 ms
62a750b+dirty 370.78 ms 376.73 ms 5.96 ms
2ec71da+dirty 375.64 ms 431.59 ms 55.95 ms
c639edf+dirty 363.39 ms 414.78 ms 51.39 ms
acadc0f+dirty 259.04 ms 304.67 ms 45.63 ms
27ef4ee+dirty 296.71 ms 351.00 ms 54.29 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
f06c879+dirty 7.15 MiB 8.12 MiB 997.78 KiB
8900e1a+dirty 7.15 MiB 8.03 MiB 901.79 KiB
e540498+dirty 7.15 MiB 8.38 MiB 1.23 MiB
62a750b+dirty 7.15 MiB 8.21 MiB 1.06 MiB
2ec71da+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c639edf+dirty 7.15 MiB 8.35 MiB 1.20 MiB
acadc0f+dirty 7.15 MiB 8.03 MiB 903.20 KiB
27ef4ee+dirty 7.15 MiB 8.08 MiB 959.49 KiB

Previous results on branch: antonis/add-breadcrumb-origin

Startup times

Revision Plain With Sentry Diff
17fc2f7+dirty 409.50 ms 462.68 ms 53.18 ms
093a11f+dirty 415.54 ms 461.00 ms 45.46 ms
9cee3c7+dirty 397.49 ms 454.02 ms 56.53 ms
f7ad13b+dirty 478.18 ms 564.25 ms 86.07 ms
168e871+dirty 508.96 ms 599.09 ms 90.13 ms
c84e457+dirty 395.52 ms 432.24 ms 36.72 ms
e62bf30+dirty 369.36 ms 405.27 ms 35.92 ms
e045c61+dirty 434.98 ms 463.36 ms 28.38 ms

App size

Revision Plain With Sentry Diff
17fc2f7+dirty 7.15 MiB 8.35 MiB 1.20 MiB
093a11f+dirty 7.15 MiB 8.35 MiB 1.20 MiB
9cee3c7+dirty 7.15 MiB 8.35 MiB 1.20 MiB
f7ad13b+dirty 7.15 MiB 8.35 MiB 1.20 MiB
168e871+dirty 7.15 MiB 8.35 MiB 1.20 MiB
c84e457+dirty 7.15 MiB 8.35 MiB 1.20 MiB
e62bf30+dirty 7.15 MiB 8.39 MiB 1.24 MiB
e045c61+dirty 7.15 MiB 8.35 MiB 1.20 MiB

Copy link
Contributor

github-actions bot commented Oct 11, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 459.00 ms 442.70 ms -16.30 ms
Size 17.74 MiB 20.08 MiB 2.34 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c639edf 466.48 ms 489.57 ms 23.09 ms
3853f43 329.68 ms 346.32 ms 16.64 ms
76d1baf+dirty 335.72 ms 355.52 ms 19.80 ms
4cc5c27 460.04 ms 496.32 ms 36.28 ms
86d6d2c+dirty 332.90 ms 352.45 ms 19.55 ms
d2c32bb 448.85 ms 450.19 ms 1.34 ms
2534337 394.15 ms 415.12 ms 20.97 ms
8900e1a+dirty 430.68 ms 456.13 ms 25.44 ms
b1e8712 462.11 ms 465.71 ms 3.60 ms
cdf2f33 469.46 ms 462.17 ms -7.29 ms

App size

Revision Plain With Sentry Diff
c639edf 17.74 MiB 20.08 MiB 2.34 MiB
3853f43 17.73 MiB 19.81 MiB 2.08 MiB
76d1baf+dirty 17.73 MiB 20.04 MiB 2.31 MiB
4cc5c27 17.73 MiB 19.95 MiB 2.21 MiB
86d6d2c+dirty 17.73 MiB 20.04 MiB 2.31 MiB
d2c32bb 17.74 MiB 20.08 MiB 2.34 MiB
2534337 17.73 MiB 19.84 MiB 2.11 MiB
8900e1a+dirty 17.73 MiB 19.75 MiB 2.01 MiB
b1e8712 17.73 MiB 19.75 MiB 2.02 MiB
cdf2f33 17.74 MiB 20.08 MiB 2.34 MiB

Previous results on branch: antonis/add-breadcrumb-origin

Startup times

Revision Plain With Sentry Diff
168e871 471.56 ms 451.59 ms -19.97 ms
f7ad13b 488.43 ms 500.04 ms 11.62 ms
e045c61 395.70 ms 386.29 ms -9.41 ms
093a11f 471.67 ms 487.81 ms 16.15 ms
9cee3c7 509.72 ms 513.58 ms 3.86 ms
e62bf30 481.92 ms 471.48 ms -10.44 ms
17fc2f7 420.78 ms 512.84 ms 92.07 ms
c84e457 490.41 ms 489.28 ms -1.13 ms

App size

Revision Plain With Sentry Diff
168e871 17.74 MiB 20.08 MiB 2.34 MiB
f7ad13b 17.74 MiB 20.07 MiB 2.34 MiB
e045c61 17.74 MiB 20.07 MiB 2.34 MiB
093a11f 17.74 MiB 20.07 MiB 2.34 MiB
9cee3c7 17.74 MiB 20.08 MiB 2.34 MiB
e62bf30 17.73 MiB 20.11 MiB 2.38 MiB
17fc2f7 17.74 MiB 20.08 MiB 2.34 MiB
c84e457 17.74 MiB 20.08 MiB 2.34 MiB

# Conflicts:
#	packages/core/android/src/main/java/io/sentry/react/RNSentryBreadcrumb.java
@antonis
Copy link
Collaborator Author

antonis commented Oct 28, 2024

Hey @lucas-zimerman, @krystofwoldrich 👋

Fixes #2146
The discussion in getsentry/sentry-capacitor#629 (comment) adds some context on the need of a new field to be used internally to solve a technical problem.

Since some time has passed since the creation of the issue and the discussion above I'd like to verify that the direction I've chosen with this implementation is valid:

Any feedback is welcome 🙏

@lucas-zimerman
Copy link
Collaborator

lucas-zimerman commented Oct 29, 2024

Hey @lucas-zimerman, @krystofwoldrich 👋

Fixes #2146
The discussion in getsentry/sentry-capacitor#629 (comment) adds some context on the need of a new field to be used internally to solve a technical problem.

Since some time has passed since the creation of the issue and the discussion above I'd like to verify that the direction I've chosen with this implementation is valid:

* [Set react-native as breadcrumb origin](https://github.com/getsentry/sentry-react-native/pull/4124/commits/24a527be83850ad4a63b47920b025fc1f7fe7003)

* [Filters out breadcrumbs with `react-native` origin from the native layer](https://github.com/getsentry/sentry-react-native/pull/4124/commits/fabf0526a8b9d70a75c703b2d29accbc09e4fe93)

* [Merges event breadcrumbs with native context](https://github.com/getsentry/sentry-react-native/pull/4124/commits/d0b163342380980a878c340f8ad55df08e86d1a5)

Any feedback is welcome 🙏

The approach looks good indeed, thank you!
One thing to note is we should respect maxBreadcrumbs and limit the merged breadcrumbs to the defined value (or 100 if not defined).

@krystofwoldrich
Copy link
Member

Thank you @antonis for checking this.

Set react-native as breadcrumb origin
Filters out breadcrumbs with react-native origin from the native layer
Merges event breadcrumbs with native context

Yes, all tree steps look good.

# Conflicts:
#	CHANGELOG.md
#	packages/core/RNSentryCocoaTester/RNSentryCocoaTester.xcodeproj/project.pbxproj
@krystofwoldrich
Copy link
Member

@antonis Thank you for the updates. It looks great, I have one last small comment regarding @lucas-zimerman caching question. Then it's good to 🚀

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.

LG 🚀 Thank you!

@krystofwoldrich
Copy link
Member

This PR doesn't trigger our dangerous file warning, but I thought anyway we might want to release this as a shot beta before latest release.

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.

Looks good too, yeah, it would be great if this PR goes first on a beta version.

@antonis
Copy link
Collaborator Author

antonis commented Nov 11, 2024

I thought anyway we might want to release this as a shot beta before latest release.

This sounds like a good idea ➕

@antonis antonis changed the title Fix: Breadcrumbs added on forked context are now captured fix(core): Breadcrumbs added on forked context are now captured Nov 14, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@krystofwoldrich krystofwoldrich merged commit fd54b38 into main Nov 18, 2024
55 of 57 checks passed
@krystofwoldrich krystofwoldrich deleted the antonis/add-breadcrumb-origin branch November 18, 2024 11:50
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.

Exception capture context is overwritten by native scope sync
3 participants