Skip to content

Add name and geo to User#2556

Merged
denrase merged 14 commits into
mainfrom
feat/user-name-and-geo
Mar 28, 2023
Merged

Add name and geo to User#2556
denrase merged 14 commits into
mainfrom
feat/user-name-and-geo

Conversation

@denrase

@denrase denrase commented Feb 21, 2023

Copy link
Copy Markdown
Collaborator

📜 Description

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

💡 Motivation and Context

Relates to getsentry/sentry-dart#1065

💚 How did you test it?

Added unit tests

📝 Checklist

  • 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

Implement on cocoa and use in hybrid SDKs

@github-actions

github-actions Bot commented Feb 21, 2023

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

Generated by 🚫 dangerJS against a40841e

@denrase
denrase marked this pull request as ready for review February 21, 2023 10:29
@denrase
denrase requested review from philipphofmann and removed request for adinauer, markushi, romtsn and stefanosiano February 21, 2023 10:29
@github-actions

github-actions Bot commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 322.90 ms 359.18 ms 36.28 ms
Size 1.73 MiB 2.34 MiB 626.62 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d81684e 235.73 ms 328.76 ms 93.03 ms

App size

Revision Plain With Sentry Diff
d81684e 1.73 MiB 2.26 MiB 547.78 KiB

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

Startup times

Revision Plain With Sentry Diff
96e2af6 412.22 ms 432.49 ms 20.26 ms
87bd976 341.90 ms 446.66 ms 104.76 ms
061e3dd 329.84 ms 393.36 ms 63.52 ms
2686d20 362.39 ms 393.20 ms 30.81 ms
8b36629 344.80 ms 381.90 ms 37.10 ms

App size

Revision Plain With Sentry Diff
96e2af6 1.73 MiB 2.34 MiB 626.19 KiB
87bd976 1.73 MiB 2.34 MiB 626.56 KiB
061e3dd 1.73 MiB 2.34 MiB 626.62 KiB
2686d20 1.73 MiB 2.34 MiB 626.73 KiB
8b36629 1.73 MiB 2.34 MiB 626.76 KiB

@philipphofmann

Copy link
Copy Markdown
Member

@denrase, did you tag me for review for a specific reason? If not, Roman or Markus could also have a look at this PR.

Comment thread sentry/src/main/java/io/sentry/protocol/Geo.java

@markushi markushi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking great, thanks for contributing - please have a look at my comments 🚀

Comment thread sentry/src/main/java/io/sentry/protocol/Geo.java Outdated
Comment thread sentry/src/test/resources/json/user.json Outdated
Comment thread sentry/src/test/java/io/sentry/protocol/UserSerializationTest.kt Outdated
@denrase
denrase requested a review from markushi February 27, 2023 09:59

@markushi markushi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice one, LGTM!

@codecov

codecov Bot commented Mar 7, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 83.33% and no project coverage change.

Comparison is base (70c2097) 81.32% compared to head (a40841e) 81.33%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2556   +/-   ##
=========================================
  Coverage     81.32%   81.33%           
- Complexity     4192     4208   +16     
=========================================
  Files           336      337    +1     
  Lines         15489    15561   +72     
  Branches       2021     2031   +10     
=========================================
+ Hits          12597    12657   +60     
- Misses         2099     2110   +11     
- Partials        793      794    +1     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/protocol/Geo.java 81.48% <81.48%> (ø)
sentry/src/main/java/io/sentry/protocol/User.java 89.52% <88.23%> (-0.25%) ⬇️
sentry/src/main/java/io/sentry/JsonSerializer.java 98.03% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@romtsn romtsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@denrase wanna go ahead and merge it?

Comment thread sentry-android-ndk/sentry-native
@denrase

denrase commented Mar 28, 2023

Copy link
Copy Markdown
Collaborator Author

@romtsn A UI running on CI test seems to be flaky. AFAIK this should not be connected to this PR. Can we merge or is there something to do first?

@romtsn

romtsn commented Mar 28, 2023

Copy link
Copy Markdown
Member

@romtsn A UI running on CI test seems to be flaky. AFAIK this should not be connected to this PR. Can we merge or is there something to do first?

merge it!

@denrase
denrase merged commit d390819 into main Mar 28, 2023
@denrase
denrase deleted the feat/user-name-and-geo branch March 28, 2023 12:17
@denrase

denrase commented Mar 28, 2023

Copy link
Copy Markdown
Collaborator Author

Thx @markushi & @romtsn for the reviews!

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.

4 participants