Skip to content

Update sentry_app queries to use the read replica #93081

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

Closed
wants to merge 16 commits into from

Conversation

kneeyo1
Copy link
Contributor

@kneeyo1 kneeyo1 commented Jun 6, 2025

This updates some of the queries to point to the control db replica.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 6, 2025
Copy link

codecov bot commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/sentry_apps/services/app/impl.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #93081      +/-   ##
==========================================
+ Coverage   84.35%   87.97%   +3.61%     
==========================================
  Files       10319    10326       +7     
  Lines      594698   593516    -1182     
  Branches    23129    22860     -269     
==========================================
+ Hits       501687   522173   +20486     
+ Misses      92518    70899   -21619     
+ Partials      493      444      -49     

@kneeyo1 kneeyo1 marked this pull request as ready for review June 10, 2025 22:52
@kneeyo1 kneeyo1 requested review from a team as code owners June 10, 2025 22:52
@Christinarlong
Copy link
Contributor

hihi I have no context but why are we doing this change? Is this a pattern we are adopting for more models?

@kneeyo1
Copy link
Contributor Author

kneeyo1 commented Jun 11, 2025

hihi I have no context but why are we doing this change? Is this a pattern we are adopting for more models?

This is to help lessen load on the primary control db.

@Christinarlong
Copy link
Contributor

hihi I have no context but why are we doing this change? Is this a pattern we are adopting for more models?

This is to help lessen load on the primary control db.

I'm confused why are we doing this at business logic level?
It's also confusing to me if we should be following this pattern for which control models, or how was that decided?

kneeyo1 and others added 2 commits June 12, 2025 11:04
Co-authored-by: Mark Story <mark@mark-story.com>
@kneeyo1
Copy link
Contributor Author

kneeyo1 commented Jun 12, 2025

hihi I have no context but why are we doing this change? Is this a pattern we are adopting for more models?

This is to help lessen load on the primary control db.

I'm confused why are we doing this at business logic level? It's also confusing to me if we should be following this pattern for which control models, or how was that decided?

We want to follow this model when possible. We do this at a business logic level (routing) because we want to edit it per query, not for all read queries. Some application logic will follow a read-your-writes pattern, and since all writes must go to the primary, if we point all reads to the replica, it can result in a miss due to replication lag.

Thus we only want to point read queries to be reading from the replica when there isn't this constraint.

If we wanted to blanket apply all reads to be from the read query though, it would probably look something like this: https://github.com/getsentry/getsentry/pull/17625

@Christinarlong
Copy link
Contributor

okay, that makes sense. Please make sure to add us for any similar changes in the integrations/notifications domain since any time sensitive models could have issues with reading from replica.

@kneeyo1 kneeyo1 enabled auto-merge (squash) June 13, 2025 17:51
@kneeyo1 kneeyo1 requested a review from markstory June 13, 2025 20:53
@kneeyo1 kneeyo1 disabled auto-merge June 16, 2025 16:02
@kneeyo1 kneeyo1 closed this Jun 16, 2025
kneeyo1 added a commit that referenced this pull request Jun 17, 2025
This updates some of the queries to point to the control db replica. 
see #93081, breaking that change apart so we can better see the effects
of these on the db.
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.

3 participants