Skip to content

Commit

Permalink
feat(flags): remove Unleash get_variant patching code (#3914)
Browse files Browse the repository at this point in the history
Follow-up to #3888

The original PR patched 2 methods used for evaluating feature flags,
`is_enabled` (simple toggle on/off) and `get_variant` (returns a dict of
metadata, see
https://docs.getunleash.io/reference/sdks/python#getting-a-variant).

We want to remove all `get_variant` code since we only support boolean
flag evals atm. It seems like the main usecase for variants is reading
payloads (non-bool) for A/B/multivariate testing. This could lead to a
lot of extraneous flags, so until it is requested and/or we support
non-bool values, let's not patch this method.
  • Loading branch information
aliu39 authored Jan 14, 2025
1 parent 288f69a commit 2ee194c
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 199 deletions.
16 changes: 0 additions & 16 deletions sentry_sdk/integrations/unleash.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ def setup_once():
# type: () -> None
# Wrap and patch evaluation methods (instance methods)
old_is_enabled = UnleashClient.is_enabled
old_get_variant = UnleashClient.get_variant

@wraps(old_is_enabled)
def sentry_is_enabled(self, feature, *args, **kwargs):
Expand All @@ -32,19 +31,4 @@ def sentry_is_enabled(self, feature, *args, **kwargs):

return enabled

@wraps(old_get_variant)
def sentry_get_variant(self, feature, *args, **kwargs):
# type: (UnleashClient, str, *Any, **Any) -> Any
variant = old_get_variant(self, feature, *args, **kwargs)
enabled = variant.get("enabled", False)

# Payloads are not always used as the feature's value for application logic. They
# may be used for metrics or debugging context instead. Therefore, we treat every
# variant as a boolean toggle, using the `enabled` field.
flags = sentry_sdk.get_current_scope().flags
flags.set(feature, enabled)

return variant

UnleashClient.is_enabled = sentry_is_enabled # type: ignore
UnleashClient.get_variant = sentry_get_variant # type: ignore
156 changes: 7 additions & 149 deletions tests/integrations/unleash/test_unleash.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def test_is_enabled(sentry_init, capture_events, uninstall_integration):
uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
client = UnleashClient() # type: ignore[arg-type]
sentry_init(integrations=[UnleashIntegration()])
client.is_enabled("hello")
client.is_enabled("world")
Expand All @@ -34,41 +34,12 @@ def test_is_enabled(sentry_init, capture_events, uninstall_integration):
}


def test_get_variant(sentry_init, capture_events, uninstall_integration):
uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
client.get_variant("no_payload_feature")
client.get_variant("string_feature")
client.get_variant("json_feature")
client.get_variant("csv_feature")
client.get_variant("number_feature")
client.get_variant("unknown_feature")

events = capture_events()
sentry_sdk.capture_exception(Exception("something wrong!"))

assert len(events) == 1
assert events[0]["contexts"]["flags"] == {
"values": [
{"flag": "no_payload_feature", "result": True},
{"flag": "string_feature", "result": True},
{"flag": "json_feature", "result": True},
{"flag": "csv_feature", "result": True},
{"flag": "number_feature", "result": True},
{"flag": "unknown_feature", "result": False},
]
}


def test_is_enabled_threaded(sentry_init, capture_events, uninstall_integration):
uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
client = UnleashClient() # type: ignore[arg-type]
sentry_init(integrations=[UnleashIntegration()])
events = capture_events()

def task(flag_key):
Expand Down Expand Up @@ -112,63 +83,14 @@ def task(flag_key):
}


def test_get_variant_threaded(sentry_init, capture_events, uninstall_integration):
uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
events = capture_events()

def task(flag_key):
# Creates a new isolation scope for the thread.
# This means the evaluations in each task are captured separately.
with sentry_sdk.isolation_scope():
client.get_variant(flag_key)
# use a tag to identify to identify events later on
sentry_sdk.set_tag("task_id", flag_key)
sentry_sdk.capture_exception(Exception("something wrong!"))

# Capture an eval before we split isolation scopes.
client.get_variant("hello")

with cf.ThreadPoolExecutor(max_workers=2) as pool:
pool.map(task, ["no_payload_feature", "other"])

# Capture error in original scope
sentry_sdk.set_tag("task_id", "0")
sentry_sdk.capture_exception(Exception("something wrong!"))

assert len(events) == 3
events.sort(key=lambda e: e["tags"]["task_id"])

assert events[0]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
]
}
assert events[1]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
{"flag": "no_payload_feature", "result": True},
]
}
assert events[2]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
{"flag": "other", "result": False},
]
}


@pytest.mark.skipif(sys.version_info < (3, 7), reason="requires python3.7 or higher")
def test_is_enabled_asyncio(sentry_init, capture_events, uninstall_integration):
asyncio = pytest.importorskip("asyncio")
uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
client = UnleashClient() # type: ignore[arg-type]
sentry_init(integrations=[UnleashIntegration()])
events = capture_events()

async def task(flag_key):
Expand Down Expand Up @@ -212,66 +134,12 @@ async def runner():
}


