feat(hybrid): Introduce RegionMapping method contracts #39253

RyanSkonnord · 2022-09-23T18:50:42Z

Set up a type system for region objects and the set of all regions, with placeholder methods for backend operations not implemented yet. These methods should serve as temporary interface contracts to unblock development on application code that would otherwise depend on those operations.

Specifically, the contracts cover:

injecting the list of all regions from config (or whatever operational source of truth we settle on);
looking up what region I'm in if I'm a region silo; and
resolving an organization to its home region.

For the first practical use, I've dropped in a simple call to the second one, replacing some older placeholder code around "snowflake" ID generation.

Set up a type system for region objects and the set of all regions, with placeholder methods for backend operations not implemented yet.

RyanSkonnord · 2022-09-23T18:55:27Z

src/sentry/types/region.py

+    "eu.sentry.io"). In a development environment, it may be useful to define
+    region addresses as a custom hostname or port number (e.g,
+    "localhost:8001").
+    """


The contract for the address attribute is a first pass and we'll probably need to iterate on it. In production, we'll definitely need to resolve regions into the subdomain where they're being hosted, and I wanted to generalize that to arbitrary hostnames and ports. But I have no idea whether it will make sense for the strings to look like "eu.sentry.io", "https://eu.sentry.io/", or whatever.

Note the assumption that the production config will be supplying a value of the form "eu.sentry.io" rather than just "eu". If it's the latter, we'll additionally need to specify the "sentry.io" part somewhere -- I'd prefer to keep that domain-name-building logic outside of the application code as much as possible.

I think you're right that this might need some more thought. We will need to address non-API resources across regions as well, e.g. control silo reaching out to region silo kafka clusters. These might just be better as separate explicit configurations though

subdomain or redirect target for default HTTPS might be useful in general however. This address may be like, "public endpoint"-eseque?

RyanSkonnord · 2022-09-23T18:56:07Z

src/sentry/types/region.py

+        REGION_ID.validate(self.id)
+
+    def to_url(self, path: str) -> str:
+        return self.address + path


As above, this is just conceptual. We'll handle "do we need to join them with a slash" and other annoying details when the time comes.

On the other hand, this definitely seems more related specifically to endpoint API and not necessary the Region as a whole. I imagine there being a "SiloHTTPApiClient" that handles that and can be constructed from a Region, but not assuming there is a singular such that associated with a Region.

RyanSkonnord · 2022-09-23T18:58:30Z

src/sentry/types/region.py

+_global_region_mapping = None
+
+
+def get_region_mapping() -> RegionMapping:


This feels ugly. I'd prefer to do something like our common "registry" pattern where the region mapping is just initialized with the module, but that leads to circular imports because sentry/conf.py needs to import the Region and RegionMapping types.

a slightly less ugly version of this is to use functools.lru_cache(maxsize=1) to hide the global part

I agree, actually getting the region with get_region_mapping().get_local_region() is a bit wordy. Maybe if this is a class and we shorten to RegionMap?
e.g.

class RegionMap: def __new__(self): return ( RegionMapping.load_from_config() if not _global_region_mapping else _global_region_mapping )

but it's essentially the same thing: RegionMap().get_local_region()

What about an api like,

get_local_region() -> str get_all_regions() -> List[str] get_region(key: str) -> Region

? I don't think we need the Mapping object directly.

IE: Essentially flatten the mapping object into module level functions and don't expose it directly.

Having a public API made of functions feels better than a global API of shared global data. I recognize that the functions are just wrapping the global state. But the functions could be cloning data so that the global state can't be mutated.

Module-level interfaces make my Java brain sad but I see how it's nicer to abstract away the mapping object. 😅

Part of the reason I didn't like that originally, was that override_settings wouldn't play nice with having the regions cached. (I don't believe the caching is premature optimization, because the mapping could otherwise produce O(n²) costs once we have a growing number of single-tenant regions.) In addition to injecting the region values, you would have to mess around with mocks in the sentry.types.region module, which usually would be two steps removed from whatever you're trying to test, as is the case in test_snowflake.

My solution is a helper named override_region in testutils, which hides away the module-level mocking for reuse in other tests. I think the current test_snowflake implementation works out pretty nicely, where it only has to inject SENTRY_REGION but doesn't have to directly mock any functions.

Is it also possible to "clear" this cached value via a pytest after hook that can be installed globally?

Not without using a more sophisticated cache than functools.lru_cache, as far as I know.

mikejihbe

Directionally this is looking good 👍 , thanks for putting this together

mikejihbe · 2022-09-23T20:04:29Z

src/sentry/types/region.py

+    "eu.sentry.io"). In a development environment, it may be useful to define
+    region addresses as a custom hostname or port number (e.g,
+    "localhost:8001").
+    """


