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(grouping): Store metadata for new grouphashes #76245

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Aug 15, 2024

As part of the rollout of the new grouphash metadata table, this creates a GroupHashMetadata row for every new GroupHash we create. It's gated by a killswitch and a feature flag, and doesn't do much yet - the only data in the table so far is a creation timestamp - but this should give us a sense of whether or not there's going to be any hit to ingest DB performance.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 15, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #76245   +/-   ##
=======================================
  Coverage   78.20%   78.20%           
=======================================
  Files        6895     6895           
  Lines      306372   306384   +12     
  Branches    50227    50229    +2     
=======================================
+ Hits       239608   239621   +13     
+ Misses      60358    60355    -3     
- Partials     6406     6408    +2     

@lobsterkatie lobsterkatie force-pushed the kmclb-fix-grouphashmetadata-grouphash-property branch 2 times, most recently from c292ef5 to 448c15d Compare August 15, 2024 22:51
Base automatically changed from kmclb-fix-grouphashmetadata-grouphash-property to master August 16, 2024 16:39
@lobsterkatie lobsterkatie force-pushed the kmclb-store-metadata-for-new-grouphashes branch from 806dcbe to 6f7b8f8 Compare August 16, 2024 18:56
@lobsterkatie lobsterkatie force-pushed the kmclb-store-metadata-for-new-grouphashes branch from 6f7b8f8 to 0b44df4 Compare August 26, 2024 21:05
@lobsterkatie lobsterkatie force-pushed the kmclb-store-metadata-for-new-grouphashes branch from 0b44df4 to a546e1e Compare August 26, 2024 21:48
@lobsterkatie lobsterkatie changed the base branch from master to kmclb-fix-grouphash-deletion-via-project-deletion August 26, 2024 21:49
@lobsterkatie lobsterkatie marked this pull request as ready for review August 27, 2024 04:15
@lobsterkatie lobsterkatie requested a review from a team as a code owner August 27, 2024 04:15
Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

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

talked during meeting, should ramp up writes slowly just to be safe

@lobsterkatie lobsterkatie force-pushed the kmclb-fix-grouphash-deletion-via-project-deletion branch from 810370e to a3bc7a6 Compare August 28, 2024 23:47
@lobsterkatie lobsterkatie force-pushed the kmclb-store-metadata-for-new-grouphashes branch from e12caf6 to 47426f0 Compare August 28, 2024 23:47
@lobsterkatie
Copy link
Member Author

lobsterkatie commented Aug 28, 2024

talked during meeting, should ramp up writes slowly just to be safe

Yup - you beat me by about two minutes. 🙂 Just pushed it again, including a feature flag.

UPDATE: Here's the PR with the actual config: https://github.com/getsentry/sentry-options-automator/pull/2189.

lobsterkatie added a commit that referenced this pull request Sep 3, 2024
…76572)

This is a follow-up to #76312, fixing another bug with grouphash deletion. Currently, when a project is deleted, its associated `GroupHash` records are deleted directly by the project deletion task, using the `BulkModelDeletionTask`. Unfortunately, that task doesn't take into account any dependent tables, meaning that the corresponding `GroupHashMetadata` records aren't deleted as they should be.

This fixes that by allowing `GroupHash` deletions to cascade from group deletions (which themselves are set off by project deletion), rather than having the project deletion task delete the grouphashes directly. (Group deletions already handle `GroupHash`/`GroupHashMetadata` deletion correctly, using the `GroupHash`-specific deletion task registered in the deletion module's `__init__.py`[1].)

Note that while the results of this change aren't tested in this PR, they are implicitly tested in #76245, which will directly follow this one and which for the first time will actually create `GroupHashMetadata` records. Without the change in this PR, the presence of that PR's new `GroupHashMetadata` records causes the project deletion test to fail, but with this change, they pass.


[1] https://github.com/getsentry/sentry/blob/9155480cddb7454bba99ae39ea4caac91261ca26/src/sentry/deletions/__init__.py#L127
Base automatically changed from kmclb-fix-grouphash-deletion-via-project-deletion to master September 3, 2024 18:45
@lobsterkatie lobsterkatie force-pushed the kmclb-store-metadata-for-new-grouphashes branch from 47426f0 to e85e583 Compare September 3, 2024 18:52
@lobsterkatie lobsterkatie merged commit f78e1bc into master Sep 3, 2024
53 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-store-metadata-for-new-grouphashes branch September 3, 2024 19:31
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
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.

3 participants