-
Notifications
You must be signed in to change notification settings - Fork 37
[FSSDK-11458] Python - Add SDK Multi-Region Support for Data Hosting #459
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
Conversation
…o esra/FSSDK-11458_eu_hosting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few small comments
) | ||
|
||
self.assertEqual(self.project_config.region, impression_event.event_context.region) | ||
self.assertEqual('EU', impression_event.event_context.region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also add a test for create_contervsion_event?
tests/test_event_processor.py
Outdated
@@ -576,7 +576,7 @@ def __init__(self, is_updated=False): | |||
self.is_updated = is_updated | |||
|
|||
def dispatch_event(self, log_event): | |||
if log_event.http_verb == 'POST' and log_event.url == EventFactory.EVENT_ENDPOINT: | |||
if log_event.url in EventFactory.EVENT_ENDPOINTS.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the log_event.http_verb == 'POST' condition got removed. Can we keep that check?
EventFactory.EVENT_ENDPOINTS.get('US'), | ||
expected_params, | ||
EventFactory.HTTP_VERB, | ||
EventFactory.HTTP_HEADERS, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test to check that create_log_event works correctly with EU region?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds multi-region support for data hosting in the Optimizely Python SDK, allowing events to be sent to region-specific endpoints (US or EU) based on project configuration.
- Added Region enum with US and EU values to project configuration
- Updated event factories and builders to use region-specific endpoints
- Enhanced event context to include region information
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
optimizely/project_config.py | Added Region enum and region configuration parsing with US as default |
optimizely/event/user_event.py | Updated EventContext to accept and store region parameter |
optimizely/event/user_event_factory.py | Modified factory methods to pass region to EventContext |
optimizely/event/event_factory.py | Replaced single endpoint with region-specific endpoint mapping |
optimizely/event_builder.py | Updated to use region-specific URLs for event creation |
tests/test_config.py | Added tests for region configuration parsing |
tests/test_user_event_factory.py | Added tests for region handling in event creation |
tests/test_event_factory.py | Updated tests to use new endpoint structure |
tests/test_event_builder.py | Updated tests for region-specific event building |
tests/test_event_processor.py | Updated test to handle multiple endpoints |
self.account_id = account_id | ||
self.project_id = project_id | ||
self.revision = revision | ||
self.client_name = CLIENT_NAME | ||
self.client_version = version.__version__ | ||
self.anonymize_ip = anonymize_ip | ||
self.region = region or 'US' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback logic uses a string literal 'US' instead of the Region enum. Consider using self.region = region or Region.US
for consistency with the type system.
self.region = region or 'US' | |
self.region = region or Region.US |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second htis use. If yuo have Region class somewhere, then we should call country attribute on the Region object.
optimizely/event_builder.py
Outdated
region = project_config.region.value if hasattr(project_config.region, 'value') else 'US' | ||
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS['US']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback to string literal 'US' should use Region.US.value for consistency. Also, this hasattr check suggests uncertainty about the type - consider using isinstance(project_config.region, Region) for more explicit type checking.
region = project_config.region.value if hasattr(project_config.region, 'value') else 'US' | |
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS['US']) | |
region = project_config.region.value if isinstance(project_config.region, Region) else Region.US.value | |
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS[Region.US.value]) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I agree with copilot lol
optimizely/event_builder.py
Outdated
region = project_config.region.value if hasattr(project_config.region, 'value') else 'US' | ||
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS['US']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as line 272 - the fallback to string literal 'US' should use Region.US.value for consistency, and the hasattr check could be replaced with more explicit type checking.
region = project_config.region.value if hasattr(project_config.region, 'value') else 'US' | |
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS['US']) | |
region = project_config.region.value if isinstance(project_config.region, enums.Region) else enums.Region.US.value | |
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS[enums.Region.US.value]) |
Copilot uses AI. Check for mistakes.
@@ -204,11 +259,11 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self): | |||
self.project_config.get_experiment_from_key('test_experiment'), | |||
'111129', | |||
'test_user', | |||
{'do_you_know_me': 'test_value'}, | |||
{'do_you_know_me': 'test_value'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Removed trailing comma in function call. While this change works, trailing commas in multi-line function calls are often preferred for easier version control diffs.
{'do_you_know_me': 'test_value'} | |
{'do_you_know_me': 'test_value',} |
Copilot uses AI. Check for mistakes.
@@ -275,12 +330,12 @@ def side_effect(*args, **kwargs): | |||
self.project_config.get_experiment_from_key('test_experiment'), | |||
'111129', | |||
'test_user', | |||
attributes, | |||
attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Removed trailing comma in function call. While this change works, trailing commas in multi-line function calls are often preferred for easier version control diffs.
attributes | |
attributes, |
Copilot uses AI. Check for mistakes.
@@ -340,12 +395,12 @@ def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled( | |||
self.project_config.get_experiment_from_key('test_experiment'), | |||
'111129', | |||
'test_user', | |||
{'$opt_user_agent': 'Edge'}, | |||
{'$opt_user_agent': 'Edge'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Removed trailing comma in function call. While this change works, trailing commas in multi-line function calls are often preferred for easier version control diffs.
{'$opt_user_agent': 'Edge'} | |
{'$opt_user_agent': 'Edge',} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we specify a region that doesn't exist, what happens? Does 'ZZ' fall back to 'US'? I think this is what happens. I'd like to see tests verifying what happens in this case. (Do we need to log something if we are passed an invalid region?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! We can consider string-type region for simple.
optimizely/event/event_factory.py
Outdated
region = user_context.region | ||
if hasattr(region, 'value'): | ||
region_str = region.value | ||
elif region is None: | ||
region_str = 'US' # Default to US | ||
else: | ||
region_str = str(region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consider using a simple string type for region. The region will be handled in type-safe way when building into datafile. It looks like fallback to "US" for safety is good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaeopt by string type do you mean using Region.US instead of string 'US'?
That's what I have been suggesting further down the code. Just we don't contradict each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we get string-type of region from datafile, and we can pass it down all the way to EventDispatcher to determine the URL (with fallback to US). The source of this region is datafile. Not much value of this enum-based type checking. It may be risky if you use defined enum set for strong-typed SDKs when we add new countries in the server.
optimizely/project_config.py
Outdated
region_value = config.get('region') | ||
self.region: Region | ||
if region_value == Region.EU.value: | ||
self.region = Region.EU | ||
else: | ||
self.region = Region.US |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another area we can get benefits with string type. No need to change the code when we add countries.
self.account_id = account_id | ||
self.project_id = project_id | ||
self.revision = revision | ||
self.client_name = CLIENT_NAME | ||
self.client_version = version.__version__ | ||
self.anonymize_ip = anonymize_ip | ||
self.region = region or 'US' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second htis use. If yuo have Region class somewhere, then we should call country attribute on the Region object.
optimizely/event_builder.py
Outdated
region = project_config.region.value if hasattr(project_config.region, 'value') else 'US' | ||
events_url = self.EVENTS_URLS.get(str(region), self.EVENTS_URLS['US']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I agree with copilot lol
optimizely/event/event_factory.py
Outdated
region = user_context.region | ||
if hasattr(region, 'value'): | ||
region_str = region.value | ||
elif region is None: | ||
region_str = 'US' # Default to US | ||
else: | ||
region_str = str(region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaeopt by string type do you mean using Region.US instead of string 'US'?
That's what I have been suggesting further down the code. Just we don't contradict each other.
@pvcraven Locally, I have created a test case as below for invalid region. It returned US endpoint.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Test plan
Issues