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

chore(hybrid): Move extra context to standalone class #2855

Merged
merged 13 commits into from
Apr 5, 2023

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Apr 3, 2023

📜 Description

This enables us to reuse the same extra context logic in hybrid SDKs.

💡 Motivation and Context

💚 How did you test it?

cocoa sample app and rn sample app

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

#skip-changelog

@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1211.38 ms 1240.22 ms 28.85 ms
Size 20.76 KiB 431.93 KiB 411.17 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7fb7afb 1202.18 ms 1219.42 ms 17.24 ms
9e96fd6 1207.20 ms 1229.40 ms 22.20 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
d60f70a 1219.63 ms 1228.54 ms 8.91 ms
e8b2fb4 1230.70 ms 1242.84 ms 12.14 ms
28333b6 1186.29 ms 1225.18 ms 38.89 ms
25bcc50 1237.69 ms 1258.40 ms 20.71 ms
3389927 1230.12 ms 1238.04 ms 7.92 ms
ddc9b9a 1210.00 ms 1234.18 ms 24.18 ms
67460f4 1244.56 ms 1255.96 ms 11.40 ms

App size

Revision Plain With Sentry Diff
7fb7afb 20.76 KiB 419.70 KiB 398.94 KiB
9e96fd6 20.76 KiB 425.80 KiB 405.04 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
d60f70a 20.76 KiB 430.97 KiB 410.21 KiB
e8b2fb4 20.76 KiB 427.23 KiB 406.46 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB
25bcc50 20.76 KiB 427.23 KiB 406.46 KiB
3389927 20.76 KiB 427.23 KiB 406.46 KiB
ddc9b9a 20.76 KiB 420.40 KiB 399.64 KiB
67460f4 20.76 KiB 426.15 KiB 405.39 KiB

Previous results on branch: kw-refactor-extra-context

Startup times

Revision Plain With Sentry Diff
83ddda5 1227.18 ms 1238.62 ms 11.44 ms
2a3e27e 1216.35 ms 1238.28 ms 21.93 ms
b7a2220 1252.66 ms 1256.64 ms 3.98 ms
a981aa9 1241.92 ms 1254.47 ms 12.55 ms
b48675b 1190.02 ms 1217.49 ms 27.47 ms

App size

Revision Plain With Sentry Diff
83ddda5 20.76 KiB 431.93 KiB 411.17 KiB
2a3e27e 20.76 KiB 431.90 KiB 411.14 KiB
b7a2220 20.76 KiB 431.99 KiB 411.23 KiB
a981aa9 20.76 KiB 431.96 KiB 411.20 KiB
b48675b 20.76 KiB 431.93 KiB 411.17 KiB

Co-authored-by: Dhiogo Brustolin <[email protected]>
@krystofwoldrich krystofwoldrich marked this pull request as ready for review April 3, 2023 16:18
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

First full look.
Some minor suggestions to make code cleaner.

And I think, instead of providing extraContextProvider to SentryClient, you should make SentryContextProvider init to ask for crashWrapper, deviceWrapper and ProcessInfoWrapper, then you initialize a SentryContextProvider inside SentryClient.
This way you fix CI

@krystofwoldrich
Copy link
Member Author

krystofwoldrich commented Apr 4, 2023

@brustolin Thank you for the suggestions and help with the tests. I've added tests for the provider and fixed the client tests. I hope the CI will be happy this time.

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #2855 (c579859) into main (d1a3794) will increase coverage by 1.54%.
The diff coverage is 95.23%.

❗ Current head c579859 differs from pull request most recent head 8f0e299. Consider uploading reports for the commit 8f0e299 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2855      +/-   ##
==========================================
+ Coverage   79.90%   81.45%   +1.54%     
==========================================
  Files         259      261       +2     
  Lines       24362    24425      +63     
  Branches    10475    10824     +349     
==========================================
+ Hits        19467    19895     +428     
+ Misses       4386     4027     -359     
+ Partials      509      503       -6     
Impacted Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.m 87.32% <0.00%> (-3.86%) ⬇️
Sources/Sentry/SentryClient.m 97.92% <100.00%> (-0.08%) ⬇️
Sources/Sentry/SentryExtraContextProvider.m 100.00% <100.00%> (ø)

... and 77 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a3794...8f0e299. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

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