From 58048aa49325f99d24cba939b2a7cbe65174e52c Mon Sep 17 00:00:00 2001 From: Radu Woinaroski <5281987+RaduW@users.noreply.github.com> Date: Fri, 24 Feb 2023 12:05:36 +0100 Subject: [PATCH 1/2] Add cross organisation methods for the string indexer --- src/sentry/sentry_metrics/indexer/base.py | 16 ++++++++++++++++ src/sentry/sentry_metrics/indexer/cache.py | 10 ++++++++++ .../indexer/cloudspanner/cloudspanner.py | 10 ++++++++++ src/sentry/sentry_metrics/indexer/mock.py | 10 ++++++++++ .../indexer/postgres/postgres_v2.py | 10 ++++++++++ src/sentry/sentry_metrics/indexer/strings.py | 10 ++++++++++ 6 files changed, 66 insertions(+) diff --git a/src/sentry/sentry_metrics/indexer/base.py b/src/sentry/sentry_metrics/indexer/base.py index de9180eba69c5e..24d980949bb3b9 100644 --- a/src/sentry/sentry_metrics/indexer/base.py +++ b/src/sentry/sentry_metrics/indexer/base.py @@ -265,3 +265,19 @@ def reverse_resolve(self, use_case_id: UseCaseKey, org_id: int, id: int) -> Opti Returns None if the entry cannot be found. """ raise NotImplementedError() + + def resolve_shared_org(self, string: str) -> Optional[int]: + """ + Look up the index for a shared (cross organisation) string. + + Typically, this function will only lookup strings that are statically defined but + regardless of the mechanism these are strings that are not organisation or use-case specific. + """ + raise NotImplementedError() + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + """Lookup the stored string given integer for a shared (cross organisation) ID. + + Returns None if the entry cannot be found. + """ + raise NotImplementedError() diff --git a/src/sentry/sentry_metrics/indexer/cache.py b/src/sentry/sentry_metrics/indexer/cache.py index 8a3c458d6d9a47..30e183535e19ba 100644 --- a/src/sentry/sentry_metrics/indexer/cache.py +++ b/src/sentry/sentry_metrics/indexer/cache.py @@ -167,3 +167,13 @@ def resolve(self, use_case_id: UseCaseKey, org_id: int, string: str) -> Optional def reverse_resolve(self, use_case_id: UseCaseKey, org_id: int, id: int) -> Optional[str]: return self.indexer.reverse_resolve(use_case_id, org_id, id) + + def resolve_shared_org(self, string: str) -> Optional[int]: + raise NotImplementedError( + "This class should not be used directly, use a wrapping class that derives from StaticStringIndexer" + ) + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + raise NotImplementedError( + "This class should not be used directly, use a wrapping class that derives from StaticStringIndexer" + ) diff --git a/src/sentry/sentry_metrics/indexer/cloudspanner/cloudspanner.py b/src/sentry/sentry_metrics/indexer/cloudspanner/cloudspanner.py index 2f298311faa533..c08751483b8085 100644 --- a/src/sentry/sentry_metrics/indexer/cloudspanner/cloudspanner.py +++ b/src/sentry/sentry_metrics/indexer/cloudspanner/cloudspanner.py @@ -431,6 +431,16 @@ def reverse_resolve(self, use_case_id: UseCaseKey, org_id: int, id: int) -> Opti else: return str(results_list[0][0]) + def resolve_shared_org(self, string: str) -> Optional[int]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class CloudSpannerIndexer" + ) + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class CloudSpannerIndexer" + ) + class CloudSpannerIndexer(StaticStringIndexer): def __init__(self, **kwargs: Any) -> None: diff --git a/src/sentry/sentry_metrics/indexer/mock.py b/src/sentry/sentry_metrics/indexer/mock.py index 8f637dd6e86b0e..c25f9aef252c6a 100644 --- a/src/sentry/sentry_metrics/indexer/mock.py +++ b/src/sentry/sentry_metrics/indexer/mock.py @@ -67,6 +67,16 @@ def _record(self, org_id: int, string: str) -> Optional[int]: self._reverse[index] = string return index + def resolve_shared_org(self, string: str) -> Optional[int]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class SimpleIndexer" + ) + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class SimpleIndexer" + ) + class SimpleIndexer(StaticStringIndexer): def __init__(self) -> None: diff --git a/src/sentry/sentry_metrics/indexer/postgres/postgres_v2.py b/src/sentry/sentry_metrics/indexer/postgres/postgres_v2.py index df5a848699b590..b2fce06670d760 100644 --- a/src/sentry/sentry_metrics/indexer/postgres/postgres_v2.py +++ b/src/sentry/sentry_metrics/indexer/postgres/postgres_v2.py @@ -198,6 +198,16 @@ def reverse_resolve(self, use_case_id: UseCaseKey, org_id: int, id: int) -> Opti def _table(self, use_case_id: UseCaseKey) -> IndexerTable: return TABLE_MAPPING[use_case_id] + def resolve_shared_org(self, string: str) -> Optional[int]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class PostgresIndexer" + ) + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + raise NotImplementedError( + "This class should not be used directly, use the wrapping class PostgresIndexer" + ) + class PostgresIndexer(StaticStringIndexer): def __init__(self) -> None: diff --git a/src/sentry/sentry_metrics/indexer/strings.py b/src/sentry/sentry_metrics/indexer/strings.py index ef20ccd3d1317c..48f55c329b0d5d 100644 --- a/src/sentry/sentry_metrics/indexer/strings.py +++ b/src/sentry/sentry_metrics/indexer/strings.py @@ -175,3 +175,13 @@ def reverse_resolve(self, use_case_id: UseCaseKey, org_id: int, id: int) -> Opti if id in REVERSE_SHARED_STRINGS: return REVERSE_SHARED_STRINGS[id] return self.indexer.reverse_resolve(use_case_id=use_case_id, org_id=org_id, id=id) + + def resolve_shared_org(self, string: str) -> Optional[int]: + if string in SHARED_STRINGS: + return SHARED_STRINGS[string] + return None + + def reverse_shared_org_resolve(self, id: int) -> Optional[str]: + if id in REVERSE_SHARED_STRINGS: + return REVERSE_SHARED_STRINGS[id] + return None From f1e0f8e7cc6b743152f0c7afff2e920992725ec8 Mon Sep 17 00:00:00 2001 From: Radu Woinaroski <5281987+RaduW@users.noreply.github.com> Date: Fri, 24 Feb 2023 14:33:25 +0100 Subject: [PATCH 2/2] Add tests --- tests/sentry/sentry_metrics/test_strings.py | 42 +++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/sentry/sentry_metrics/test_strings.py b/tests/sentry/sentry_metrics/test_strings.py index c3baa01065b6c9..0648579783a284 100644 --- a/tests/sentry/sentry_metrics/test_strings.py +++ b/tests/sentry/sentry_metrics/test_strings.py @@ -14,3 +14,45 @@ def test_static_strings_only() -> None: assert results[3]["production"] == SHARED_STRINGS["production"] assert results[3]["environment"] == SHARED_STRINGS["environment"] assert results[3]["release"] == SHARED_STRINGS["release"] + + +def test_resolve_shared_org_existing_entry() -> None: + """ + Tests it is able to resolve shared strings + """ + indexer = StaticStringIndexer(MockIndexer()) + + actual = indexer.resolve_shared_org("release") + expected = SHARED_STRINGS["release"] + + assert actual == expected + + +def test_reverse_resolve_shared_org_existing_entry() -> None: + """ + Tests it is able to return correct strings for known + shared string ids + """ + indexer = StaticStringIndexer(MockIndexer()) + + release_idx = indexer.resolve_shared_org("release") + actual = indexer.reverse_shared_org_resolve(release_idx) + + assert actual == "release" + + +def test_resolve_shared_org_no_entry() -> None: + """ + Tests that it returns None for unknown strings + """ + indexer = StaticStringIndexer(MockIndexer()) + actual = indexer.resolve_shared_org("SOME_MADE_UP_STRING") + assert actual is None + + +def test_reverse_resolve_shared_org_no_entry() -> None: + indexer = StaticStringIndexer(MockIndexer()) + + # shared string start quite high 2^63 so anything smaller should return None + actual = indexer.reverse_shared_org_resolve(5) + assert actual is None