Feature/support feature flags #12

imhmdb · 2021-06-04T22:06:12Z

Follows spec for implementing feature flags support:
https://github.com/PostHog/posthog.com/pull/1455/files

yakkomajuri

Hey, thanks a lot for this! Appreciate you submitting it upstream.

Left one inline comment, but overall looking good. Some thoughts:

I'm happy to merge this with only LibCurl support
Some tests that mock the endpoint response and verify that the code handles the response appropriately should be enough.
This implementation appears correct and is an improvement over what we currently have (no feature flags), so it should be valid to merge. However, I'd appreciate if you could consider adding support for is_simple_flag (see Python lib). I'm adding feature flags to posthog-node this week and will write a spec for implementing flags in other libraries. Essentially, for is_simple_flag, we use a Personal API Key to poll for flags and if they're "simple" (only rollout percentage, no filters), we calculate them locally for speed. This can definitely be a separate PR and I'm happy to help.

lib/Consumer/LibCurl.php

yakkomajuri · 2021-06-09T11:49:28Z

Node is done and the "spec" is written

@imhmdb are you planning on making the changes here? totally understand if not, just checking

imhmdb · 2021-06-09T15:59:56Z

Node is done and the "spec" is written

@imhmdb are you planning on making the changes here? totally understand if not, just checking

of course, I'll give it a shot today and see how it goes.

imhmdb · 2021-06-10T17:49:20Z

I think this only needs to have tests fixed with mocks

… response

imhmdb · 2021-06-12T00:57:31Z

Done fixing tests with the mocked response of simple flags.

yakkomajuri · 2021-06-14T10:45:10Z

Awesome, thanks a lot! I'll give this a review latest by tomorrow

yakkomajuri

This is awesome! Follows the spec ~~perfectly~~ for the most part (see below). I'm about to test it locally and will report back.

Thanks again for doing this.

One final comment that just came to mind: should we also error out if people chose not to use libcurl and they try to use feature flags?

yakkomajuri · 2021-06-16T12:55:12Z

lib/Client.php

