AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #814

manstis · 2024-02-07T13:49:21Z

Jira Issue: https://issues.redhat.com/browse/AAP-20249

Replaces #804

Description

This PR moves the "Schema 2 Telemetry analytics" feature flag to LaunchDarkly.

Launch Darkly has a rule that checks whether the feature is enabled for a given org_id.

The feature flag controls:-

Exposure of the "Telemetry" section in the Admin Console
Enabling/disabling the "Schema 2 Telemetry" REST API endpoints
Enabling/disabling sending "Schema 2 Telemetry" events to Segment.
Including/excluding the "Schema 2 Telemetry" opt-in/out flag in the api/v0/me REST API endpoint.

Testing

You can test locally; with a flagdata.json file locally.

See #776 for details 🎉

"schema_2_telemetry_org_enabled": true

Each of the items listed in the description should be enabled.

"schema_2_telemetry_org_enabled": false

Each of the items listed in the description should be disabled.

You can test using "Development" LaunchDarkly SDK key.

You may need to update the "schema_2_telemetry_org_enabled" rule to include your org_id.

Steps to test

Pull down the PR
make admin-portal-bundle
make start-backends
make create-application
make run-server (or whatever you normally do)

Scenarios tested

See "Testing" above.

Production deployment

This code change is ready for production on its own
This code change requires the following considerations before going to production:

I removed the system-wide environment variable ADMIN_PORTAL_TELEMETRY_OPT_ENABLED.

This needs to be removed in Staging Secret Manager too.

robinbobbitt

Magic! Thanks for this. Just a few things to please take a look.

robinbobbitt · 2024-02-08T04:54:42Z

ansible_wisdom/ai/feature_flags.py

+        if self.client:
+            logger.debug(f"constructing User context for Organization '{org_id}'")
+            logger.debug(f"retrieving feature flag '{WisdomFlags.SCHEMA_2_TELEMETRY_ORG_ENABLED}'")
+            context = Context.builder(str(org_id)).set("org_id", org_id).build()


I'm just reading a little on "context kinds" and wondering if we should either (a) specify the userid as the context key, since we're using the user kind here by default, or (b) create an "organization" kind and specify the organization kind in this statement. IDK if it would end up ever mattering, but as it is here, we're going to end up with a mix of context keys that are part user guids and part org ids. wdyt?

I agree. I'll change my code to provide the user.UUID as the key and record the User Name too.

It might be useful to know that the check for "Schema 2 Telemetry" came from User X. Who knows!

I have to navigate the request->user->organization anyway.. so it's not difficult to get to the data.

Hmmmmmmmmmmmmm...

I did agree.. but when it came to making the changes I'd have needed to move is_schema_2_telemetry_enabled to the User model as I don't have access to the User from an Organization. This all then started smelling bad. The attribute - in OO terms - is not relevant to a User and the change would have been simply to support what amounts to logging additional data.

So, I didn't make the change after all and prefer to create a new kind in LaunchDarkly for organization.

If we want to record who tried to access "Schema 2 telemetry" I propose we add additional logging in the service and not bastardize our object model and the use of LaunchDarkly.

WDYT?

I'm good with the organization kind. @jameswnl FYI/shout with concerns.

that sounds good. I can create the organization kind, what do u think of this?

{ "kind": "organization", "key": "123", }

~~updated that to use org_id to line up with your code~~

actually, by default, Context always use key as the attribute name. So you just need to do
Context.builder(str(org_id)).build() which will build

{ "kind": "organization", "key": "<ord_id>", }

And the context kind definition only really just take a kind as input, we can't/don't need to specify the other attributes (key is default the PK of the kind)

Ok, happy to simplify and change the LaunchDarkly rule accordingly. I'm new to how LaunchDarkly works. I'd assumed it was in essence a remote key/value pair store.

So you just need to do
Context.builder(str(org_id)).build()

How would that know to use the organization kind? I think we might need Context.create(str(org_id), "organization") per https://docs.launchdarkly.com/sdk/features/user-context-config#python.

