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.

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

feat(sdk-crashes): Set project id as user id #52331

Merged
merged 1 commit into from
Jul 6, 2023
Merged

feat(sdk-crashes): Set project id as user id #52331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/sentry/utils/sdk_crashes/sdk_crash_detection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ def detect_sdk_crash(
},
)

# So Sentry can tell how many projects are impacted by this SDK crash
set_path(sdk_crash_event_data, "user", "id", value=event.project.id)
Copy link
Member

Choose a reason for hiding this comment

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

If we're only interested in the number of users (not tracking those users down), should we hash the project id here, to be on the safe side of any privacy concerns?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already store the project id here

sentry/src/sentry/utils/sdk_crashes/sdk_crash_detection.py

Lines 62 to 65 in a54d212

value={
"original_project_id": event.project.id,
"original_event_id": event.event_id,
},

There is nothing you can do with the project id. Only with admin rights in Sentry you can find and access the project. Why do you think there are any privacy concerns?

Copy link
Member

Choose a reason for hiding this comment

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

OK, missed that. I may have been thinking too far ahead -- if this ever becomes a more general "sentry for library developers" feature and we make these errors public, we have to make sure that this user ID cannot be traced back to the original organization. As long as this is our own private feature, this should be fine though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent that you think ahead, but this is not a problem right now.


return self.sdk_crash_reporter.report(sdk_crash_event_data, event_project_id)

return None
Expand Down
3 changes: 3 additions & 0 deletions tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ def execute_test(self, event_data, should_be_reported, mock_sdk_crash_reporter):
"original_project_id": event.project_id,
"original_event_id": event.event_id,
}
assert reported_event_data["user"] == {
"id": event.project_id,
}
else:
assert mock_sdk_crash_reporter.report.call_count == 0

Expand Down