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: Adds span thread info #4579

Merged
merged 11 commits into from
Feb 25, 2025
Merged

feat: Adds span thread info #4579

merged 11 commits into from
Feb 25, 2025

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Feb 21, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Adds span thread info. This would be:

  • name: main for UI threads
  • name: javascript for JS threads

Ref: https://reactnative.dev/architecture/threading-model

💡 Motivation and Context

#4461

💚 How did you test it?

Manual, CI

📝 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 draft February 21, 2025 12:10
Copy link
Contributor

github-actions bot commented Feb 21, 2025

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

Generated by 🚫 dangerJS against 8226705

Copy link
Contributor

github-actions bot commented Feb 21, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 405.45 ms 409.35 ms 3.90 ms
Size 7.15 MiB 8.39 MiB 1.23 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c991c90+dirty 270.25 ms 290.85 ms 20.60 ms
0677344+dirty 288.40 ms 391.44 ms 103.04 ms
e99226d+dirty 413.20 ms 460.37 ms 47.17 ms
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms
8ab11b6+dirty 391.36 ms 417.86 ms 26.50 ms
8de2810+dirty 368.43 ms 412.20 ms 43.77 ms
946a600+dirty 397.17 ms 439.35 ms 42.17 ms
c398f67+dirty 315.08 ms 345.60 ms 30.52 ms
5bb8d5f+dirty 356.71 ms 389.65 ms 32.94 ms
e5bc97b+dirty 409.10 ms 471.61 ms 62.51 ms

App size

Revision Plain With Sentry Diff
c991c90+dirty 7.15 MiB 8.38 MiB 1.22 MiB
0677344+dirty 7.15 MiB 8.07 MiB 949.80 KiB
e99226d+dirty 7.15 MiB 8.38 MiB 1.23 MiB
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB
8ab11b6+dirty 7.15 MiB 8.37 MiB 1.22 MiB
8de2810+dirty 7.15 MiB 8.35 MiB 1.20 MiB
946a600+dirty 7.15 MiB 8.37 MiB 1.22 MiB
c398f67+dirty 7.15 MiB 8.21 MiB 1.07 MiB
5bb8d5f+dirty 7.15 MiB 8.21 MiB 1.06 MiB
e5bc97b+dirty 7.15 MiB 8.35 MiB 1.20 MiB

Previous results on branch: antonis/span-thread-info

Startup times

Revision Plain With Sentry Diff
a0ddcab+dirty 354.28 ms 359.61 ms 5.33 ms
eec51ce+dirty 374.92 ms 369.45 ms -5.47 ms
e4df67c+dirty 337.46 ms 345.14 ms 7.68 ms
4c162ec+dirty 406.00 ms 416.41 ms 10.41 ms

App size

Revision Plain With Sentry Diff
a0ddcab+dirty 7.15 MiB 8.39 MiB 1.23 MiB
eec51ce+dirty 7.15 MiB 8.39 MiB 1.23 MiB
e4df67c+dirty 7.15 MiB 8.39 MiB 1.23 MiB
4c162ec+dirty 7.15 MiB 8.39 MiB 1.23 MiB

Copy link
Contributor

github-actions bot commented Feb 21, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1210.22 ms 1218.98 ms 8.76 ms
Size 2.63 MiB 3.75 MiB 1.12 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9cab16b+dirty 1237.76 ms 1234.00 ms -3.76 ms
416f465+dirty 1221.56 ms 1226.48 ms 4.92 ms
4a6664f+dirty 1209.49 ms 1208.63 ms -0.86 ms
cdf2bdf+dirty 1230.10 ms 1238.08 ms 7.98 ms
c81e67f+dirty 1221.59 ms 1227.22 ms 5.63 ms
9c48b2c+dirty 1246.96 ms 1255.73 ms 8.77 ms
575f9da+dirty 1266.22 ms 1274.84 ms 8.62 ms
15c80ab+dirty 1223.74 ms 1228.96 ms 5.22 ms
9dabcce+dirty 1231.39 ms 1238.02 ms 6.63 ms
8ae23a7+dirty 1230.02 ms 1227.62 ms -2.40 ms

