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: context defualts on native platforms other than iOS/Android #2439

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Nov 21, 2024

📜 Description

I've noticed windows doesn't have device context filled in. It's because originally we expected native integrations to sync from native iOS/Android to dart via LoadContexts(). This is not available with sentry-native which doesn't have such APIs.

To fix this, I've changed the logic to always set contexts in IoEnricherEventProcessor and made it so that it runs after the native _LoadContextsIntegrationEventProcessor. This way, the former will fill in the blanks all the time.

💡 Motivation and Context

💚 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
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.84%. Comparing base (7f97e6c) to head (068659d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2439      +/-   ##
==========================================
- Coverage   86.92%   86.84%   -0.08%     
==========================================
  Files         259      259              
  Lines        9245     9246       +1     
==========================================
- Hits         8036     8030       -6     
- Misses       1209     1216       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Nov 21, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 456.45 ms 493.58 ms 37.13 ms
Size 6.49 MiB 7.56 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1ac008b 370.04 ms 435.58 ms 65.54 ms
4ad2751 374.58 ms 462.00 ms 87.42 ms
43760f9 321.78 ms 366.77 ms 44.99 ms
fec92cc 399.96 ms 479.26 ms 79.30 ms
c9d3212 301.34 ms 361.58 ms 60.24 ms
5603ab2 309.84 ms 345.20 ms 35.36 ms
3e9fb0e 329.14 ms 359.16 ms 30.02 ms
6d317ea 303.46 ms 356.06 ms 52.60 ms
1a93825 347.31 ms 424.54 ms 77.23 ms
bffc2c5 348.00 ms 399.89 ms 51.89 ms

App size

Revision Plain With Sentry Diff
1ac008b 6.33 MiB 7.27 MiB 954.13 KiB
4ad2751 6.16 MiB 7.14 MiB 1010.86 KiB
43760f9 6.15 MiB 7.13 MiB 1000.49 KiB
fec92cc 6.35 MiB 7.33 MiB 1005.56 KiB
c9d3212 6.16 MiB 7.14 MiB 1010.90 KiB
5603ab2 5.94 MiB 6.95 MiB 1.01 MiB
3e9fb0e 5.94 MiB 6.95 MiB 1.01 MiB
6d317ea 5.94 MiB 6.92 MiB 1001.74 KiB
1a93825 6.27 MiB 7.20 MiB 956.36 KiB
bffc2c5 6.34 MiB 7.28 MiB 966.31 KiB

Previous results on branch: fix/native-contexts-propagation

Startup times

Revision Plain With Sentry Diff
0344dd8 475.67 ms 511.56 ms 35.89 ms
c3fa61f 457.23 ms 520.26 ms 63.03 ms
ca374dd 471.09 ms 508.68 ms 37.59 ms
0eae47f 436.68 ms 472.85 ms 36.17 ms

App size

Revision Plain With Sentry Diff
0344dd8 6.49 MiB 7.56 MiB 1.07 MiB
c3fa61f 6.49 MiB 7.56 MiB 1.07 MiB
ca374dd 6.49 MiB 7.56 MiB 1.07 MiB
0eae47f 6.49 MiB 7.56 MiB 1.07 MiB

Copy link
Contributor

github-actions bot commented Nov 21, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.82 ms 1246.08 ms 9.27 ms
Size 8.38 MiB 9.78 MiB 1.40 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6f3717a 1259.84 ms 1280.90 ms 21.06 ms
e6b16cd 1226.20 ms 1246.52 ms 20.32 ms
8fa3934 1252.79 ms 1272.41 ms 19.62 ms
cc80714 1205.53 ms 1223.90 ms 18.37 ms
0a82a1e 1233.56 ms 1244.56 ms 11.00 ms
3334ac1 1259.22 ms 1275.40 ms 16.17 ms
256df44 1252.49 ms 1274.27 ms 21.78 ms
cf91c9d 1217.08 ms 1233.00 ms 15.92 ms
b728df4 1287.43 ms 1293.94 ms 6.51 ms
61e71ec 1243.14 ms 1257.21 ms 14.07 ms

App size

Revision Plain With Sentry Diff
6f3717a 8.33 MiB 9.62 MiB 1.29 MiB
e6b16cd 8.33 MiB 9.54 MiB 1.22 MiB
8fa3934 8.09 MiB 9.07 MiB 1000.86 KiB
cc80714 8.33 MiB 9.40 MiB 1.07 MiB
0a82a1e 8.29 MiB 9.37 MiB 1.08 MiB
3334ac1 8.10 MiB 9.17 MiB 1.08 MiB
256df44 8.38 MiB 9.71 MiB 1.33 MiB
cf91c9d 8.33 MiB 9.40 MiB 1.07 MiB
b728df4 8.15 MiB 9.15 MiB 1020.72 KiB
61e71ec 8.33 MiB 9.40 MiB 1.07 MiB

Previous results on branch: fix/native-contexts-propagation

Startup times

Revision Plain With Sentry Diff
0344dd8 1252.76 ms 1267.66 ms 14.90 ms
c3fa61f 1242.08 ms 1267.04 ms 24.96 ms
0eae47f 1246.22 ms 1267.33 ms 21.11 ms
ca374dd 1231.43 ms 1256.87 ms 25.45 ms

App size

Revision Plain With Sentry Diff
0344dd8 8.38 MiB 9.77 MiB 1.40 MiB
c3fa61f 8.38 MiB 9.77 MiB 1.40 MiB
0eae47f 8.38 MiB 9.77 MiB 1.40 MiB
ca374dd 8.38 MiB 9.78 MiB 1.40 MiB

@vaind vaind marked this pull request as ready for review November 22, 2024 09:01
Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

👍

@vaind vaind merged commit ab0eee1 into main Nov 25, 2024
136 checks passed
@vaind vaind deleted the fix/native-contexts-propagation branch November 25, 2024 20:17
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