Skip to content
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

ref(flags): allow OpenFeature integration to track a single client #3895

Closed
wants to merge 6 commits into from

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Dec 23, 2024

All provider SDKs so far have the concept of a "client" class, which is used to connect to the server and query flags. Since clients can query multiple projects/environments by specifying a context, it is rare for an app to use >1 client.

I discussed with @billyvg and we want to stay consistent for all FF integrations, and focus on tracking one client. This means users should pass a client to the Integration, and we'll register hooks on that client, rather than globally.

Update 12/26:
For backwards compatibility, I think it's best to

  • keep LD as-is: optionally takes a client. If one is not given, then hook into the global singleton.
  • mirror this in OF, optionally taking in a client.

Updated setup code can be seen at: https://github.com/getsentry/sentry-docs/pull/12222/files

@aliu39 aliu39 requested a review from cmanallen December 23, 2024 22:38
Copy link

codecov bot commented Dec 23, 2024

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
17189 4 17185 4771
View the top 2 failed tests by shortest run time
tests.integrations.launchdarkly.test_launchdarkly test_launchdarkly_integration_threaded
Stack Traces | 0.104s run time
.../integrations/launchdarkly/test_launchdarkly.py:104: in test_launchdarkly_integration_threaded
    assert len(events) == 3
E   AssertionError: assert 4 == 3
E    +  where 4 = len([{'breadcrumbs': {'values': []}, 'contexts': {'flags': {'values': [{'flag': 'hello', 'result': True}, {'flag': 'world', 'result': False}]}, 'runtime': {'build': '3.13.1 (main, Dec  4 2024, 06:21:55) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.13.1'}, 'trace': {'parent_span_id': None, 'span_id': 'a7f4715ac316a810', 'trace_id': 'a878f46bec2e489d9780457570f62bb5'}}, 'environment': 'production', 'event_id': 'c0ce8f969dde4809ac3403603b9822a6', ...}, {'breadcrumbs': {'values': []}, 'contexts': {'flags': {'values': [{'flag': 'hello', 'result': True}, {'flag': 'other', 'result': False}]}, 'runtime': {'build': '3.13.1 (main, Dec  4 2024, 06:21:55) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.13.1'}, 'trace': {'parent_span_id': None, 'span_id': 'a7f4715ac316a810', 'trace_id': 'a878f46bec2e489d9780457570f62bb5'}}, 'environment': 'production', 'event_id': '0780d1bb1638401aa162f1e77ab94a50', ...}, {'breadcrumbs': {'values': []}, 'contexts': {'runtime': {'build': '3.13.1 (main, Dec  4 2024, 06:21:55) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.13.1'}, 'trace': {'parent_span_id': None, 'span_id': '90fe13aacddc1ad2', 'trace_id': '888602915d98427dade2f287b3bae04a'}}, 'environment': 'production', 'event_id': '4f4b914610ca40c7b00fc284595a52b8', ...}, {'breadcrumbs': {'values': []}, 'contexts': {'flags': {'values': [{'flag': 'hello', 'result': True}]}, 'runtime': {'build': '3.13.1 (main, Dec  4 2024, 06:21:55) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.13.1'}, 'trace': {'parent_span_id': None, 'span_id': 'a7f4715ac316a810', 'trace_id': 'a878f46bec2e489d9780457570f62bb5'}}, 'environment': 'production', 'event_id': '7b7c3f0e9b8c4e8ea94dba59715ae1b1', ...}])
tests.test_basics test_classmethod_tracing
Stack Traces | 0.143s run time
tests/test_basics.py:946: in test_classmethod_tracing
    assert fake_start_child.call_count == 1
E   AssertionError: assert 3 == 1
E    +  where 3 = <MagicMock name='mock.start_child' id='140384176632496'>.call_count
View the full list of 1 ❄️ flaky tests
tests.test_basics test_staticmethod_tracing

Flake rate in main: 18.84% (Passed 56 times, Failed 13 times)

Stack Traces | 0.079s run time
tests/test_basics.py:925: in test_staticmethod_tracing
    assert fake_start_child.call_count == 1
E   AssertionError: assert 2 == 1
E    +  where 2 = <MagicMock name='mock.start_child' id='140283105610336'>.call_count

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@aliu39 aliu39 changed the title ref(flags): change LaunchDarkly and OpenFeature integrations to track a single client instance ref(flags): allow OpenFeature integration to track a single client Dec 27, 2024
@cmanallen
Copy link
Member

I discussed with @billyvg and we want to stay consistent for all FF integrations, and focus on tracking one client. This means users should pass a client to the Integration, and we'll register hooks on that client, rather than globally.

This needs to be discussed as a team and documented. Writing code is the last step not the first. I'm closing this for now. Not to stop us from working on this forever but to prevent someone from merging code accidentally. We can re-open once we understand the implications of the decisions we're making.

@cmanallen cmanallen closed this Jan 3, 2025
antonpirker added a commit that referenced this pull request Jan 7, 2025
Adds an integration for tracking flag evaluations from [Unleash](https://www.getunleash.io/) customers. 

Implementation
Unleash has no native support for evaluation hooks/listeners, unless the user opts in for each flag. Therefore we decided to patch the `is_enabled` and `get_variant` methods on the `UnleashClient` class. The methods are wrapped and the only side effect is writing to Sentry scope, so users shouldn't see any change in behavior. 

We patch one `UnleashClient` instance instead of the whole class. The reasons for this are described in
- #3895

It's also safer to not modify the unleash import.

References
- https://develop.sentry.dev/sdk/expected-features/#feature-flags
- https://docs.getunleash.io/reference/sdks/python for methods we're patching/wrapping

---------

Co-authored-by: Anton Pirker <[email protected]>
Co-authored-by: Colton Allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants