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

Create User and Breadcrumb from map #2820

Merged
merged 46 commits into from
Apr 25, 2023
Merged

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Mar 21, 2023

📜 Description

Expose initialiser to create User and Breadcrumb from dictionary data.

💡 Motivation and Context

Relates to getsentry/team-mobile#59

Closes #2687

💚 How did you test it?

Unit tests

📝 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.

@github-actions
Copy link

github-actions bot commented Mar 21, 2023

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

Generated by 🚫 dangerJS against b21de57

denrase and others added 5 commits March 21, 2023 09:44
…try/sentry-cocoa into feat/user-and-breadcrumb-from-map

# Conflicts:
#	Sources/Sentry/PrivateSentrySDKOnly.m
…try/sentry-cocoa into feat/user-and-breadcrumb-from-map
@github-actions
Copy link

github-actions bot commented Mar 21, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1197.52 ms 1228.60 ms 31.08 ms
Size 20.76 KiB 434.92 KiB 414.16 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
89b12eb 1236.02 ms 1246.63 ms 10.61 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms
43aa39d 1239.16 ms 1270.42 ms 31.26 ms
e8b2fb4 1230.70 ms 1242.84 ms 12.14 ms
4977fbc 1231.55 ms 1239.80 ms 8.25 ms
7f691b5 1233.94 ms 1243.80 ms 9.86 ms
ea73af6 1230.96 ms 1244.98 ms 14.02 ms
25bcc50 1233.47 ms 1234.27 ms 0.80 ms
4c00f8c 1231.62 ms 1237.76 ms 6.14 ms
efb2222 1256.44 ms 1278.90 ms 22.46 ms

App size

Revision Plain With Sentry Diff
89b12eb 20.76 KiB 432.88 KiB 412.11 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
43aa39d 20.76 KiB 432.82 KiB 412.06 KiB
e8b2fb4 20.76 KiB 427.23 KiB 406.46 KiB
4977fbc 20.76 KiB 419.86 KiB 399.10 KiB
7f691b5 20.76 KiB 420.55 KiB 399.79 KiB
ea73af6 20.76 KiB 425.75 KiB 404.99 KiB
25bcc50 20.76 KiB 427.22 KiB 406.46 KiB
4c00f8c 20.76 KiB 419.62 KiB 398.86 KiB
efb2222 20.76 KiB 424.45 KiB 403.69 KiB

Previous results on branch: feat/user-and-breadcrumb-from-map

Startup times

Revision Plain With Sentry Diff
3d27ebf 1204.04 ms 1225.50 ms 21.46 ms
4b1bd8f 1232.34 ms 1251.66 ms 19.32 ms
68027d7 1209.98 ms 1232.00 ms 22.02 ms
12d47c8 1242.31 ms 1253.18 ms 10.87 ms
429242d 1239.08 ms 1250.02 ms 10.94 ms
2b84441 1228.06 ms 1241.43 ms 13.37 ms
f83b263 1237.90 ms 1256.74 ms 18.84 ms
e9514a8 1230.00 ms 1257.54 ms 27.54 ms
b7526d8 1225.58 ms 1233.20 ms 7.62 ms

App size

Revision Plain With Sentry Diff
3d27ebf 20.76 KiB 434.14 KiB 413.38 KiB
4b1bd8f 20.76 KiB 434.63 KiB 413.87 KiB
68027d7 20.76 KiB 427.39 KiB 406.63 KiB
12d47c8 20.76 KiB 433.01 KiB 412.25 KiB
429242d 20.76 KiB 433.02 KiB 412.25 KiB
2b84441 20.76 KiB 427.31 KiB 406.54 KiB
f83b263 20.76 KiB 427.31 KiB 406.55 KiB
e9514a8 20.76 KiB 428.67 KiB 407.91 KiB
b7526d8 20.76 KiB 427.39 KiB 406.63 KiB

@denrase denrase marked this pull request as ready for review March 21, 2023 09:31
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 look, a few things I believe should be changed.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/PrivateSentrySDKOnly.m Outdated Show resolved Hide resolved
@denrase denrase requested a review from brustolin March 21, 2023 13:12
@denrase
Copy link
Collaborator Author

denrase commented Mar 27, 2023

@philipphofmann Having trouble to satisfy CI as an import of SentryUser.h or SentryBreadcrumb.h is not found. I guess that it ha to be additionally imported in another header. Could you point me to the correct place?

@brustolin
Copy link
Contributor

brustolin commented Mar 29, 2023

Having trouble to satisfy CI as an import of SentryUser.h or SentryBreadcrumb.h is not found

@denrase, I fixed it for you

@brustolin
Copy link
Contributor

Still need to fix other tests not compiling.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Just a couple small things from me.

Sources/Sentry/include/HybridPublic/SentryUser+Private.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryBreadcrumb.m Outdated Show resolved Hide resolved
@denrase
Copy link
Collaborator Author

denrase commented Apr 24, 2023

@brustolin @armcknight Merged in main and fixed changelog. Think we can merge?

@denrase denrase merged commit d3abae0 into main Apr 25, 2023
@denrase denrase deleted the feat/user-and-breadcrumb-from-map branch April 25, 2023 08:22
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.

Create User and Breadcrumb data classes from a dictionary
5 participants