feat(server): expose flags that local evaluation could not resolve - #827
gustavohstrassburger wants to merge 1 commit into
Conversation
|
[Medium risk] Adds unresolved flag tracking to local evaluation results. The PR appears safe to merge, with non-blocking documentation and warning-deduplication issues to address. Reviews (1) · Last reviewed commit: "feat(server): expose flags that local ev..." |
| /** | ||
| * The call did not supply a property, group key or group property the flag's conditions need, | ||
| * or supplied it in a form the evaluator cannot use. Pass it. | ||
| */ |
There was a problem hiding this comment.
Missing group keys are not reported
A group flag evaluated without its group key resolves to false; it does not appear in unresolvedFlags with MISSING_CONTEXT. This documentation tells callers to expect a signal they will not receive. Remove “group key” from the description, or change evaluation to report that case as inconclusive.
| /** | |
| * The call did not supply a property, group key or group property the flag's conditions need, | |
| * or supplied it in a form the evaluator cannot use. Pass it. | |
| */ | |
| /** | |
| * The call did not supply a property or group property the flag's conditions need, | |
| * or supplied it in a form the evaluator cannot use. Pass it. | |
| */ |
Knowledge Base Used: Server feature-flag evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog-server/src/main/java/com/posthog/server/PostHogUnresolvedFlagReason.kt
Line: 19-22
Comment:
**Missing group keys are not reported**
A group flag evaluated without its group key resolves to `false`; it does not appear in `unresolvedFlags` with `MISSING_CONTEXT`. This documentation tells callers to expect a signal they will not receive. Remove “group key” from the description, or change evaluation to report that case as inconclusive.
```suggestion
/**
* The call did not supply a property or group property the flag's conditions need,
* or supplied it in a form the evaluator cannot use. Pass it.
*/
```
**Knowledge Base Used:** [Server feature-flag evaluation](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-android/-/docs/server-feature-flag-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| private fun recordUnresolvableFlagsLocked(flags: List<FlagDefinition>): List<String> { | ||
| val unresolvable = flags.filter { it.active && it.ensureExperienceContinuity } | ||
| val keys = unresolvable.mapTo(HashSet()) { it.key } | ||
| warnedUnresolvableFlagVersions.keys.retainAll(keys) |
There was a problem hiding this comment.
Warning history is discarded
If an active experience-continuity flag is inactive or absent for one definitions load, retainAll deletes its warning history. When the flag returns at the same version, the SDK warns again, despite promising one warning per flag version. Preserve that version history across intervening loads, or narrow the promise.
Knowledge Base Used: Server feature-flag evaluation
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt
Line: 813
Comment:
**Warning history is discarded**
If an active experience-continuity flag is inactive or absent for one definitions load, `retainAll` deletes its warning history. When the flag returns at the same version, the SDK warns again, despite promising one warning per flag version. Preserve that version history across intervening loads, or narrow the promise.
**Knowledge Base Used:** [Server feature-flag evaluation](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-android/-/docs/server-feature-flag-evaluation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
💡 Motivation and Context
With
onlyEvaluateLocally, a flag that local evaluation cannot resolve is dropped from theevaluateFlags()snapshot without any signal, and reading it reportsflag_missing. The most common case is a flag with experience continuity, which/local_evaluationreturns but the evaluator never resolves. Callers cannot tell it apart from a flag that does not exist.Implements the "Snapshot unresolved flag reporting" requirement from PostHog/sdk-specs#85:
PostHogFeatureFlagEvaluations.unresolvedFlagslists flags with a local definition that have no value, each with aPostHogUnresolvedFlagReason(EXPERIENCE_CONTINUITY,UNSUPPORTED_DEFINITION,MISSING_CONTEXT,UNRESOLVED_DEPENDENCY). A flag filled by the/flagsfallback is not listed; one the fallback failed to fill or omitted is.$feature_flag_error: local_evaluation_inconclusiveinstead offlag_missing.InconclusiveMatchExceptionnow carries a reason, chosen by whether the failing value came from the flag definition or from the caller.Accessors,
keys, capture enrichment and filtered snapshots are unchanged.No divergence from the spec. This SDK has no device-id bucketing, so the spec's missing-device-id case does not apply here.
💚 How did you test it?
New unit and integration tests cover each reason, the definition-vs-caller split, local-only and fallback paths, scoping, inactive flags, the event error and the load-time warning.
./gradlew :posthog-server:testpasses, along withmake checkFormatandapiCheck.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file