@pytest.mark.skipif(sys.version_info < (3, 7), reason="requires python3.7 or higher")
def test_get_variant_asyncio(sentry_init, capture_events, uninstall_integration):
asyncio = pytest.importorskip("asyncio")

uninstall_integration(UnleashIntegration.identifier)

with mock_unleash_client():
client = UnleashClient()
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
events = capture_events()

async def task(flag_key):
with sentry_sdk.isolation_scope():
client.get_variant(flag_key)
# use a tag to identify to identify events later on
sentry_sdk.set_tag("task_id", flag_key)
sentry_sdk.capture_exception(Exception("something wrong!"))

async def runner():
return asyncio.gather(task("no_payload_feature"), task("other"))

# Capture an eval before we split isolation scopes.
client.get_variant("hello")

asyncio.run(runner())

# Capture error in original scope
sentry_sdk.set_tag("task_id", "0")
sentry_sdk.capture_exception(Exception("something wrong!"))

assert len(events) == 3
events.sort(key=lambda e: e["tags"]["task_id"])

assert events[0]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
]
}
assert events[1]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
{"flag": "no_payload_feature", "result": True},
]
}
assert events[2]["contexts"]["flags"] == {
"values": [
{"flag": "hello", "result": False},
{"flag": "other", "result": False},
]
}


def test_wraps_original(sentry_init, uninstall_integration):
with mock_unleash_client():
client = UnleashClient()
client = UnleashClient() # type: ignore[arg-type]

mock_is_enabled = mock.Mock(return_value=random() < 0.5)
mock_get_variant = mock.Mock(return_value={"enabled": random() < 0.5})
client.is_enabled = mock_is_enabled
client.get_variant = mock_get_variant

uninstall_integration(UnleashIntegration.identifier)
sentry_init(integrations=[UnleashIntegration()]) # type: ignore
Expand All @@ -283,26 +151,16 @@ def test_wraps_original(sentry_init, uninstall_integration):
{"kwarg": 1},
)

res = client.get_variant("test-flag", "arg", kwarg=1)
assert res == mock_get_variant.return_value
assert mock_get_variant.call_args == (
("test-flag", "arg"),
{"kwarg": 1},
)


def test_wrapper_attributes(sentry_init, uninstall_integration):
with mock_unleash_client():
client = UnleashClient() # <- Returns a MockUnleashClient
client = UnleashClient() # type: ignore[arg-type]

original_is_enabled = client.is_enabled
original_get_variant = client.get_variant

uninstall_integration(UnleashIntegration.identifier)
sentry_init(integrations=[UnleashIntegration()]) # type: ignore

# Mock clients methods have not lost their qualified names after decoration.
assert client.is_enabled.__name__ == "is_enabled"
assert client.is_enabled.__qualname__ == original_is_enabled.__qualname__
assert client.get_variant.__name__ == "get_variant"
assert client.get_variant.__qualname__ == original_get_variant.__qualname__
36 changes: 2 additions & 34 deletions tests/integrations/unleash/testutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ def mock_unleash_client():
Temporarily replaces UnleashClient's methods with mock implementations
for testing.
This context manager swaps out UnleashClient's __init__, is_enabled,
and get_variant methods with mock versions from MockUnleashClient.
This context manager swaps out UnleashClient's __init__ and is_enabled,
methods with mock versions from MockUnleashClient.
Original methods are restored when exiting the context.
After mocking the client class the integration can be initialized.
Expand All @@ -23,17 +23,14 @@ def mock_unleash_client():
"""
old_init = UnleashClient.__init__
old_is_enabled = UnleashClient.is_enabled
old_get_variant = UnleashClient.get_variant

UnleashClient.__init__ = MockUnleashClient.__init__
UnleashClient.is_enabled = MockUnleashClient.is_enabled
UnleashClient.get_variant = MockUnleashClient.get_variant

yield

UnleashClient.__init__ = old_init
UnleashClient.is_enabled = old_is_enabled
UnleashClient.get_variant = old_get_variant


class MockUnleashClient:
Expand All @@ -44,34 +41,5 @@ def __init__(self, *a, **kw):
"world": False,
}

self.feature_to_variant = {
"string_feature": {
"name": "variant1",
"enabled": True,
"payload": {"type": "string", "value": "val1"},
},
"json_feature": {
"name": "variant1",
"enabled": True,
"payload": {"type": "json", "value": '{"key1": 0.53}'},
},
"number_feature": {
"name": "variant1",
"enabled": True,
"payload": {"type": "number", "value": "134.5"},
},
"csv_feature": {
"name": "variant1",
"enabled": True,
"payload": {"type": "csv", "value": "abc 123\ncsbq 94"},
},
"no_payload_feature": {"name": "variant1", "enabled": True},
}

self.disabled_variant = {"name": "disabled", "enabled": False}

def is_enabled(self, feature, *a, **kw):
return self.features.get(feature, False)

def get_variant(self, feature, *a, **kw):
return self.feature_to_variant.get(feature, self.disabled_variant)

0 comments on commit 2ee194c

Please sign in to comment.