I think you're right that this might need some more thought. We will need to address non-API resources across regions as well, e.g. control silo reaching out to region silo kafka clusters. These might just be better as separate explicit configurations though

leeandher

👏 Looks good! I appreciate how documented and clear the API is. I cloned and spun this up locally too with no issues!

leeandher · 2022-09-23T20:25:12Z

src/sentry/types/region.py

+_global_region_mapping = None
+
+
+def get_region_mapping() -> RegionMapping:


I agree, actually getting the region with get_region_mapping().get_local_region() is a bit wordy. Maybe if this is a class and we shorten to RegionMap?
e.g.

class RegionMap: def __new__(self): return ( RegionMapping.load_from_config() if not _global_region_mapping else _global_region_mapping )

but it's essentially the same thing: RegionMap().get_local_region()

leeandher · 2022-09-23T20:27:23Z

src/sentry/utils/snowflake.py

+    segment_values[VERSION_ID] = msb_0_ordering(settings.SNOWFLAKE_VERSION_ID, VERSION_ID.length)
+
+    local_region = get_region_mapping().get_local_region()
+    segment_values[REGION_ID] = local_region.id if local_region else 0


Should we lock this 0 in a constant to denote a 'default region'? Just thinking that when snowflake IDs are rolled out into prod, we'd want 0 to map the the main US region, if we haven't set up the region config in SaaS yet.

I guess we can think of it as 'default region' or 'unregioned' though, I think we should avoid SnowflakeIDs if we haven't set the region config just yet.

As discussed elsewhere, I prefer not to represent a "default region" when what we really mean is "there are no regions in my entire system (which may be a self-hosted monolith)".

I think we should avoid SnowflakeIDs if we haven't set the region config just yet.

The plan is to have snowflake IDs be used for all organizations and projects, for the sake of parity, regardless of whether the system is multi-region. So it would be normal for future self-hosted systems to be generating snowflake IDs with zeroes where the region IDs would be if they had regions. (CC @maxiuyuan)

In contrast to not wanting a region object as a "default value", the snowflake IDs force us to represent both the "region" and "no-region" cases with numbers. Therefore, I agree with reserving 0 for "unregioned" and documenting it in code (and error-checking it in Region.__init__) . We'd then count actual regions from 1.

Went ahead and did this in b52156c. Let me know if anyone feels strongly about the North American region being 0.

corps · 2022-09-23T20:59:48Z

mypy.ini

@@ -107,6 +107,7 @@ files = src/sentry/analytics/,
        src/sentry/tasks/symbolication.py,


Ah, interesting, so to be clear do all type-checked files need to be included here?

corps · 2022-09-23T21:07:37Z

src/sentry/types/region.py

+        # For now, assume that all region configs can be taken in through Django
+        # settings. We may investigate other ways of delivering those configs in
+        # production.
+        return cls(settings.SENTRY_REGION_CONFIG)


src/sentry/types/region.py

corps · 2022-09-23T21:16:32Z

tests/sentry/utils/test_snowflake.py

@@ -57,3 +63,27 @@ def test_out_of_region_sequences(self):
            generate_snowflake_id("test_redis_key")

        assert str(context.value) == "No available ID"
+
+    @freeze_time(CURRENT_TIME)
+    @mock.patch("sentry.utils.snowflake.get_region_mapping")


How about using with override_settings as the test target? Helps test deeper the load from settings mechanism as well. Minor nit though.

As I mentioned here, override_settings isn't enough by itself, but I think the current implementation is as good.

corps · 2022-09-23T21:17:15Z

Thank you for getting this started, honestly I can imagine using this framework for features in the near future.

src/sentry/types/region.py

markstory · 2022-09-23T22:04:22Z

src/sentry/types/region.py

+_global_region_mapping = None
+
+
+def get_region_mapping() -> RegionMapping:


Having a public API made of functions feels better than a global API of shared global data. I recognize that the functions are just wrapping the global state. But the functions could be cloning data so that the global state can't be mutated.

src/sentry/types/region.py

tests/sentry/types/test_region.py

This reverts commit a7d5d84. Co-authored-by: anthony.sottile via Slack <[email protected]>

Set up a type system for region objects and the set of all regions, with placeholders for backend operations not implemented yet. Remaking changes from #39253, which was reverted due to an error now fixed by getsentry/getsentry#8407

Set up a type system for region objects and the set of all regions, with placeholders for backend operations not implemented yet.

