feat(server): filter the evaluateFlags snapshot by evaluation runtime - #805
Conversation
 The implementation appears behaviorally sound, but the repository’s required public-API agreement must be completed before merging. Reviews (1) · Last reviewed commit: "feat(server): expose flag evaluation run..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public fun getEvaluationRuntime(key: String): String? { | ||
| return flagMap[key]?.metadata?.evaluationRuntime | ||
| } |
There was a problem hiding this comment.
This adds the public getEvaluationRuntime API even though the PR states that its shape has not been discussed on an issue. The repository requires public API shapes to be agreed on an issue first, so that requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog-server/src/main/java/com/posthog/server/PostHogFeatureFlagEvaluations.kt
Line: 121-123
Comment:
**Public API lacks agreement**
This adds the public `getEvaluationRuntime` API even though the PR states that its shape has not been discussed on an issue. The repository requires public API shapes to be agreed on an issue first, so that requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/posthog/posthog-android/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
@gustavohstrassburger make sure to document this new API in https://github.com/PostHog/sdk-specs
happy to review!
posthog-android Compliance ReportDate: 2026-09-28 17:10:09 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
node added that recently PostHog/posthog-js#4885 |
|
suggestion: Consider preserving |
No, it's a different thing. The only overlap is the #4885 is about the This PR is about the Different endpoint, different direction, different problem. |
|
Done in 299ade7. A flag that falls back to |
|
@marandaneto heads-up since your approval predates it: d5f3035 adds |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Nice addition! One blocking issue inline, plus a few suggestions.
| continue | ||
| } | ||
| val runtimes = filter.evaluationRuntimes | ||
| if (runtimes != null && flag.metadata.evaluationRuntime !in runtimes) continue |
There was a problem hiding this comment.
blocking: only(filter) throws a NullPointerException when evaluationRuntimes is a JDK immutable collection and any flag's runtime is null. With a null runtime, flag.metadata.evaluationRuntime !in runtimes calls Collection.contains(null), and Set.of, List.of, and Set.copyOf throw instead of returning false. So new PostHogFeatureFlagFilter(null, Set.of("client", "all")) fails on the first flag without a local definition, or on every flag when local evaluation is off. The tests pass because Kotlin's setOf and the builder's list both return false for contains(null).
Check for null before the lookup, and add a test that filters with java.util.Set.of("client") over a flag whose runtime is null:
| if (runtimes != null && flag.metadata.evaluationRuntime !in runtimes) continue | |
| val runtime = flag.metadata.evaluationRuntime | |
| if (runtimes != null && (runtime == null || runtime !in runtimes)) continue |
There was a problem hiding this comment.
Good catch, thanks. only(filter) now reads the runtime into a local and skips the flag when it is null before asking the collection. Covered by only with a runtime filter never asks the collection about a null runtime. It uses a java.util.TreeSet rather than Set.of, because animal sniffer checks the test sources against the Java 8 signature and Set.of is Java 9; a TreeSet throws the same NullPointerException on contains(null).
| val version: Int, | ||
| @SerializedName("has_experiment") | ||
| val hasExperiment: Boolean? = null, | ||
| @SerializedName("evaluation_runtime") |
There was a problem hiding this comment.
suggestion: Mapping evaluation_runtime here makes /flags a second source for the runtime, but /flags never sends it (the FlagDetailsMetadata in the flags service has no such field). The override loop only replaces the value for flags that have a local definition, and it doesn't run when no definitions are loaded. So for a flag without a local definition, the snapshot reports whatever /flags sends. The test at PostHogEvaluateFlagsTest.kt:636 asserts "client" for that case, while the getEvaluationRuntime KDoc, the changeset, and the sdk-specs#74 contract all say it returns null.
Marking the field @Transient (as RRWireframe.kt:29 does) stops Gson from reading it, so local definitions become the only source. Remove the sentence about reclassification from the merge-loop comment in PostHogFeatureFlags.kt, and update the test at line 636 to assert null.
There was a problem hiding this comment.
Done. evaluationRuntime on FeatureFlagMetadata is now @Transient, so Gson never reads it from /flags and the local definition is the only source. The merge comment lost the reclassification sentence, and the test is now an evaluation runtime reported by flags is ignored, asserting null for the flag without a definition and an empty runtime-filtered snapshot.
| // `locally_evaluated_keys`. Note a group flag evaluated without `groups` resolves locally to | ||
| // `false`, and that now beats the server's answer — pass `groups` when gating on one. | ||
| val merged = LinkedHashMap(entry?.flags ?: EMPTY_FLAGS).apply { putAll(localFlags) } | ||
| // The definition in memory is the source of the runtime, even for a flag that fell back to |
There was a problem hiding this comment.
suggestion: This keeps a fallback flag's runtime only for "all" and "server" flags. The flags service classifies the posthog-server/<version> user agent as a server runtime and leaves every "client" flag out of the /flags response. So a "client" flag gated on a person property the caller didn't pass resolves neither locally nor remotely. It never reaches merged, and only(PostHogFeatureFlagFilter(evaluationRuntimes = setOf("client", "all"))) can't return it. The fallback test only uses an "all" flag.
Could you say so in this comment, the getEvaluationRuntime KDoc and the changeset? Sending an explicit evaluation_runtime in the /flags request would bring these flags back, but it changes every /flags call, so that's your call.
There was a problem hiding this comment.
Said so in the helper's KDoc, in the getEvaluationRuntime KDoc and in the changeset: /flags answers this SDK as a server runtime and leaves "client" flags out, so a "client" flag that does not resolve locally is not in the snapshot at all. Sending an explicit evaluation_runtime on the /flags request stays out of this PR, since it changes every call.
| } | ||
|
|
||
| @Test | ||
| fun `getEvaluationRuntime passes through the runtime of locally evaluated flags`() { |
There was a problem hiding this comment.
suggestion: This test doesn't cover the runtime when every flag resolves locally. gated-flag requires email, so evaluateFlags goes through the /flags merge, where the override loop sets every defined flag's runtime from its definition. Deleting evaluationRuntime = flagDef.evaluationRuntime at PostHogFeatureFlags.kt:803 would leave both runtime tests passing. A local-only read catches it:
val localOnly = postHog.evaluateFlags("user-1", onlyEvaluateLocally = true)
assertEquals("client", localOnly.getEvaluationRuntime("client-flag"))There was a problem hiding this comment.
Added a local-only read to the passthrough test (onlyEvaluateLocally = true), asserting "client" and "server" for the two conclusive flags, so removing the runtime from buildFeatureFlagFromResult fails it.
| // `/flags`, so a caller filtering flags to forward does not drop a client flag because one | ||
| // person property was missing. It also wins over anything `/flags` reports for the field, | ||
| // so a response cannot reclassify a server-only flag as client-safe. | ||
| val definitions = flagDefinitions |
There was a problem hiding this comment.
suggestion: evaluateFlags is already the longest function in this file, and this block is a separate step placed inside the local/remote merge. Moving it into a named helper, the way evaluateMissingFlagsRemotely is, keeps the merge readable and gives the rule a name:
private fun applyDefinitionRuntimes(flags: MutableMap<String, FeatureFlag>) {
val definitions = flagDefinitions ?: return
for (slot in flags.entries) {
val runtime = (definitions[slot.key] ?: continue).evaluationRuntime
val flag = slot.value
if (flag.metadata.evaluationRuntime != runtime) {
slot.setValue(flag.copy(metadata = flag.metadata.copy(evaluationRuntime = runtime)))
}
}
}There was a problem hiding this comment.
Extracted as applyDefinitionRuntimes(flags), as suggested, with the rule's comment moved onto it.
| /** | ||
| * Mutable builder for [PostHogFeatureFlagFilter]. | ||
| */ | ||
| public class Builder { |
There was a problem hiding this comment.
suggestion: The only builder test sets both criteria, so the Java form of the browser filter (PostHogFeatureFlagFilter.builder().evaluationRuntimes("client", "all").build()) has no test. Neither does an empty runtime list, which keeps no flags while null keeps them all.
A PostHogFeatureFlagFilterTest modeled on PostHogEvaluateFlagsOptionsTest would cover both.
There was a problem hiding this comment.
Added PostHogFeatureFlagFilterTest with the no-criteria default, the runtimes-only builder (evaluationRuntimes("client", "all") leaving keys null) and accumulation across list and varargs calls. The empty-versus-null runtime criterion is asserted in only with a runtime filter keeps the listed runtimes and drops unknown ones: an empty set keeps nothing, an unset filter keeps everything.
|
|
||
| @Test | ||
| fun `getFlagPayload returns the raw payload string and does not fire an event`() { | ||
| fun `getFlagPayload and getEvaluationRuntime do not fire an event or record access`() { |
There was a problem hiding this comment.
nit: This test reads only known keys, but unknown keys are where isEnabled and getFlag fire flag_missing. Consider adding assertNull(snapshot.getEvaluationRuntime("missing")) before the capture assertion to cover that case too.
There was a problem hiding this comment.
Added assertNull(snapshot.getEvaluationRuntime("missing")) before the capture assertion.
…the runtime surface
gustavohstrassburger
changed the title
3 participants
💡 Motivation and Context
With local evaluation, a server can't tell which flags are safe to forward to a browser client, for example when bootstrapping a client SDK. Each flag's
evaluation_runtime("all","client"or"server") is already in the/local_evaluationpayload, butposthog-serverdrops it.This adds a runtime criterion to the snapshot filter, so the caller picks the flags to forward in one call:
only(PostHogFeatureFlagFilter)filters the snapshot in memory by keys and/or evaluation runtimes, next to the existingonly(keys)andonlyAccessed(). Criteria AND together. Java callers usePostHogFeatureFlagFilter.builder()./flagskeeps the runtime of its local definition. A flag with an unknown runtime (no local definition, or a definition without the field) never matches a runtime criterion, because/flagsdoes not report the runtime and unknown is not client-safe.FlagDefinitionandFeatureFlagMetadata(both@PostHogInternal) gainevaluationRuntime, the same routehasExperimenttook in feat(flags): add $feature_flag_has_experiment to $feature_flag_called events #621. The metadata field is@Transientso a/flagsresponse is never a source for it.An earlier revision also exposed a per-key
getEvaluationRuntime(key)accessor. It was dropped after review on PostHog/sdk-specs#74, where @dustinbyrne asked for one surface and preferredonly(filter)because it extends the existing filter API and does not require iterating keys from the caller's end. PostHog/sdk-specs#77 makes the filter the spec's only runtime surface.Origin: https://us.posthog.com/project/2/support/tickets/74646 (internal support ticket)
💚 How did you test it?
only(filter): the runtime criterion keeps the listed runtimes, drops unknown ones, and combines with keys.PostHogEvaluateFlagsTest: a"client"and a"server"definition, a definition without the field, a flag filled from/flagsthat keeps its local runtime, a flag with no definition, a local-only evaluation, and a/flagsresponse that reports a runtime and is ignored.:posthog-server:test,apiDumpandspotlessApplyrun locally.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
evaluateFlagsoption. It was rejected in favor of a filter on the snapshot, so the server still evaluates once and branches on the full snapshot.pnpm changesetformat.