-
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
Changes from 31 commits
d9e8f83
0866962
5a0e19d
730fede
47ccb94
a7d74ac
8a65e0f
24d2fcd
ba5beee
17c9280
155ab25
ba049b6
a777fa0
c1f1693
6de0889
83afabe
158aab3
0a0cdef
f68d678
81cd042
9d2c892
3c1332f
11523cf
1af68db
7a09f1d
0b1e83f
e07c1b4
1d61988
e24a34e
80365fa
174a7d0
f83865f
8e92fda
dafb205
c774dde
c645e73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ | |||||
from sys import version_info | ||||||
|
||||||
from optimizely import version | ||||||
from optimizely.project_config import Region | ||||||
|
||||||
|
||||||
if version_info < (3, 8): | ||||||
|
@@ -97,10 +98,11 @@ def __init__( | |||||
class EventContext: | ||||||
""" Class respresenting User Event Context. """ | ||||||
|
||||||
def __init__(self, account_id: str, project_id: str, revision: str, anonymize_ip: bool): | ||||||
def __init__(self, account_id: str, project_id: str, revision: str, anonymize_ip: bool, region: Region): | ||||||
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 commentThe 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
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -54,7 +54,10 @@ class EventBuilder: | |||||||||
""" Class which encapsulates methods to build events for tracking | ||||||||||
impressions and conversions using the new V3 event API (batch). """ | ||||||||||
|
||||||||||
EVENTS_URL: Final = 'https://logx.optimizely.com/v1/events' | ||||||||||
EVENTS_URLS: Final = { | ||||||||||
'US': 'https://logx.optimizely.com/v1/events', | ||||||||||
'EU': 'https://eu.logx.optimizely.com/v1/events' | ||||||||||
} | ||||||||||
HTTP_VERB: Final = 'POST' | ||||||||||
HTTP_HEADERS: Final = {'Content-Type': 'application/json'} | ||||||||||
|
||||||||||
|
@@ -266,7 +269,10 @@ def create_impression_event( | |||||||||
|
||||||||||
params[self.EventParams.USERS][0][self.EventParams.SNAPSHOTS].append(impression_params) | ||||||||||
|
||||||||||
return Event(self.EVENTS_URL, params, http_verb=self.HTTP_VERB, headers=self.HTTP_HEADERS) | ||||||||||
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 commentThe 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I agree with copilot lol |
||||||||||
|
||||||||||
return Event(events_url, params, http_verb=self.HTTP_VERB, headers=self.HTTP_HEADERS) | ||||||||||
|
||||||||||
def create_conversion_event( | ||||||||||
self, project_config: ProjectConfig, event_key: str, | ||||||||||
|
@@ -289,4 +295,8 @@ def create_conversion_event( | |||||||||
conversion_params = self._get_required_params_for_conversion(project_config, event_key, event_tags) | ||||||||||
|
||||||||||
params[self.EventParams.USERS][0][self.EventParams.SNAPSHOTS].append(conversion_params) | ||||||||||
return Event(self.EVENTS_URL, params, http_verb=self.HTTP_VERB, headers=self.HTTP_HEADERS) | ||||||||||
|
||||||||||
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 commentThe 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||
|
||||||||||
return Event(events_url, params, http_verb=self.HTTP_VERB, headers=self.HTTP_HEADERS) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
import json | ||
from typing import TYPE_CHECKING, Optional, Type, TypeVar, cast, Any, Iterable, List | ||
from sys import version_info | ||
from enum import Enum | ||
|
||
from . import entities | ||
from . import exceptions | ||
|
@@ -42,6 +43,11 @@ | |
EntityClass = TypeVar('EntityClass') | ||
|
||
|
||
class Region(str, Enum): | ||
US = 'US' | ||
EU = 'EU' | ||
|
||
|
||
class ProjectConfig: | ||
""" Representation of the Optimizely project config. """ | ||
|
||
|
@@ -85,6 +91,13 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): | |
self.host_for_odp: Optional[str] = None | ||
self.all_segments: list[str] = [] | ||
|
||
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 commentThe 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. |
||
|
||
# Utility maps for quick lookup | ||
self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) | ||
self.experiment_id_map: dict[str, entities.Experiment] = self._generate_key_map( | ||
|
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.