Skip to content

Fix(V7): missing ip settings from SDK package #5137

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 3, 2025
Merged

Fix(V7): missing ip settings from SDK package #5137

merged 5 commits into from
Sep 3, 2025
+57 −18

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Sep 3, 2025 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

The change wasn't done automatically on JS V10 so we needed to copy what was implemented on JS V10 in order to fix the IP resolution.

Original Bump: #5093
Based on:
https://github.com/getsentry/sentry-lynx/blob/5d45c33eaefda526e649a1414f1f3ef2ad206262/packages/lynx-react/src/client.ts#L79-L84

Sample event with the new fix:
https://us.sentry.io/api/0/projects/sentry-sdks/sentry-react-native/events/de4d0214f8574ecda8313891527fd41c/json/

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

@lucas-zimerman lucas-zimerman changed the title Fix deprecated ip Fix(V7): missing ip settings from SDK package Sep 3, 2025
@lucas-zimerman lucas-zimerman requested a review from Lms24 September 3, 2025 09:31
@lucas-zimerman lucas-zimerman marked this pull request as ready for review September 3, 2025 09:33
@lucas-zimerman
Copy link
Collaborator Author

@Lms24 Do we need to also patch sessions? our client uses this.on('beforeSendSession', addAutoIpAddressToSession); and that is not deprecated.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 369.57 ms 363.35 ms -6.22 ms
Size 17.75 MiB 19.68 MiB 1.93 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c08359e 421.87 ms 445.37 ms 23.50 ms
20daa0a 359.51 ms 374.90 ms 15.39 ms
21c9e75 450.39 ms 452.92 ms 2.53 ms
d916aa3 425.37 ms 427.02 ms 1.65 ms
20d5eaa 377.62 ms 406.50 ms 28.88 ms
7be1f99 454.83 ms 461.36 ms 6.53 ms
64cd15c 439.02 ms 427.63 ms -11.39 ms
eb07ba3 470.04 ms 473.35 ms 3.31 ms
534ba8c 484.00 ms 499.93 ms 15.93 ms
c1573b3 400.85 ms 411.82 ms 10.97 ms

App size

Revision Plain With Sentry Diff
c08359e 17.75 MiB 20.15 MiB 2.41 MiB
20daa0a 17.75 MiB 20.15 MiB 2.41 MiB
21c9e75 17.75 MiB 20.15 MiB 2.41 MiB
d916aa3 17.75 MiB 20.15 MiB 2.41 MiB
20d5eaa 17.75 MiB 20.15 MiB 2.41 MiB
7be1f99 17.75 MiB 20.15 MiB 2.41 MiB
64cd15c 17.75 MiB 20.15 MiB 2.41 MiB
eb07ba3 17.75 MiB 20.15 MiB 2.41 MiB
534ba8c 17.75 MiB 20.15 MiB 2.41 MiB
c1573b3 17.75 MiB 20.15 MiB 2.41 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.20 ms 1225.64 ms 3.43 ms
Size 3.19 MiB 4.44 MiB 1.25 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
20d5eaa+dirty 1224.67 ms 1223.16 ms -1.51 ms
21c9e75+dirty 1206.20 ms 1223.54 ms 17.35 ms
20daa0a+dirty 1227.71 ms 1233.72 ms 6.01 ms
a0b15d6+dirty 1213.79 ms 1210.45 ms -3.34 ms
46bd012+dirty 1231.78 ms 1212.30 ms -19.47 ms
eb07ba3+dirty 1214.49 ms 1221.59 ms 7.10 ms
bc9680d+dirty 1228.57 ms 1233.64 ms 5.07 ms
3e0a5f9+dirty 1233.65 ms 1239.10 ms 5.45 ms
d916aa3+dirty 1211.02 ms 1221.33 ms 10.31 ms
7be1f99+dirty 1222.43 ms 1217.15 ms -5.28 ms

App size

Revision Plain With Sentry Diff
20d5eaa+dirty 3.19 MiB 4.38 MiB 1.19 MiB
21c9e75+dirty 3.19 MiB 4.38 MiB 1.19 MiB
20daa0a+dirty 3.19 MiB 4.38 MiB 1.19 MiB
a0b15d6+dirty 3.19 MiB 4.38 MiB 1.19 MiB
46bd012+dirty 3.19 MiB 4.38 MiB 1.19 MiB
eb07ba3+dirty 3.19 MiB 4.38 MiB 1.19 MiB
bc9680d+dirty 3.19 MiB 4.38 MiB 1.19 MiB
3e0a5f9+dirty 3.19 MiB 4.38 MiB 1.19 MiB
d916aa3+dirty 3.19 MiB 4.38 MiB 1.19 MiB
7be1f99+dirty 3.19 MiB 4.38 MiB 1.19 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.57 ms 1216.32 ms -5.25 ms
Size 2.63 MiB 3.87 MiB 1.24 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
20d5eaa+dirty 1231.12 ms 1226.00 ms -5.12 ms
21c9e75+dirty 1237.78 ms 1247.66 ms 9.88 ms
20daa0a+dirty 1233.12 ms 1233.35 ms 0.22 ms
a0b15d6+dirty 1220.18 ms 1223.18 ms 3.00 ms
46bd012+dirty 1220.49 ms 1226.89 ms 6.40 ms
eb07ba3+dirty 1222.46 ms 1220.37 ms -2.08 ms
bc9680d+dirty 1221.41 ms 1241.47 ms 20.06 ms
3e0a5f9+dirty 1226.94 ms 1230.02 ms 3.08 ms
d916aa3+dirty 1221.02 ms 1228.98 ms 7.96 ms
7be1f99+dirty 1226.69 ms 1217.76 ms -8.93 ms

App size

Revision Plain With Sentry Diff
20d5eaa+dirty 2.63 MiB 3.81 MiB 1.18 MiB
21c9e75+dirty 2.63 MiB 3.81 MiB 1.18 MiB
20daa0a+dirty 2.63 MiB 3.81 MiB 1.18 MiB
a0b15d6+dirty 2.63 MiB 3.81 MiB 1.18 MiB
46bd012+dirty 2.63 MiB 3.81 MiB 1.18 MiB
eb07ba3+dirty 2.63 MiB 3.81 MiB 1.18 MiB
bc9680d+dirty 2.63 MiB 3.81 MiB 1.18 MiB
3e0a5f9+dirty 2.63 MiB 3.81 MiB 1.18 MiB
d916aa3+dirty 2.63 MiB 3.81 MiB 1.18 MiB
7be1f99+dirty 2.63 MiB 3.81 MiB 1.18 MiB

antonis
antonis reviewed Sep 3, 2025
View reviewed changes
antonis
antonis approved these changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@antonis antonis 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 for fixing this so quickly @lucas-zimerman 🙇

Should we wait for a response before merging?

lucas-zimerman
lucas-zimerman commented Sep 3, 2025
View reviewed changes
lucas-zimerman
lucas-zimerman commented Sep 3, 2025
View reviewed changes
@lucas-zimerman
Copy link
Collaborator Author

LGTM 🎸 Thank you for fixing this so quickly @lucas-zimerman 🙇

Should we wait for a response before merging?

ATM we follow the implementation of sentry browser https://github.com/getsentry/sentry-javascript/blob/84243e62101f37164d1917acfccda80dc6d5b923/packages/browser/src/client.ts#L127-L130
So I'd say it's safe to merge. If it also requires any changes, we can do it as a follow-up fix. (My gut feelings says it's ok to leave that feature as is since the user IP is not exposed to the clients and is only used for counting the amount of users).

Lms24
Lms24 approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this so quickly!

We can leave the session stuff as-is, since session envelopes never had the same problem on the Relay side as event envelopes. So they're not affected and IP inferral should already work as expected.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 3, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 347.88 ms 396.12 ms 48.25 ms
Size 7.15 MiB 8.41 MiB 1.26 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c08359e+dirty 406.04 ms 428.87 ms 22.83 ms
64cd15c+dirty 488.79 ms 483.54 ms -5.24 ms
a02e30b+dirty 346.13 ms 381.76 ms 35.62 ms
bc9680d+dirty 346.77 ms 463.48 ms 116.71 ms
46bd012+dirty 333.76 ms 359.24 ms 25.48 ms
3e0a5f9+dirty 379.92 ms 450.96 ms 71.04 ms
98f632c+dirty 323.98 ms 375.39 ms 51.41 ms
e2fa43d+dirty 326.56 ms 372.88 ms 46.32 ms
a0b15d6+dirty 414.33 ms 448.85 ms 34.52 ms
785ffb1+dirty 380.65 ms 451.83 ms 71.18 ms

App size

Revision Plain With Sentry Diff
c08359e+dirty 7.15 MiB 8.42 MiB 1.27 MiB
64cd15c+dirty 7.15 MiB 8.42 MiB 1.27 MiB
a02e30b+dirty 7.15 MiB 8.42 MiB 1.27 MiB
bc9680d+dirty 7.15 MiB 8.42 MiB 1.27 MiB
46bd012+dirty 7.15 MiB 8.42 MiB 1.27 MiB
3e0a5f9+dirty 7.15 MiB 8.42 MiB 1.27 MiB
98f632c+dirty 7.15 MiB 8.42 MiB 1.27 MiB
e2fa43d+dirty 7.15 MiB 8.42 MiB 1.27 MiB
a0b15d6+dirty 7.15 MiB 8.42 MiB 1.27 MiB
785ffb1+dirty 7.15 MiB 8.42 MiB 1.27 MiB

@lucas-zimerman lucas-zimerman merged commit a8caa5e into main Sep 3, 2025
73 checks passed
@lucas-zimerman lucas-zimerman deleted the lz/fix-ip branch September 3, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonis antonis antonis approved these changes

@Lms24 Lms24 Lms24 approved these changes

@alwx alwx Awaiting requested review from alwx alwx is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucas-zimerman @antonis @Lms24