Repository navigation
feat(flags): report flags that local evaluation cannot resolve - #237
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 6901e84: 4 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
posthog-elixir Compliance ReportDate: 2026-11-03T23:00:26.102695+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 610ms |
| Format Validation.Event Has Uuid | ✅ | 611ms |
| Format Validation.Event Has Lib Properties | ✅ | 610ms |
| Format Validation.Distinct Id Is String | ✅ | 610ms |
| Format Validation.Token Is Present | ✅ | 610ms |
| Format Validation.Custom Properties Preserved | ✅ | 610ms |
| Format Validation.Event Has Timestamp | ✅ | 610ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ❌ | 610ms |
| Retry Behavior.Retries On 503 | ✅ | 5614ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2612ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2612ms |
| Retry Behavior.Respects Retry After Header | ✅ | 5616ms |
| Retry Behavior.Implements Backoff | ✅ | 15625ms |
| Retry Behavior.Retries On 500 | ✅ | 5616ms |
| Retry Behavior.Retries On 502 | ✅ | 5616ms |
| Retry Behavior.Retries On 504 | ✅ | 5616ms |
| Retry Behavior.Max Retries Respected | ✅ | 15626ms |
| Deduplication.Generates Unique Uuids | ✅ | 621ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 5614ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10621ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 5618ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 616ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 613ms |
| Compression.Sends Gzip When Enabled | ✅ | 610ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 609ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 607ms |
| Batch Format.Multiple Events Batched Together | ✅ | 615ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2612ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2612ms |
| Error Handling.Retries On 408 | ✅ | 5614ms |
Failures
format_validation.non_utc_event_timestamp_is_converted_to_utc
Event 0 field 'timestamp' instant '2026-11-03T22:58:42.027222Z' != expected '2025-01-02T03:04:05Z'
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 9ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 7ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 7ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 6ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 7ms |
| Request Payload.Groups Round Trip | ✅ | 7ms |
| Request Payload.Groups Default To Empty Object | ❌ | 7ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 7ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 7ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 7ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 4ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 609ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 11ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 7ms |
| Retry Behavior.Retries Flags On 502 | ❌ | 310ms |
| Retry Behavior.Retries Flags On 504 | ❌ | 312ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 610ms |
Failures
request_payload.groups_default_to_empty_object
Field 'groups' not found in /flags request body at path 'groups'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']
request_payload.disable_geoip_omitted_defaults_to_false
Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']
request_lifecycle.mock_response_value_is_returned_to_caller
Last action result missing field 'value'. Keys: ['error', 'success']
retry_behavior.retries_flags_on_502
Last action result missing field 'value'. Keys: ['error', 'success']
retry_behavior.retries_flags_on_504
Last action result missing field 'value'. Keys: ['error', 'success']
side_effect_events.get_feature_flag_captures_feature_flag_called_event
Expected 1 events with name '$feature_flag_called', got 0
dustinbyrne
left a comment
There was a problem hiding this comment.
The snapshot, fallback and warning changes look good. Two non-blocking reason-classification improvements remain: definition-only equality limitations and regex input-only limitations. Neither changes the existing conservative flag-value behavior.
AI-assisted review.
| operator when operator in ["exact", "is_not", "is_set", "is_not_set"] -> | ||
| true |
There was a problem hiding this comment.
F1 — [P2] Check equality operands before blaming caller context
MINOR · OPTIONAL · introduced diagnostic gap
Location: lib/posthog/feature_flags/local_evaluator.ex:879–880, RIGHT.
An exact or is_not filter valued %{"n" => 0.1} remains unresolved but reports :missing_context. Supplying more context cannot fix this: the evaluator rejects the filter’s composite floating-point serialization itself. It should report :unsupported_definition. Composite Sigma values such as %{"name" => "ΟΣ"} have the same issue.
The unconditional equality-support branch causes both the incorrect reason and an empty definition_reasons/1 map, preventing the loader’s definition warning. Backend validators permit these JSON equality values; this is not an invalid Elixir-only struct.
For a loaded checkout flag with that equality filter on profile:
{:ok, snapshot} =
PostHog.FeatureFlags.evaluate_flags(%{
distinct_id: "user",
only_evaluate_locally: true,
person_properties: %{"profile" => "anything"}
})
snapshot.unresolved_flags
# Actual: %{"checkout" => :missing_context}
# Expected: %{"checkout" => :unsupported_definition}The pinned contract distinguishes causes by whether changing the definition or supplying usable context can repair them. sort_json_objects/1 rejects composite floats; case_insensitive_equal?/2 rejects composite Sigma casing.
Executed regression: float-entry.exs head produced two failures, one per operator, specifically expected unsupported_definition versus actual missing_context. Base controls passed; both revisions omit the value. Scalar float comparisons passed (exact: true, is_not: false). Sigma evaluator/public-entry regressions confirmed the same gap.
Equivalent project-test regression using existing helpers:
for operator <- ["exact", "is_not"] do
property = %{
"key" => "profile", "type" => "person",
"operator" => operator, "value" => %{"n" => 0.1}
}
definitions = snapshot([flag("checkout", [property])])
local = LocalEvaluator.evaluate(definitions, %{
distinct_id: "user",
person_properties: %{"profile" => "anything"}
})
assert local.results == %{}
assert local.reasons == %{"checkout" => :unsupported_definition}
assert LocalEvaluator.definition_reasons(definitions) ==
%{"checkout" => :unsupported_definition}
endThis snippet was not installed into the project test file; equivalent vectors ran against exact copied sources. The reason assertion fails on head. The empty definition-reason map was separately observed; actual warning delivery was not executed.
Consider classifying definition-only equality limitations using the existing serialization/casing rules. Preserve scalar comparisons, property-only missing-context classification and lazy candidate-list matching; broadening equality evaluation is unnecessary.
This affects remediation guidance, not flag values, fallback safety or application stability. Existing discussion: r4447864513; the float and executed public-path evidence supplement that report.
There was a problem hiding this comment.
Fixed in 9520b65. filter_supported?/3 now runs each exact/is_not candidate through the evaluator's own prepare_exact_comparison/1. A composite value with a float or with Sigma now reports unsupported_definition and shows up in definition_reasons/1, so the load warning fires. Scalar candidates and missing properties keep their behavior. The evaluator table has rows for both operators with %{"n" => 0.1}, the ΟΣ case and a scalar control.
| :inconclusive when operator in ["regex", "not_regex"] -> | ||
| {:inconclusive, :unsupported_definition} |
There was a problem hiding this comment.
F2 — [P2] Distinguish unusable regex context from unsupported patterns
MINOR · OPTIONAL · introduced diagnostic mismatch
Location: lib/posthog/feature_flags/local_evaluator.ex:852–853, RIGHT.
A valid ^a$ filter resolves locally for ASCII inputs but reports :unsupported_definition when caller property profile is "é". The definition passes the support scan; it is the supplied input that ascii_regex_input?/1 cannot handle. The reason should identify unusable context rather than direct the caller to repair the pattern.
With that loaded filter:
PostHog.FeatureFlags.evaluate_flags(%{
distinct_id: "user",
only_evaluate_locally: true,
person_properties: %{"profile" => "é"}
})
# Actual unresolved reason: :unsupported_definition
# Contract reason: :missing_contextThe same pinned contract explicitly includes context supplied in a form the evaluator cannot use. This branch maps every regex inconclusive outcome to a definition problem.
Executed base/head controls:
| Pattern / input | Base | Head |
|---|---|---|
^a$ / "a" |
true | true |
^a$ / "b" |
false | false |
^a$ / "é" |
unresolved | unresolved, unsupported_definition; no definition reason |
(?=a) / "a" |
unresolved | unresolved, unsupported_definition; definition reason present |
additional-vectors.exs head contract fails the expected missing_context assertion for the stated reason. Base has no reasons API; fallback and values are unchanged. These regex controls execute the evaluator directly; public snapshot propagation is source-traced, not separately executed for regex.
Equivalent project-test regression using existing helpers:
property = %{
"key" => "profile", "type" => "person",
"operator" => "regex", "value" => "^a$"
}
definitions = snapshot([flag("checkout", [property])])
local = LocalEvaluator.evaluate(definitions, %{
distinct_id: "user",
person_properties: %{"profile" => "é"}
})
assert local.results == %{}
assert local.reasons == %{"checkout" => :missing_context}
assert LocalEvaluator.definition_reasons(definitions) == %{}This snippet was not installed into the project test file; its equivalent reason assertion and the controls above ran in the isolated harness.
The author’s peer-consistency reply is relevant but not a specification exemption. Python/Ruby use broad unsupported labels for some portability failures, but source inspection indicates they would resolve this exact vector as false; that peer result was not executed.
Consider distinguishing input rejection from unsupported-pattern outcomes and aligning the reason to the contract. If the broad label is intentional, document its consequences and resolve it as a cross-SDK policy with equivalent vectors. This does not require broadening Unicode regex evaluation here.
Existing discussion: r4447416873. The controlled input/pattern comparison supplements that report.
There was a problem hiding this comment.
Fixed in 9520b65. A supported pattern whose value the evaluator rejects as non-ASCII now reports missing_context. A pattern only the server can run, such as (?=a), stays unsupported_definition. Values and the fallback are unchanged. Both cases are rows in the evaluator table.
… and malformed cohort references
💡 Motivation and Context
Applies PostHog/sdk-specs#85 to the Elixir SDK. A caller of
evaluate_flags/2can't tell a flag that local evaluation could not resolve from a flag that doesn't exist, and$feature_flag_calledreports both asflag_missing. The other server SDKs already ship this: PostHog/posthog-python#1133, PostHog/posthog-ruby#319, PostHog/posthog-php#281, PostHog/posthog-go#396, PostHog/posthog-dotnet#357, PostHog/posthog-android#896 and posthog-node (PostHog/posthog-js#5234, PostHog/posthog-js#5471).Changes:
PostHog.FeatureFlags.Evaluationshas a newunresolved_flagsfield. It maps each flag that local evaluation could not resolve, and that no/flagsfallback returned, to a reason::experience_continuity,:unsupported_definition,:unresolved_dependencyor:missing_context. When several causes apply, the spec's precedence picks one. Filtered snapshots have no entries.local_evaluation_inconclusiveinstead offlag_missing./flags. Before, a later condition could resolve it locally to a different variant than the server returns.Differences from the spec or the other SDKs:
/flagsfallback fails,evaluate_flags/2still returns{:error, reason}, so there is no snapshot that can listunresolved_flags. This keeps the existing error contract. The spec scenario for a failed fallback is therefore only met when at least one flag resolved.:missing_contexteven when a dependency would also be unresolved. Node can report:unresolved_dependencythere. Only the label differs, never the value.💚 How did you test it?
local_evaluation_inconclusiveread event, and the fallback outcomes: a returned value, a returnedfailed: truerecord (kept asfalseand not reported), no flags, and HTTP 503. They also cover the variant regression for a server-only dependency through scoped, unscoped andcheck/3calls.mix format --check-formatted,mix compile --warnings-as-errors,mix credo --strict,mix posthog.public_api --checkandmix test(866 passed) pass locally. No manual testing in an app.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
failedrecords, andmissing_contextfor a missing group key.:unsupported_definition.{:error, reason}contract for a failed fallback with nothing resolved locally, instead of returning a snapshot, to avoid a public API change.