From 2ee194c0d4fac809b40ef81d90ae859998962afa Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Tue, 14 Jan 2025 00:00:55 -0800 Subject: [PATCH] feat(flags): remove Unleash get_variant patching code (#3914) Follow-up to /~https://github.com/getsentry/sentry-python/pull/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. --- sentry_sdk/integrations/unleash.py | 16 --- tests/integrations/unleash/test_unleash.py | 156 +-------------------- tests/integrations/unleash/testutils.py | 36 +---- 3 files changed, 9 insertions(+), 199 deletions(-) diff --git a/sentry_sdk/integrations/unleash.py b/sentry_sdk/integrations/unleash.py index 442ec39d0f..c7108394d0 100644 --- a/sentry_sdk/integrations/unleash.py +++ b/sentry_sdk/integrations/unleash.py @@ -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): @@ -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 diff --git a/tests/integrations/unleash/test_unleash.py b/tests/integrations/unleash/test_unleash.py index 9a7a3f57bd..379abba8f6 100644 --- a/tests/integrations/unleash/test_unleash.py +++ b/tests/integrations/unleash/test_unleash.py @@ -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") @@ -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): @@ -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): @@ -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 @@ -283,20 +151,12 @@ 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 @@ -304,5 +164,3 @@ def test_wrapper_attributes(sentry_init, uninstall_integration): # 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__ diff --git a/tests/integrations/unleash/testutils.py b/tests/integrations/unleash/testutils.py index c424b34c3a..07b065e2f0 100644 --- a/tests/integrations/unleash/testutils.py +++ b/tests/integrations/unleash/testutils.py @@ -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. @@ -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: @@ -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)