chore: configurabel endpoint for flags local evaluation#483
Open
patricio-posthog wants to merge 3 commits intomasterfrom
Open
chore: configurabel endpoint for flags local evaluation#483patricio-posthog wants to merge 3 commits intomasterfrom
patricio-posthog wants to merge 3 commits intomasterfrom
Conversation
Contributor
posthog-python Compliance ReportDate: 2026-04-07 17:51:01 UTC ✅ All Tests Passed!0/0 tests passed |
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3783-3855
Comment:
**Prefer parameterised tests**
The four new test methods each exercise a variation of the same scenario (endpoint selection and fallback), but are written as separate methods. Per project convention, these should be parameterised. The two endpoint-selection tests share identical structure and only differ in the env-var value and expected URL prefix — a natural fit for `@parameterized.expand` or a `subTest` loop:
```python
# Example: collapse the two endpoint-selection tests
@parameterized.expand([
("/flags/definitions", "/flags/definitions?"),
(None, "/api/feature_flag/local_evaluation/"),
])
@mock.patch("posthog.client.get")
def test_endpoint_selection(self, env_value, expected_prefix, patch_get):
patch_get.return_value = GetResponse(
data={"flags": [], "group_type_mapping": {}}, etag=None, not_modified=False
)
env = {"POSTHOG_LOCAL_EVALUATION_ENDPOINT": env_value} if env_value else {}
with mock.patch.dict(os.environ, env, clear=False):
if env_value is None:
os.environ.pop("POSTHOG_LOCAL_EVALUATION_ENDPOINT", None)
client = Client(FAKE_TEST_API_KEY, personal_api_key="test-key")
client._fetch_feature_flags_from_api()
self.assertTrue(patch_get.call_args[0][1].startswith(expected_prefix))
```
The two fallback tests (`test_fallback_to_default_on_custom_endpoint_failure` and `test_no_fallback_when_default_endpoint_fails`) can similarly be parameterised on `(env_var, expected_call_count)`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3813-3816
Comment:
**`import os` should be at module level**
`import os` is declared inside the test method body. Python caches module imports so this doesn't break anything, but it is unconventional and harder to notice. Move it to the top of the file alongside the other stdlib imports.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/client.py
Line: 1317-1319
Comment:
**Add inline comment explaining the fallback rationale**
Per project convention, significant behavioral changes — especially ones that silently alter which endpoint is called — should have an inline comment explaining *why*. Without context, a future reader may remove the fallback thinking it is defensive dead-code.
```suggestion
except Exception as e:
# Fall back to the stable Django endpoint when the custom endpoint
# (e.g. the Rust-backed /flags/definitions) fails. This enables a
# zero-downtime gradual migration: the custom endpoint is tried first
# and, on any error, flag evaluation degrades transparently to the
# default rather than being blocked entirely.
if endpoint != self._DEFAULT_LOCAL_EVAL_ENDPOINT:
```
**Rule Used:** Add inline comments to clarify the purpose of sign... ([source](https://app.greptile.com/review/custom-context?memory=4d5b48c5-045d-4693-9cd9-4081bb19508b))
**Learnt From**
[PostHog/posthog#32083](https://github.com/PostHog/posthog/pull/32083)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "lint" | Re-trigger Greptile |
dmarticus
approved these changes
Apr 7, 2026
Comment on lines
+1292
to
+1295
| return os.environ.get( | ||
| "POSTHOG_LOCAL_EVALUATION_ENDPOINT", | ||
| self._DEFAULT_LOCAL_EVAL_ENDPOINT, | ||
| ) |
Contributor
There was a problem hiding this comment.
I still think I'm more supportive of providing a config option that's explicitly passed in rather than deriving one from the environment (it just seems a bit less explicit), but I understand why we're doing it this way. Not blocking, just wanted to reiterate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Python SDK's local evaluation endpoint (
/api/feature_flag/local_evaluation/) is hardcoded, making it impossible to redirect traffic to the new Rust-powered/flags/definitionsendpoint without modifying the SDK source code. This blocks the gradual migration of local evaluation from Django to Rust.Changes
POSTHOG_LOCAL_EVALUATION_ENDPOINTenvironment variable support inclient.py. When set, the SDK uses the specified endpoint for fetching flag definitions instead of the default Django one./api/feature_flag/local_evaluation/endpoint with a warning log, ensuring zero downtime during the migration.test_feature_flags.pyverifying: