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

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

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jul 6, 2023

Set the project id as the user id, so Sentry can tell how many projects are impacted by an SDK crash.

Set the project id as the user id, so Sentry can tell how many projects
are impacted by this SDK crash.
@philipphofmann philipphofmann requested a review from a team July 6, 2023 07:28
@philipphofmann philipphofmann self-assigned this Jul 6, 2023
@philipphofmann philipphofmann added the Scope: Backend Automatically applied to PRs that change backend components label Jul 6, 2023
@philipphofmann philipphofmann enabled auto-merge (squash) July 6, 2023 07:29
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #52331 (24063cc) into master (a54d212) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52331      +/-   ##
==========================================
- Coverage   79.31%   79.31%   -0.01%     
==========================================
  Files        4908     4908              
  Lines      205703   205704       +1     
  Branches    35164    35164              
==========================================
  Hits       163160   163160              
  Misses      37556    37556              
- Partials     4987     4988       +1     
Impacted Files Coverage Δ
...rc/sentry/utils/sdk_crashes/sdk_crash_detection.py 95.00% <100.00%> (+0.12%) ⬆️

... and 3 files with indirect coverage changes

@@ -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

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.

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

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.

@philipphofmann philipphofmann merged commit 7046a3e into master Jul 6, 2023
65 checks passed
@philipphofmann philipphofmann deleted the feat/sdk-crashes-user-id branch July 6, 2023 09:00
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants