sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat: Add secondary database and update router

Open markstory opened this issue 1 year ago • 3 comments

We've recently moved crons to a dedicated primary in saas. This has resulted in a tombstone issue as the joins used for tombstones don't work in the crons database. Reproducing this issue in the test suite has been challenging because in tests monitor tables and tombstone tables end up in the same connection.

By adding an additional database, connection and a new db router we're able to more closely represent how the application operates in tests.

markstory avatar Apr 25 '24 19:04 markstory

@getsentry/crons There are a few failing tests in these results from the monitor details endpoint.

https://github.com/getsentry/sentry/blob/793c62cb6cf3f99dc8fe6d13de5945a333bf09c0/src/sentry/monitors/endpoints/base_monitor_details.py#L181-L247

This passage of code currently makes a cross database transaction. While this code behaves correctly in tests, it is not correct in production as the operations on Rule and Environment cannot occur within a transaction on the crons connection.

I can get that problem addressed, but given that we can't have all of the changes happen within a single transaction anymore, how should it work?

markstory avatar Apr 25 '24 19:04 markstory

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.00%. Comparing base (abf1a07) to head (17cad13). Report is 243 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #69697      +/-   ##
==========================================
+ Coverage   79.98%   80.00%   +0.02%     
==========================================
  Files        6492     6476      -16     
  Lines      289830   289204     -626     
  Branches    49943    49868      -75     
==========================================
- Hits       231828   231391     -437     
+ Misses      57591    57421     -170     
+ Partials      411      392      -19     
Files Coverage Δ
src/sentry/db/router.py 93.57% <100.00%> (+0.50%) :arrow_up:
src/sentry/monitors/consumers/monitor_consumer.py 91.11% <100.00%> (+0.08%) :arrow_up:
.../sentry/monitors/endpoints/base_monitor_details.py 96.63% <100.00%> (+0.08%) :arrow_up:
...sentry/services/hybrid_cloud/import_export/impl.py 90.43% <100.00%> (+0.08%) :arrow_up:
src/sentry/tasks/deletion/hybrid_cloud.py 94.35% <ø> (+4.57%) :arrow_up:
src/sentry/testutils/pytest/sentry.py 86.91% <100.00%> (+0.59%) :arrow_up:

... and 766 files with indirect coverage changes

codecov[bot] avatar Apr 25 '24 19:04 codecov[bot]

@getsentry/crons There are a few failing tests in these results from the monitor details endpoint.

https://github.com/getsentry/sentry/blob/793c62cb6cf3f99dc8fe6d13de5945a333bf09c0/src/sentry/monitors/endpoints/base_monitor_details.py#L181-L247

This passage of code currently makes a cross database transaction. While this code behaves correctly in tests, it is not correct in production as the operations on Rule and Environment cannot occur within a transaction on the crons connection.

I can get that problem addressed, but given that we can't have all of the changes happen within a single transaction anymore, how should it work?

I think we should probably just open a transaction on both databases. We don't get any perfect guarantee of transactional consistency, but if any errors occur then we'll roll back both. Really the only failure case is if the last transaction fails to commit, which seems about the best we could do.

wedamija avatar Apr 25 '24 20:04 wedamija