oh, yeah, I missed that :(
and seems using the create() method would be simpler/intuitive (the builder pattern isn't helping much in this case, IMHO)

the from_dict is even easier https://launchdarkly-python-sdk.readthedocs.io/en/latest/api-main.html#ldclient.ContextBuilder

robinbobbitt · 2024-02-08T05:04:17Z

ansible_wisdom/organizations/tests/test_organizations.py

+
+
+class TestIsOrgLightspeedSubscriber(TestCase):
+    def test_org_with_telemetry_schema_2_enabled(self):


I think this test and the next one are establishing that telemetry_opt_out value has no bearing on is_schema_2_telemetry_override_enabled, right? I think maybe the names need to be revisited to be clear we're switching up the opt out, not the feature flag, in the two tests.

Great spot!

All of the tests in test_organizations.py are out of date.

IDK why they pass... it is by luck rather than design.

I've rewritten them.

robinbobbitt · 2024-02-08T05:05:12Z

ansible_wisdom/organizations/tests/test_organizations.py

+from organizations.models import Organization
+
+
+class TestIsOrgLightspeedSubscriber(TestCase):


I think you probably meant to rename this?

robinbobbitt · 2024-02-08T05:10:27Z

ansible_wisdom/organizations/tests/test_organizations.py

+    @override_settings(LAUNCHDARKLY_SDK_KEY='dummy_key')
+    @patch.object(feature_flags, 'LDClient')
+    def test_org_with_telemetry_schema_2_disabled_with_feature_flags_no_override(self, LDClient):
+        LDClient.return_value.variation.return_value = '999'


These tests look like they're written for the old way you were doing the flag, where it returned a comma separated list of values. I'm admittedly a little surprised they are still all passing. Please take a look.

I agree.. See here.

ansible_wisdom/ai/feature_flags.py

jameswnl · 2024-02-09T15:14:47Z

ansible_wisdom/organizations/models.py


 logger = logging.getLogger(__name__)


 class Organization(models.Model):
    id = models.IntegerField(primary_key=True)
    telemetry_opt_out = models.BooleanField(default=False)
+
+    @cached_property
+    def is_schema_2_telemetry_enabled(self) -> bool:


~~this method name is ambiguous. I thought it will return the combined effect of both the feature flag evaluation and the telemtry_opt_out.~~

~~can u either~~
~~- rename to e.g. schema_2_telemetry_feature_flag_enabled~~
~~- or adapt it to return the combined effect of telemtry_opt_out and feature flag evaluation result?~~

nvm, I think it is alright

manstis requested review from romartin, goneri, jameswnl and robinbobbitt February 7, 2024 13:49

manstis mentioned this pull request Feb 7, 2024

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #804

Closed

1 task

manstis force-pushed the AAP-20249 branch from ecfaec7 to 08ac0d5 Compare February 7, 2024 14:21

robinbobbitt requested changes Feb 8, 2024

View reviewed changes

manstis force-pushed the AAP-20249 branch from 9616944 to db8ffb8 Compare February 8, 2024 09:12

jameswnl reviewed Feb 8, 2024

View reviewed changes

ansible_wisdom/ai/feature_flags.py Outdated Show resolved Hide resolved

manstis added 3 commits February 9, 2024 10:22

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations

1ffbbc8

Updates following peer review.

b0e926f

More updates.

12ea8e2

manstis force-pushed the AAP-20249 branch from db8ffb8 to 12ea8e2 Compare February 9, 2024 10:38

jameswnl reviewed Feb 9, 2024

View reviewed changes

jameswnl approved these changes Feb 9, 2024

View reviewed changes

robinbobbitt approved these changes Feb 9, 2024

View reviewed changes

manstis merged commit 8071f4f into main Feb 9, 2024
8 checks passed

romartin mentioned this pull request Mar 22, 2024

Admin Dashboard: [Feature flag] Drop -M-A-G-I-C- Organizations #893

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #814

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #814

manstis commented Feb 7, 2024 •

edited

Loading

robinbobbitt left a comment

robinbobbitt Feb 8, 2024

manstis Feb 8, 2024

manstis Feb 8, 2024

robinbobbitt Feb 8, 2024

jameswnl Feb 8, 2024 •

edited

Loading

jameswnl Feb 8, 2024

manstis Feb 8, 2024

robinbobbitt Feb 8, 2024

jameswnl Feb 8, 2024

jameswnl Feb 8, 2024

robinbobbitt Feb 8, 2024

manstis Feb 8, 2024

robinbobbitt Feb 8, 2024

manstis Feb 8, 2024

robinbobbitt Feb 8, 2024

manstis Feb 8, 2024

jameswnl Feb 9, 2024 •

edited

Loading

jameswnl Feb 9, 2024



		class TestIsOrgLightspeedSubscriber(TestCase):
		def test_org_with_telemetry_schema_2_enabled(self):

		from organizations.models import Organization


		class TestIsOrgLightspeedSubscriber(TestCase):

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #814

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #814

Conversation

manstis commented Feb 7, 2024 • edited Loading

Description

Testing

Steps to test

Scenarios tested

Production deployment

robinbobbitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswnl Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswnl Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manstis commented Feb 7, 2024 •

edited

Loading

jameswnl Feb 8, 2024 •

edited

Loading

jameswnl Feb 9, 2024 •

edited

Loading