App size

Revision Plain With Sentry Diff
9cab16b+dirty 2.36 MiB 3.08 MiB 737.23 KiB
416f465+dirty 2.36 MiB 3.11 MiB 759.80 KiB
4a6664f+dirty 2.36 MiB 3.04 MiB 696.39 KiB
cdf2bdf+dirty 2.36 MiB 3.12 MiB 779.40 KiB
c81e67f+dirty 2.63 MiB 3.75 MiB 1.12 MiB
9c48b2c+dirty 2.36 MiB 2.85 MiB 495.77 KiB
575f9da+dirty 2.36 MiB 2.87 MiB 520.20 KiB
15c80ab+dirty 2.36 MiB 2.83 MiB 474.49 KiB
9dabcce+dirty 2.36 MiB 3.10 MiB 757.52 KiB
8ae23a7+dirty 2.36 MiB 3.10 MiB 752.42 KiB

Previous results on branch: antonis/span-thread-info

Startup times

Revision Plain With Sentry Diff
eec51ce+dirty 1226.57 ms 1232.71 ms 6.13 ms
a0ddcab+dirty 1224.65 ms 1223.67 ms -0.98 ms
e4df67c+dirty 1236.49 ms 1239.79 ms 3.30 ms
4c162ec+dirty 1210.77 ms 1212.33 ms 1.56 ms

App size

Revision Plain With Sentry Diff
eec51ce+dirty 2.63 MiB 3.75 MiB 1.12 MiB
a0ddcab+dirty 2.63 MiB 3.75 MiB 1.12 MiB
e4df67c+dirty 2.63 MiB 3.75 MiB 1.12 MiB
4c162ec+dirty 2.63 MiB 3.75 MiB 1.12 MiB

Copy link
Contributor

github-actions bot commented Feb 21, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1209.86 ms 1214.96 ms 5.10 ms
Size 3.19 MiB 4.32 MiB 1.13 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0d3e677+dirty 1239.02 ms 1241.22 ms 2.20 ms
de59d3a+dirty 1241.17 ms 1249.16 ms 8.00 ms
40c35c5+dirty 1223.90 ms 1217.19 ms -6.71 ms
c6f01ea+dirty 1229.52 ms 1237.16 ms 7.64 ms
3f05680+dirty 1226.09 ms 1235.67 ms 9.59 ms
7f6950b+dirty 1250.94 ms 1252.92 ms 1.98 ms
025d490+dirty 1205.27 ms 1206.15 ms 0.88 ms
9dabcce+dirty 1247.71 ms 1239.18 ms -8.53 ms
488c9c5+dirty 1225.71 ms 1222.00 ms -3.71 ms
1c65324+dirty 1239.71 ms 1239.86 ms 0.15 ms

App size

Revision Plain With Sentry Diff
0d3e677+dirty 2.92 MiB 3.66 MiB 758.42 KiB
de59d3a+dirty 2.92 MiB 3.67 MiB 772.59 KiB
40c35c5+dirty 3.19 MiB 4.27 MiB 1.08 MiB
c6f01ea+dirty 2.92 MiB 3.69 MiB 789.94 KiB
3f05680+dirty 3.19 MiB 4.31 MiB 1.12 MiB
7f6950b+dirty 2.92 MiB 3.67 MiB 772.53 KiB
025d490+dirty 3.19 MiB 4.32 MiB 1.13 MiB
9dabcce+dirty 2.92 MiB 3.67 MiB 770.02 KiB
488c9c5+dirty 3.19 MiB 4.25 MiB 1.06 MiB
1c65324+dirty 2.92 MiB 3.61 MiB 705.56 KiB

Previous results on branch: antonis/span-thread-info

Startup times

Revision Plain With Sentry Diff
eec51ce+dirty 1234.52 ms 1243.67 ms 9.15 ms
a0ddcab+dirty 1214.13 ms 1211.31 ms -2.81 ms
e4df67c+dirty 1238.98 ms 1233.90 ms -5.08 ms
4c162ec+dirty 1216.59 ms 1213.88 ms -2.72 ms

App size

