Skip to content

fix(aci): Change open period range boundary to inclusive #93535

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

Merged
merged 9 commits into from
Jun 18, 2025

Conversation

snigdhas
Copy link
Member

Some issues have the same regression/resolution time in our db and the range boundary today would reject those as valid start/end times for open periods. Updating the boundary to be inclusive on both ends should fix this and allow an open period to start at the same time that the previous one ends.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 13, 2025
@snigdhas snigdhas marked this pull request as ready for review June 13, 2025 18:06
@snigdhas snigdhas requested a review from a team as a code owner June 13, 2025 18:06
@snigdhas snigdhas requested review from wedamija and a team June 13, 2025 18:06
Copy link

codecov bot commented Jun 13, 2025

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   #93535    +/-   ##
========================================
  Coverage   88.03%   88.04%            
========================================
  Files       10325    10330     +5     
  Lines      595677   596288   +611     
  Branches    23134    23134            
========================================
+ Hits       524411   524989   +578     
- Misses      70773    70806    +33     
  Partials      493      493            

@snigdhas
Copy link
Member Author

Hm ADD CONSTRAINT EXCLUDE is an unsafe operation -- we can wipe the table and pause writes before applying this if that would make it safe? Otherwise the safer suggestion is to make a new table 🤔

@snigdhas
Copy link
Member Author

Talked to @wedamija - we'll turn off the ff for writes and run the post-deploy migration to delete rows before running this migration with SafeRunSQL.

Copy link
Contributor

github-actions bot commented Jun 13, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0927_make_open_period_range_boundary_inclusive.py

for 0927_make_open_period_range_boundary_inclusive in sentry

--
-- Custom state/database change combination
--
                    ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "exclude_overlapping_date_start_end" EXCLUDE USING GIST ("group" WITH =, (TSTZRANGE("date_started", "date_ended", '[]')) WITH &&);
                    
--
-- Remove constraint exclude_overlapping_start_end from model groupopenperiod
--
ALTER TABLE "sentry_groupopenperiod" DROP CONSTRAINT "exclude_overlapping_start_end";

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Btw, if you want to delete the table quickly, you can use a TRUNCATE on the table so that you don't have to wait.

Copy link
Contributor

github-actions bot commented Jun 16, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0930_make_open_period_range_boundary_inclusive.py

for 0930_make_open_period_range_boundary_inclusive in sentry

--
-- Custom state/database change combination
--
                    ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "exclude_overlapping_date_start_end" EXCLUDE USING GIST ("group_id" WITH =, (TSTZRANGE("date_started", "date_ended", '[]')) WITH &&);
                    
--
-- Remove constraint exclude_overlapping_start_end from model groupopenperiod
--
ALTER TABLE "sentry_groupopenperiod" DROP CONSTRAINT "exclude_overlapping_start_end";

expressions=[
(models.F("group"), RangeOperators.EQUAL),
("group", "="),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we change this to be the string? Can't you just keep it as modes.F(...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm that was something I had changed when the migration check kept failing, but it shouldn't be necessary

@snigdhas snigdhas merged commit 5b3ad90 into master Jun 18, 2025
64 checks passed
@snigdhas snigdhas deleted the snigdha/fix-boundary branch June 18, 2025 16:06
andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
Some issues have the same regression/resolution time in our db and the
range boundary today would reject those as valid start/end times for
open periods. Updating the boundary to be inclusive on both ends should
fix this and allow an open period to start at the same time that the
previous one ends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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