-
Notifications
You must be signed in to change notification settings - Fork 66
feat: events #357
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
feat: events #357
Changes from all commits
80d845e
6e00602
cad378b
71f5bec
dbd97bf
cd2066c
b79259e
1c5d6dd
4557060
1961fea
0cb9c21
aa5380a
ec9cfae
af050b8
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,12 @@ | |||||
| SDK_NAME, | ||||||
| SDK_VERSION, | ||||||
| ) | ||||||
| from UnleashClient.events import UnleashEvent, UnleashEventType | ||||||
| from UnleashClient.events import ( | ||||||
| BaseEvent, | ||||||
| UnleashEvent, | ||||||
| UnleashEventType, | ||||||
| UnleashReadyEvent, | ||||||
| ) | ||||||
| from UnleashClient.loader import load_features | ||||||
| from UnleashClient.periodic_tasks import ( | ||||||
| aggregate_and_send_metrics, | ||||||
|
|
@@ -46,6 +51,37 @@ | |||||
| ] | ||||||
|
|
||||||
|
|
||||||
| def build_ready_callback( | ||||||
| event_callback: Optional[Callable[[BaseEvent], None]] = None, | ||||||
| ) -> Optional[Callable]: | ||||||
| """ | ||||||
| Builds a callback function that can be used to notify when the Unleash client is ready. | ||||||
| """ | ||||||
|
|
||||||
| if not event_callback: | ||||||
| return None | ||||||
|
|
||||||
| already_fired = False | ||||||
|
|
||||||
| def ready_callback() -> None: | ||||||
| """ | ||||||
| Callback function to notify that the Unleash client is ready. | ||||||
| This will only call the event_callback once. | ||||||
| """ | ||||||
| nonlocal already_fired | ||||||
| if already_fired: | ||||||
| return | ||||||
| if event_callback: | ||||||
| event = UnleashReadyEvent( | ||||||
| event_type=UnleashEventType.READY, | ||||||
| event_id=uuid.uuid4(), | ||||||
| ) | ||||||
| already_fired = True | ||||||
| event_callback(event) | ||||||
|
|
||||||
| return ready_callback | ||||||
|
|
||||||
|
|
||||||
| # pylint: disable=dangerous-default-value | ||||||
| class UnleashClient: | ||||||
| """ | ||||||
|
|
@@ -99,7 +135,7 @@ def __init__( | |||||
| scheduler: Optional[BaseScheduler] = None, | ||||||
| scheduler_executor: Optional[str] = None, | ||||||
| multiple_instance_mode: InstanceAllowType = InstanceAllowType.WARN, | ||||||
| event_callback: Optional[Callable[[UnleashEvent], None]] = None, | ||||||
| event_callback: Optional[Callable[[BaseEvent], None]] = None, | ||||||
|
Contributor
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.
Suggested change
Following my other comment, the alternative would be something like this. |
||||||
| ) -> None: | ||||||
| custom_headers = custom_headers or {} | ||||||
| custom_options = custom_options or {} | ||||||
|
|
@@ -132,6 +168,7 @@ def __init__( | |||||
| self.unleash_project_name = project_name | ||||||
| self.unleash_verbose_log_level = verbose_log_level | ||||||
| self.unleash_event_callback = event_callback | ||||||
| self._ready_callback = build_ready_callback(event_callback) | ||||||
|
|
||||||
| self._do_instance_check(multiple_instance_mode) | ||||||
|
|
||||||
|
|
@@ -283,12 +320,15 @@ def initialize_client(self, fetch_toggles: bool = True) -> None: | |||||
| "request_timeout": self.unleash_request_timeout, | ||||||
| "request_retries": self.unleash_request_retries, | ||||||
| "project": self.unleash_project_name, | ||||||
| "event_callback": self.unleash_event_callback, | ||||||
| "ready_callback": self._ready_callback, | ||||||
| } | ||||||
| job_func: Callable = fetch_and_load_features | ||||||
| else: | ||||||
| job_args = { | ||||||
| "cache": self.cache, | ||||||
| "engine": self.engine, | ||||||
| "ready_callback": self._ready_callback, | ||||||
| } | ||||||
| job_func = load_features | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| from dataclasses import dataclass | ||
| from enum import Enum | ||
| from json import loads | ||
| from typing import Optional | ||
| from uuid import UUID | ||
|
|
||
|
|
@@ -11,17 +12,51 @@ class UnleashEventType(Enum): | |
|
|
||
| FEATURE_FLAG = "feature_flag" | ||
| VARIANT = "variant" | ||
| FETCHED = "fetched" | ||
| READY = "ready" | ||
|
|
||
|
|
||
| @dataclass | ||
| class UnleashEvent: | ||
| class BaseEvent: | ||
| """ | ||
| Dataclass capturing information from an Unleash feature flag or variant check. | ||
| Base event type for all events in the Unleash client. | ||
| """ | ||
|
|
||
| event_type: UnleashEventType | ||
| event_id: UUID | ||
|
|
||
|
|
||
| @dataclass | ||
| class UnleashEvent(BaseEvent): | ||
| """ | ||
| Dataclass capturing information from an Unleash feature flag or variant check. | ||
| """ | ||
|
|
||
| context: dict | ||
| enabled: bool | ||
| feature_name: str | ||
| variant: Optional[str] = "" | ||
|
|
||
|
|
||
| @dataclass | ||
| class UnleashReadyEvent(BaseEvent): | ||
| """ | ||
| Event indicating that the Unleash client is ready. | ||
| """ | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| @dataclass | ||
| class UnleashFetchedEvent(BaseEvent): | ||
| """ | ||
| Event indicating that the Unleash client has fetched feature flags. | ||
| """ | ||
|
|
||
| raw_features: str | ||
|
|
||
| @property | ||
| def features(self) -> dict: | ||
|
Member
Author
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. Parsing is JSON deserialize, which is expensive. This gives us lazy, cached parse here which the cost is only incurred on demand |
||
| if not hasattr(self, "_parsed_payload"): | ||
| self._parsed_payload = loads(self.raw_features)["features"] | ||
|
Member
Author
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. Annoyingly this now becomes part of the public API. Kinda struggled a little with choosing between only returning the features vs returning the whole response. Leaning ever so slightly in the direction of only returning the features since its less confusing for end users. That being said, every time we've done that in the past (fetch features, bootstrap, load from backup), it's been a mistake that needs to be patched later. Open to being told this is similar but I can't see how right now
Contributor
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. This is still a dict, I think it'd be different if this is an array because it's not easy to extend an array with other properties, but being a dict I think this gives enough flexibility. And I agree, returning less is better IMO |
||
| return self._parsed_payload | ||
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.
Poor mans optional.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.
I wonder if it makes sense to support this optional parameter. IMO, if you're going to call this function, you better have a callback. If you supply something that's not a callback then you should expect an error. But having this ability to call all functions with optional and checking if it's set seems a bit off.
On the other hand, if this function can be called from multiple places it centralizes the check in one single place in the code, so just picking your brain about it.