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

Add name and geo to User #2710

Merged
merged 29 commits into from
Mar 28, 2023
Merged

Add name and geo to User #2710

merged 29 commits into from
Mar 28, 2023

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Feb 21, 2023

📜 Description

  • Add property name to User
  • Add class Geo
  • Add property geo to User

💡 Motivation and Context

Relates to getsentry/sentry-dart#1065

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

🔮 Next steps

Set values correctly when syncing user from hybrid SDK

@denrase denrase changed the title Add name and geo to User Add name and geo to User Feb 21, 2023
@github-actions
Copy link

github-actions bot commented Feb 21, 2023

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

Generated by 🚫 dangerJS against 6cc9013

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #2710 (6cc9013) into main (e5ac362) will increase coverage by 0.01%.
The diff coverage is 90.62%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2710      +/-   ##
==========================================
+ Coverage   81.37%   81.38%   +0.01%     
==========================================
  Files         258      259       +1     
  Lines       24161    24220      +59     
  Branches    10722    10750      +28     
==========================================
+ Hits        19660    19711      +51     
- Misses       4005     4008       +3     
- Partials      496      501       +5     
Impacted Files Coverage Δ
Sources/Sentry/SentryGeo.m 88.00% <88.00%> (ø)
Sources/Sentry/SentryUser.m 95.91% <100.00%> (+0.68%) ⬆️

... and 7 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 e5ac362...6cc9013. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Almost LGTM @denrase.

Sources/Sentry/Public/SentryGeo.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryUser.h Show resolved Hide resolved
Tests/SentryTests/Protocol/TestData.swift Show resolved Hide resolved
Sources/Sentry/SentryUser.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryGeo.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryGeo.m Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 27, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1241.19 ms 1271.90 ms 30.71 ms
Size 20.76 KiB 427.54 KiB 406.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
06548c0 1262.80 ms 1275.00 ms 12.20 ms
e79ead7 1216.91 ms 1247.06 ms 30.15 ms
cce35c6 1228.88 ms 1241.14 ms 12.26 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
b6ba04e 1217.45 ms 1248.92 ms 31.47 ms
85b619d 1252.51 ms 1275.33 ms 22.82 ms
7bc3c0d 1259.74 ms 1268.45 ms 8.71 ms
f576153 1210.02 ms 1228.94 ms 18.92 ms
25bcc50 1237.69 ms 1258.40 ms 20.71 ms
d80d410 1231.50 ms 1240.14 ms 8.64 ms

App size

Revision Plain With Sentry Diff
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
e79ead7 20.76 KiB 426.11 KiB 405.35 KiB
cce35c6 20.76 KiB 425.80 KiB 405.04 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
b6ba04e 20.76 KiB 414.45 KiB 393.69 KiB
85b619d 20.76 KiB 426.11 KiB 405.35 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
f576153 20.76 KiB 425.77 KiB 405.01 KiB
25bcc50 20.76 KiB 427.23 KiB 406.46 KiB
d80d410 20.76 KiB 425.71 KiB 404.95 KiB

Previous results on branch: feat/user-name-and-geo

Startup times

Revision Plain With Sentry Diff
04f7598 1221.70 ms 1231.46 ms 9.76 ms
b2dedcb 1226.18 ms 1236.56 ms 10.38 ms
56ba69d 1234.88 ms 1258.69 ms 23.82 ms
a27f9bf 1214.08 ms 1240.52 ms 26.44 ms
9570e63 1243.00 ms 1253.20 ms 10.20 ms
7614270 1216.38 ms 1231.70 ms 15.32 ms

App size

Revision Plain With Sentry Diff
04f7598 20.76 KiB 426.22 KiB 405.46 KiB
b2dedcb 20.76 KiB 427.54 KiB 406.78 KiB
56ba69d 20.76 KiB 429.04 KiB 408.28 KiB
a27f9bf 20.76 KiB 429.05 KiB 408.29 KiB
9570e63 20.76 KiB 426.91 KiB 406.15 KiB
7614270 20.76 KiB 427.54 KiB 406.78 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This comment stops me from giving this an approval.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Some minor improvements and a question about the equality comparison, but otherwise LGTM.

Sources/Sentry/SentryGeo.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryGeo.m Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryGeo.h Show resolved Hide resolved
/**
* Optional: Two-letter country code (ISO 3166-1 alpha-2).
*/
@property (nullable, atomic, copy) NSString *countryCode;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can't just grab the NSLocale object that would probably be used to populate this, so we can get any other info from that we need at a later date, as well as ensure the right value is actually set in our serialize implementation (there's a lot going on in NSLocale and some people might not get it right).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As i understood this is filled out manually by users, so i'm not sure if we should do things automatically to populate these fields. What other info that we might need are you refering to?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose even if we know sure we won't want anything other than country code, we could still just get it directly from the NSLocale. I've seen plenty of mistakes mixing up country codes, language codes and locale identifiers. I would ask for some validation logic, but that would be more work than us just using the locale object in a known safe way.

this is filled out manually by users

Do you mean end-users of apps consuming this SDK? Or users of the SDK, as in app developers? If the former, even more reason to validate input, but again, that's more work.

Copy link
Contributor

Choose a reason for hiding this comment

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

the server infers that automatically from the IP if provided, this should only be set if the user wanna override the inferred value by the server as explained here #2710 (comment)

Sources/Sentry/Public/SentryUser.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryUser.h Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

@denrase, I think this PR should be ready for review. After you remove the synchronize blocks and maybe add examples for the region, I think you can merge this.

Copy link
Member

@philipphofmann philipphofmann 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 @denrase 😺

Sources/Sentry/SentryGeo.m Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Ups forgot to LGTM

@philipphofmann philipphofmann marked this pull request as ready for review March 21, 2023 15:49
@denrase denrase merged commit f4e0299 into main Mar 28, 2023
@denrase denrase deleted the feat/user-name-and-geo branch March 28, 2023 12:18
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.

6 participants