Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Send user.ip_address = {{auto}} when sendDefaultPii is true #1144
Send user.ip_address = {{auto}} when sendDefaultPii is true #1144
Changes from 1 commit
143305e
de0a5ef
b560520
37735b8
6df9856
f7d952c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to undo this in the
spring
integration, since{{auto}}
there will give the IP of the server.Like check for default and set null instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such case wouldn't it be better if we make it an opt in which is pre-configured in Android? Undoing it in Spring looks for me like a workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maciejwalkowiak That means desktop apps would have to opt-in too.
I think it's fair to say: It's enabled by default but you can opt-out. And we know better in some integrations to opt out already.
We need to document this behavior. Similar to how the default of another option is documented here:
getsentry/sentry-docs#2815
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we check for
{{auto}}
and set it to null ifisSendDefaultPii
is disabled would be a breaking change btw which is fine as 4.0 is a major bump, but yeah we'd need to document.Also, it could be written
{{ auto }}
with spaces, so we'd need to trim spaces off if we want to compare that properly as well, otherwise, it'd not be a match and IPs will be set by the server.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is breaking the Android user, see:
https://github.com/getsentry/sentry-java/blob/main/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java#L151-L154
now the
user
will never be null (if isSendDefaultPii enabled), we need to fix theif
condition in theDefaultAndroidEventProcessor
classThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undoing in Spring integration ✅
Updating Android code ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been merged but there's a bug here.
{{ auto }}
is valid{{auto}}
is also validpreviously
{{auto}}
was not valid and its been fixed on the backend, so people were setting{{ auto }}
manually, those people, it won't work as the comparison is not trimming off the spaces.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will be fixed in next PR.