Revision Plain With Sentry Diff
eec51ce+dirty 3.19 MiB 4.32 MiB 1.13 MiB
a0ddcab+dirty 3.19 MiB 4.32 MiB 1.13 MiB
e4df67c+dirty 3.19 MiB 4.32 MiB 1.13 MiB
4c162ec+dirty 3.19 MiB 4.32 MiB 1.13 MiB

Copy link
Contributor

github-actions bot commented Feb 21, 2025

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 416.12 ms 434.77 ms 18.65 ms
Size 17.75 MiB 20.12 MiB 2.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1c65324 426.37 ms 460.36 ms 33.99 ms
ae7b03d 428.82 ms 412.33 ms -16.49 ms
9433f35 347.64 ms 356.22 ms 8.58 ms
b95b8af 454.05 ms 454.53 ms 0.48 ms
63ed251 457.74 ms 441.54 ms -16.20 ms
5852d77 415.81 ms 421.02 ms 5.21 ms
5bb8d5f 431.21 ms 459.40 ms 28.19 ms
b1e8712 462.11 ms 465.71 ms 3.60 ms
5f03ae9 444.88 ms 448.89 ms 4.01 ms
e1ea4a8 506.82 ms 510.48 ms 3.66 ms

App size

Revision Plain With Sentry Diff
1c65324 17.73 MiB 19.95 MiB 2.21 MiB
ae7b03d 17.75 MiB 20.11 MiB 2.37 MiB
9433f35 17.73 MiB 19.81 MiB 2.08 MiB
b95b8af 17.73 MiB 20.11 MiB 2.37 MiB
63ed251 17.74 MiB 20.08 MiB 2.34 MiB
5852d77 17.75 MiB 20.11 MiB 2.36 MiB
5bb8d5f 17.73 MiB 19.93 MiB 2.20 MiB
b1e8712 17.73 MiB 19.75 MiB 2.02 MiB
5f03ae9 17.75 MiB 20.11 MiB 2.36 MiB
e1ea4a8 17.74 MiB 20.08 MiB 2.34 MiB

Previous results on branch: antonis/span-thread-info

Startup times

Revision Plain With Sentry Diff
4c162ec 445.55 ms 471.73 ms 26.18 ms
a0ddcab 429.46 ms 483.07 ms 53.61 ms
e4df67c 453.67 ms 478.46 ms 24.79 ms
eec51ce 310.30 ms 311.04 ms 0.74 ms

App size

Revision Plain With Sentry Diff
4c162ec 17.75 MiB 20.12 MiB 2.37 MiB
a0ddcab 17.75 MiB 20.12 MiB 2.37 MiB
e4df67c 17.75 MiB 20.12 MiB 2.37 MiB
eec51ce 17.75 MiB 20.12 MiB 2.37 MiB

@antonis antonis marked this pull request as ready for review February 24, 2025 14:54
Comment on lines 84 to 85
[SPAN_THREAD_ID]: SPAN_THREAD_ID_JAVASCRIPT,
[SPAN_THREAD_NAME]: SPAN_THREAD_NAME_JAVASCRIPT,
Copy link
Member

Choose a reason for hiding this comment

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

When considering what TTID and TTFD measure it includes both JS and main threads. But the majority of the work should be in JS so I think this is OK.

I just want to note this down.

@@ -154,3 +154,33 @@ export function addDefaultOpForSpanFrom(client: Client): void {
}
});
}

export const SPAN_THREAD_ID = 'thread.id';
export const SPAN_THREAD_ID_MAIN = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Depending on the scenario and where the value is read from sentry-cocoa sets MAIN_ID as 0 or 259.

I don't know the situation on Android, but I think we should leave the IDs for now until we establish how to read them so they map correctly to what the native SDKs are reporting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense @krystofwoldrich 👍
Removed the thread id with 9163202 and I'll investigate how to get this information from the native SDKs separately

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.

Overall the Pr looks good!

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 🚀

Thank you!

@antonis antonis merged commit d053743 into main Feb 25, 2025
70 checks passed
@antonis antonis deleted the antonis/span-thread-info branch February 25, 2025 17:14
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