From 5a031ed06ee79d0a713655f82ec54e0c5c9d3ab4 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Aug 2022 13:38:30 +0200 Subject: [PATCH 1/3] [WIP] feat(metrics): PoC of running metrics indexer migrations against spanner --- requirements-base.txt | 2 +- requirements-dev-frozen.txt | 2 +- requirements-frozen.txt | 2 +- src/sentry/conf/server.py | 11 +++- src/sentry/db/models/fields/bounded.py | 2 +- src/sentry/db/router.py | 60 +++++++++++++++++++ .../0284_metrics_indexer_alter_seq.py | 14 +---- .../migrations/0291_add_new_perf_indexer.py | 11 ---- src/sentry/models/counter.py | 3 - 9 files changed, 75 insertions(+), 32 deletions(-) create mode 100644 src/sentry/db/router.py diff --git a/requirements-base.txt b/requirements-base.txt index 75342a78adb2c5..e55bc935ffa3d2 100644 --- a/requirements-base.txt +++ b/requirements-base.txt @@ -102,4 +102,4 @@ phabricator>=0.7.0 # test dependencies, but unable to move to requirements-test until # sentry.utils.pytest and sentry.testutils are moved to tests/ selenium>=4.1.5 -sqlparse>=0.2.4 +sqlparse>=0.3.0 diff --git a/requirements-dev-frozen.txt b/requirements-dev-frozen.txt index 810548d26240df..7d8200c56368ea 100644 --- a/requirements-dev-frozen.txt +++ b/requirements-dev-frozen.txt @@ -151,7 +151,7 @@ sniffio==1.2.0 snuba-sdk==1.0.0 sortedcontainers==2.4.0 soupsieve==2.3.2.post1 -sqlparse==0.2.4 +sqlparse==0.4.2 statsd==3.3 structlog==21.1.0 symbolic==8.7.1 diff --git a/requirements-frozen.txt b/requirements-frozen.txt index 2df4c991e6860c..2c94ed5680a890 100644 --- a/requirements-frozen.txt +++ b/requirements-frozen.txt @@ -104,7 +104,7 @@ sniffio==1.2.0 snuba-sdk==1.0.0 sortedcontainers==2.4.0 soupsieve==2.3.2.post1 -sqlparse==0.2.4 +sqlparse==0.4.2 statsd==3.3 structlog==21.1.0 symbolic==8.7.1 diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 407df51254b4e3..42a824bcb07713 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -145,9 +145,17 @@ def env(key, default="", type=None): "PORT": "", "AUTOCOMMIT": True, "ATOMIC_REQUESTS": False, - } + }, + "metrics-spanner": { + "ENGINE": "django_spanner", + "PROJECT": "search-and-storage", + "INSTANCE": "markus-test-spanner", + "NAME": "markus-test-spanner-db", + }, } +DATABASE_ROUTERS = ("sentry.db.router.MultiDatabaseRouter",) + if "DATABASE_URL" in os.environ: url = urlparse(os.environ["DATABASE_URL"]) @@ -321,6 +329,7 @@ def env(key, default="", type=None): ] INSTALLED_APPS = ( + "django_spanner", "django.contrib.admin", "django.contrib.auth", "django.contrib.contenttypes", diff --git a/src/sentry/db/models/fields/bounded.py b/src/sentry/db/models/fields/bounded.py index f5638b7aba15d2..9e16843da7a77d 100644 --- a/src/sentry/db/models/fields/bounded.py +++ b/src/sentry/db/models/fields/bounded.py @@ -66,7 +66,7 @@ class BoundedBigAutoField(models.AutoField): # type: ignore MAX_VALUE = 9223372036854775807 def db_type(self, connection: BaseDatabaseWrapper) -> str: - return "bigserial" + return "int64" def get_related_db_type(self, connection: BaseDatabaseWrapper) -> str: return cast(str, BoundedBigIntegerField().db_type(connection)) diff --git a/src/sentry/db/router.py b/src/sentry/db/router.py new file mode 100644 index 00000000000000..6d0ddcc78d0c67 --- /dev/null +++ b/src/sentry/db/router.py @@ -0,0 +1,60 @@ +_db_table_to_db = { + "sentry_stringindexer": "metrics-spanner", # will replace ^ table above +} + + +def db_for_model(model): + return db_for_table(model._meta.db_table) + + +def db_for_table(table): + return _db_table_to_db.get(table, "default") + + +class MultiDatabaseRouter: + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._setup_replicas() + + def _setup_replicas(self): + from django.conf import settings + + self._replicas = {} + for db, db_conf in settings.DATABASES.items(): + primary = db_conf.get("REPLICA_OF") + if primary: + self._replicas[primary] = db + + def db_for_read(self, model, **hints): + db = db_for_model(model) + if hints.get("replica", False): + db = self._replicas.get(db, db) + return db + + def db_for_write(self, model, **hints): + return db_for_model(model) + + def allow_relation(self, obj1, obj2, **hints): + return db_for_model(obj1) == db_for_model(obj2) + + def allow_syncdb(self, db, model): + if db_for_model(model) == db: + return True + return False + + def allow_migrate(self, db, app_label, model=None, **hints): + if model: + return db_for_model(model) == db + + # We use this hint in our RunSql/RunPython migrations to help resolve databases. + if "tables" in hints: + dbs = {db_for_table(table) for table in hints["tables"]} + if len(dbs) > 1: + raise RuntimeError( + "Migration tables resolve to multiple databases. " + f"Got {dbs} when only one database should be used." + ) + return dbs.pop() == db + # Assume migrations with no model routing or hints need to run on + # the default database. + return db == "default" diff --git a/src/sentry/migrations/0284_metrics_indexer_alter_seq.py b/src/sentry/migrations/0284_metrics_indexer_alter_seq.py index 275a7b811aa5e1..0751c3d901a17d 100644 --- a/src/sentry/migrations/0284_metrics_indexer_alter_seq.py +++ b/src/sentry/migrations/0284_metrics_indexer_alter_seq.py @@ -27,16 +27,4 @@ class Migration(CheckedMigration): ("sentry", "0283_extend_externalissue_key"), ] - operations = [ - migrations.RunSQL( - """ - ALTER SEQUENCE sentry_stringindexer_id_seq START WITH 65536; - ALTER SEQUENCE sentry_stringindexer_id_seq RESTART; - """, - hints={"tables": ["sentry_stringindexer"]}, - reverse_sql=""" - ALTER SEQUENCE sentry_stringindexer_id_seq START WITH 1; - ALTER SEQUENCE sentry_stringindexer_id_seq RESTART; - """, - ) - ] + operations = [] diff --git a/src/sentry/migrations/0291_add_new_perf_indexer.py b/src/sentry/migrations/0291_add_new_perf_indexer.py index 0973b58fc6ef9b..5ad9f5690d3e98 100644 --- a/src/sentry/migrations/0291_add_new_perf_indexer.py +++ b/src/sentry/migrations/0291_add_new_perf_indexer.py @@ -58,15 +58,4 @@ class Migration(CheckedMigration): fields=("string", "organization_id"), name="perf_unique_org_string" ), ), - migrations.RunSQL( - """ - ALTER SEQUENCE sentry_perfstringindexer_id_seq START WITH 65536; - ALTER SEQUENCE sentry_perfstringindexer_id_seq RESTART; - """, - hints={"tables": ["sentry_perfstringindexer"]}, - reverse_sql=""" - ALTER SEQUENCE sentry_perfstringindexer_id_seq START WITH 1; - ALTER SEQUENCE sentry_perfstringindexer_id_seq RESTART; - """, - ), ] diff --git a/src/sentry/models/counter.py b/src/sentry/models/counter.py index dd5d403cd5f8ff..7eea6c66738e63 100644 --- a/src/sentry/models/counter.py +++ b/src/sentry/models/counter.py @@ -122,6 +122,3 @@ def create_counter_function(app_config, using, **kwargs): ) finally: cursor.close() - - -post_migrate.connect(create_counter_function, dispatch_uid="create_counter_function", weak=False) From 44e5d9b063dde91e63754a86ef619e2aebd336fe Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Tue, 9 Aug 2022 13:47:58 +0200 Subject: [PATCH 2/3] avoid using db_type in BigAutoField --- src/sentry/conf/server.py | 4 ++-- src/sentry/db/models/fields/bounded.py | 11 ++--------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 42a824bcb07713..6232adbe24be86 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -149,8 +149,8 @@ def env(key, default="", type=None): "metrics-spanner": { "ENGINE": "django_spanner", "PROJECT": "search-and-storage", - "INSTANCE": "markus-test-spanner", - "NAME": "markus-test-spanner-db", + "INSTANCE": "markus-test-spanner-pg", + "NAME": "markus-test-spanner-db-std", }, } diff --git a/src/sentry/db/models/fields/bounded.py b/src/sentry/db/models/fields/bounded.py index 9e16843da7a77d..d6f48b25f51006 100644 --- a/src/sentry/db/models/fields/bounded.py +++ b/src/sentry/db/models/fields/bounded.py @@ -2,7 +2,6 @@ from django.conf import settings from django.db import models -from django.db.backends.base.base import BaseDatabaseWrapper from django.utils.translation import ugettext_lazy as _ __all__ = ( @@ -60,19 +59,13 @@ def get_prep_value(self, value: int) -> int: assert value <= self.MAX_VALUE return cast(int, super().get_prep_value(value)) - class BoundedBigAutoField(models.AutoField): # type: ignore + class BoundedBigAutoField(models.BigAutoField): # type: ignore description = _("Big Integer") MAX_VALUE = 9223372036854775807 - def db_type(self, connection: BaseDatabaseWrapper) -> str: - return "int64" - - def get_related_db_type(self, connection: BaseDatabaseWrapper) -> str: - return cast(str, BoundedBigIntegerField().db_type(connection)) - def get_internal_type(self) -> str: - return "BigIntegerField" + return "BigAutoField" def get_prep_value(self, value: int) -> int: if value: From aeb22db6b874b0cd3fb1a413621475f20e32961d Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Tue, 9 Aug 2022 21:11:31 +0200 Subject: [PATCH 3/3] also add perfstringindexer, rename db --- src/sentry/conf/server.py | 2 +- src/sentry/db/router.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 6232adbe24be86..79efdb10a54de1 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -150,7 +150,7 @@ def env(key, default="", type=None): "ENGINE": "django_spanner", "PROJECT": "search-and-storage", "INSTANCE": "markus-test-spanner-pg", - "NAME": "markus-test-spanner-db-std", + "NAME": "metrics-spanner", }, } diff --git a/src/sentry/db/router.py b/src/sentry/db/router.py index 6d0ddcc78d0c67..5b91586b889296 100644 --- a/src/sentry/db/router.py +++ b/src/sentry/db/router.py @@ -1,5 +1,6 @@ _db_table_to_db = { "sentry_stringindexer": "metrics-spanner", # will replace ^ table above + "sentry_perfstringindexer": "metrics-spanner", # will replace ^ table above }