+    private function loadFeatureFlags(): void
+    {
+        $response = $this->httpClient->sendRequest(
+            '/api/feature_flag',


We've made a change since you last edited this, so this should now be /api/feature_flag?token=project_api_key_here.

No way you could have known though!

yakkomajuri · 2021-06-16T12:56:19Z

lib/Client.php

+            $this->featureFlags = [];
+        }
+
+        $this->featureFlags = $responseBody['results'];


Another change we made is that this should filter by active flags.

The active property is in $responseBody['results'][i]['active']

yakkomajuri · 2021-06-16T12:58:12Z

test/PostHogTest.php

+    {
+        $this->assertIsArray(PostHog::fetchEnabledFeatureFlags('user-id'));
+    }
+


could we add a test for isSimpleFlagEnabled?

Flag of key a with distinct id b should be on for rollout percentage 42 but off for rollout percentage 40

yakkomajuri

Having tried this and re-read the code - there are a few issues. First is a POST request to api/feature_flag. Second is that we're missing a poller. We shouldn't load feature flags once, we should load them regularly at an interval in the background.

yakkomajuri · 2021-06-16T15:14:48Z

lib/Client.php

+
+    private function loadFeatureFlags(): void
+    {
+        $response = $this->httpClient->sendRequest(


Ok so I tried this locally and it didn't work. I suspect this is a potential problem. We're sending a POST request here, when it should actually be a GET request.

imhmdb · 2021-06-16T15:41:00Z

This is awesome! Follows the spec ~~perfectly~~ for the most part (see below). I'm about to test it locally and will report back.

Thanks again for doing this.

One final comment that just came to mind: should we also error out if people chose not to use libcurl and they try to use feature flags?

Hi, @yakkomajuri

Regarding the libcurl error, when I reimplemented according to spec I moved the feature flags from libcurl consumer into the client, not sure if you mean ext-curl and not the libcurl-posthog-consumer here, cuz that would be a valid case to error on.

Regarding the poller, I wasn't sure how can I keep polling at an interval when php doesn't supports threads or pools, have this poller been implemented in a php-like client that I can take insights from?

Regarding the errors on the POST request, this sure is weird As I made sure the tests were working on my production environment keys instead of debug ones, but maybe the tests were also broken 😂

I'll sure take a second look and try to debug and fix the addressed issues.

Side note: I just got out of surgery so it might take me a week or two of recovery until I can get my hands on this thing again, just letting you guys know.

yakkomajuri · 2021-06-16T17:07:36Z

I could also be at fault here - not a PHP dev, so if anything I say sounds crazy, it probably is.

Also, get better! No rush!

I might also pick this up if I get the time.

Thanks again for all your work @imhmdb

yakkomajuri · 2021-07-08T12:00:41Z

Closing in favor of #18

@posthog-bot please add @imhmdb for code

posthog-contributions-bot · 2021-07-08T12:00:48Z

@yakkomajuri

I've put up a pull request to add @imhmdb! 🎉

Twixes · 2021-07-16T21:12:40Z

@posthog-bot please add @imhmdb for code

posthog-contributions-bot · 2021-07-16T21:12:42Z

@Twixes

@imhmdb we tried to send you an email with a merch code but that failed :(
We didn't give up on getting you some merch though, so email yakko [at] posthog [.] com and he'll send you a code!

Twixes · 2021-07-16T21:12:56Z

@posthog-bot please add @imhmdb for code

posthog-contributions-bot · 2021-07-16T21:13:04Z

@Twixes

I've put up a pull request to add @imhmdb! 🎉

yakkomajuri self-requested a review June 7, 2021 10:57

yakkomajuri suggested changes Jun 7, 2021

View reviewed changes

lib/Consumer/LibCurl.php Outdated Show resolved Hide resolved

yakkomajuri added this to To Do Next Sprint (1.27 1/2) in Extensibility Jun 7, 2021

yakkomajuri self-assigned this Jun 7, 2021

Twixes moved this from To Do This Sprint (1.27 1/2) to In Progress in Extensibility Jun 8, 2021

imhmdb added 4 commits June 9, 2021 18:27

Upgrade phplint

692e8ba

Feature/support feature flags decide API

706baa3

Change default host to app.posthog.com

ceae2ac

Remove extra line

8c3d022

imhmdb closed this Jun 9, 2021

imhmdb reopened this Jun 9, 2021

imhmdb added 4 commits June 9, 2021 20:34

Extract http client functionalities

dbc3357

(WIP) Rework feature flags based on spec `PostHog/posthog.com#1455

2a35f87

Don't set payload on get requests

25aefc7

Implement isSimpleFlagEnabled

3b4bf56

imhmdb requested a review from yakkomajuri June 10, 2021 17:49

imhmdb added 5 commits June 10, 2021 20:53

Use LONG_SCALE const

bfc6cd7

Fix is_simple_flag tests by mocking response

dcff4fa

Merge remote-tracking branch 'upstream/master'

09c443d

Fix: make rolloutPercentage nullable in isSimpleFlagEnabled

587a1c6

Add test case for isFeatureEnabled with the simple flag in the mocked…

5ddb701

… response

imhmdb changed the title ~~(WIP) Feature/support feature flags decide API~~ Feature/support feature flags decide API Jun 12, 2021

imhmdb changed the title ~~Feature/support feature flags decide API~~ Feature/support feature flags Jun 12, 2021

imhmdb added 2 commits June 14, 2021 20:43

Fix user agent undefined array key

bf461fe

Cleanup isFeatureEnabled docblock

86ef9fb

yakkomajuri suggested changes Jun 16, 2021

View reviewed changes

Extensibility automation moved this from In Progress to In Review/Pending Deployment Jun 16, 2021

yakkomajuri added the extra merch label Jun 16, 2021

yakkomajuri suggested changes Jun 16, 2021

View reviewed changes

This was referenced Jun 18, 2021

Sprint 1.27.0 2/2 - Jun 21 to Jul 2 (Funnels #1) PostHog/posthog#4765

Closed

Add feature flags #18

Merged

yakkomajuri closed this Jul 8, 2021

posthog-contributions-bot bot mentioned this pull request Jul 8, 2021

🤖: Add imhmdb as a contributor 🎉 PostHog/posthog#5038

Closed

posthog-contributions-bot bot mentioned this pull request Jul 16, 2021

🤖: Add imhmdb as a contributor 🎉 PostHog/posthog#5181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/support feature flags #12

Feature/support feature flags #12

imhmdb commented Jun 4, 2021 •

edited

yakkomajuri left a comment

yakkomajuri commented Jun 9, 2021

imhmdb commented Jun 9, 2021

imhmdb commented Jun 10, 2021

imhmdb commented Jun 12, 2021

yakkomajuri commented Jun 14, 2021

yakkomajuri left a comment •

edited

yakkomajuri Jun 16, 2021

yakkomajuri Jun 16, 2021

yakkomajuri Jun 16, 2021

yakkomajuri left a comment

yakkomajuri Jun 16, 2021

imhmdb commented Jun 16, 2021

yakkomajuri commented Jun 16, 2021

yakkomajuri commented Jul 8, 2021

posthog-contributions-bot bot commented Jul 8, 2021

Twixes commented Jul 16, 2021

posthog-contributions-bot bot commented Jul 16, 2021

Twixes commented Jul 16, 2021

posthog-contributions-bot bot commented Jul 16, 2021

Feature/support feature flags #12

Feature/support feature flags #12

Conversation

imhmdb commented Jun 4, 2021 • edited

yakkomajuri left a comment

Choose a reason for hiding this comment

yakkomajuri commented Jun 9, 2021

imhmdb commented Jun 9, 2021

imhmdb commented Jun 10, 2021

imhmdb commented Jun 12, 2021

yakkomajuri commented Jun 14, 2021

yakkomajuri left a comment • edited

Choose a reason for hiding this comment

yakkomajuri Jun 16, 2021

Choose a reason for hiding this comment

yakkomajuri Jun 16, 2021

Choose a reason for hiding this comment

yakkomajuri Jun 16, 2021

Choose a reason for hiding this comment

yakkomajuri left a comment

Choose a reason for hiding this comment

yakkomajuri Jun 16, 2021

Choose a reason for hiding this comment

imhmdb commented Jun 16, 2021

yakkomajuri commented Jun 16, 2021

yakkomajuri commented Jul 8, 2021

posthog-contributions-bot bot commented Jul 8, 2021

Twixes commented Jul 16, 2021

posthog-contributions-bot bot commented Jul 16, 2021

Twixes commented Jul 16, 2021

posthog-contributions-bot bot commented Jul 16, 2021

imhmdb commented Jun 4, 2021 •

edited

yakkomajuri left a comment •

edited