This reverts commit a7d5d84. Co-authored-by: anthony.sottile via Slack <[email protected]>

Set up a type system for region objects and the set of all regions, with placeholders for backend operations not implemented yet. Remaking changes from #39253, which was reverted due to an error now fixed by getsentry/getsentry#8407

RyanSkonnord added 2 commits September 23, 2022 11:44

feat(hybrid): Introduce RegionMapping method contracts

0f8044f

Set up a type system for region objects and the set of all regions, with placeholder methods for backend operations not implemented yet.

Use region ID in snowflake ID generation

c8f43c5

RyanSkonnord requested a review from a team as a code owner September 23, 2022 18:50

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 23, 2022

RyanSkonnord commented Sep 23, 2022

View reviewed changes

RyanSkonnord requested review from leeandher, maxiuyuan, AniketDas-Tekky, markstory and corps September 23, 2022 19:00

mikejihbe reviewed Sep 23, 2022

View reviewed changes

leeandher approved these changes Sep 23, 2022

View reviewed changes

corps reviewed Sep 23, 2022

View reviewed changes

src/sentry/types/region.py Outdated Show resolved Hide resolved

corps reviewed Sep 23, 2022

View reviewed changes

markstory reviewed Sep 23, 2022

View reviewed changes

RyanSkonnord added 2 commits September 26, 2022 10:14

Merge remote-tracking branch 'origin/master' into region-mapping

e173e1a

Raise exceptions over returning None

03d8911

vercel bot deployed to Preview – storybook September 26, 2022 18:27 View deployment

vercel bot deployed to Preview – sentry September 26, 2022 18:27 View deployment

RyanSkonnord added 2 commits September 26, 2022 13:34

Rename GEOGRAPHIC to MULTI_TENANT

33f6366

Replace RegionMapping with module-level API

ceafdf5

vercel bot deployed to Preview – sentry September 26, 2022 20:58 View deployment

vercel bot deployed to Preview – storybook September 26, 2022 20:59 View deployment

AniketDas-Tekky approved these changes Sep 26, 2022

View reviewed changes

src/sentry/types/region.py Outdated Show resolved Hide resolved

src/sentry/types/region.py Outdated Show resolved Hide resolved

tests/sentry/types/test_region.py Show resolved Hide resolved

Designate NULL_REGION_ID; add check when constructing Region

b52156c

vercel bot deployed to Preview – storybook September 26, 2022 21:47 View deployment

vercel bot deployed to Preview – sentry September 26, 2022 21:47 View deployment

corps approved these changes Sep 26, 2022

View reviewed changes

RyanSkonnord added 2 commits September 27, 2022 10:53

Merge branch 'master' into region-mapping

4b38350

Clarify docstrings around region address

330b1cb

RyanSkonnord enabled auto-merge (squash) September 27, 2022 18:02

vercel bot deployed to Preview – sentry September 27, 2022 18:03 View deployment

vercel bot deployed to Preview – storybook September 27, 2022 18:04 View deployment

RyanSkonnord merged commit a7d5d84 into master Sep 27, 2022

RyanSkonnord deleted the region-mapping branch September 27, 2022 18:18

getsentry-bot added a commit that referenced this pull request Sep 27, 2022

Revert "feat(hybrid): Introduce region function contracts (#39253)"

2ee35b9

This reverts commit a7d5d84. Co-authored-by: anthony.sottile via Slack <[email protected]>

RyanSkonnord mentioned this pull request Sep 27, 2022

feat(hybrid): Introduce region function contracts #39356

Merged

ryan953 pushed a commit that referenced this pull request Oct 3, 2022

feat(hybrid): Introduce region function contracts (#39253)

4166b08

Set up a type system for region objects and the set of all regions, with placeholders for backend operations not implemented yet.

ryan953 pushed a commit that referenced this pull request Oct 3, 2022

Revert "feat(hybrid): Introduce region function contracts (#39253)"

aac6a82

This reverts commit a7d5d84. Co-authored-by: anthony.sottile via Slack <[email protected]>

github-actions bot locked and limited conversation to collaborators Oct 13, 2022

		_global_region_mapping = None


		def get_region_mapping() -> RegionMapping:

		@@ -107,6 +107,7 @@ files = src/sentry/analytics/,
		src/sentry/tasks/symbolication.py,

Uh oh!

feat(hybrid): Introduce RegionMapping method contracts #39253

feat(hybrid): Introduce RegionMapping method contracts #39253

Uh oh!

Conversation

RyanSkonnord commented Sep 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikejihbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeandher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corps commented Sep 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leeandher left a comment •

edited

Loading