Conversation
Parse experiment metadata from remote evaluation and add an opt-in event
processor so SDK users can resolve experiment flags and record exposures.
- Flag and FeatureStateModel now carry variant, reason and experiment
(metadata.experiment), populated by remote evaluation only. Local
evaluation sets reason from FlagResult; variant and experiment stay null
because the environment document has no variant keys.
- New EventProcessor buffers events and POSTs {"events": [...]} to
{eventsUri}v1/events, flushing on a 10s timer, at 1000 buffered events
and on close(). Exposures are deduplicated per flush window; a failed
batch is retried once on a connection error or 5xx, never on 4xx, then
dropped. Nothing thrown inside it reaches caller code.
- New client methods: getExperimentFlag, trackEvent, trackExposureEvent
and flushEvents. close() now also closes the event processor.
- Opt in with FlagsmithConfig.Builder.withEnableEvents(true). Configuring
the buffer, interval or events URI without enabling events is rejected
at build time, as is enabling events in offline mode.
- Retry gains an opt-in statusForcelistOnly flag so a force-listed status
respects the attempts budget instead of retrying forever. The default
stays false, preserving existing behaviour.
- RequestProcessor gains submit(), returning a CompletableFuture so the
event processor can compose on batch completion.
Nothing changes for users who do not opt in.
Three defects found in adversarial review of the event processor.
flush() registered a batch as in-flight only after serialising it and
building the request, both outside the buffer lock. A concurrent flush()
could observe an empty buffer and an in-flight set that did not yet
contain the batch, and return an already-completed future. The batch is
now created and added to inFlight inside the same synchronized block
that empties the buffer.
send() added the tracking future to inFlight before submitting. If
submit threw - RejectedExecutionException once the request processor is
closed, or anything out of newPostRequest - the future was left pending
forever, wedging every later flush() and burning the full close()
timeout. send() now settles it in a finally block on every path, and
buffering is a no-op once the processor is closed.
Traits were put on the wire verbatim, so a TraitConfig value serialised
as {"value":..,"isTransient":..} instead of the flat map the events API
expects, and a trait the caller marked transient was shipped to the
event store. Values are now unwrapped through TraitConfig, transient
traits are dropped, and the map is copied at buffer time so a caller
mutating it cannot change a buffered event.
Also covers the retry paths that had no tests: connection failures, and
Retry.isRetry under statusForcelistOnly, whose attempts-budget branch is
what stops a permanently failing endpoint from retrying forever. The
timer flush test now waits on a latch instead of sleeping.
eventsUri() marked the events config as touched, so setting a custom events host threw at build() unless that same config also enabled events. That blocked the ordinary case of a shared configuration carrying the URL while only some services opt in. The spec only requires the buffer size and flush interval to be gated, which they still are.
The class-level @Getter made buffer, lock, dedupeKeys, scheduler, inFlight, requestProcessor, logger and api public getters. Once released, every one of them is API the SDK has to keep; the buffer getter also handed out a list guarded by a private lock. Only the four immutable settings stay public. The test constructor and the scheduler/request processor accessors become package-private, and tests read the buffer through a snapshot taken under the lock.
Traits and metadata are arbitrary caller objects, and were only serialised when the whole batch was. A single value Jackson cannot handle (a java.time type, a bean without properties) failed that serialisation and dropped every event in the batch, up to 1000. Converting traits and metadata to JSON trees at buffer time drops and logs only the offending event. It also deep-copies them, where the previous copy was shallow and a caller mutating a nested map could still change an event already buffered.
While the events API is slow or down, each batch can hold a request thread for two timeouts plus backoff, and the request processor's queue is unbounded. Traffic kept producing batches faster than they were given up on, so an outage grew memory with the host app's load. A flush now drops its batch, with an error log, once ten batches are already waiting. The API answers 202 even when it rejects some events, listing them under 'rejected'. Those were discarded unread; the count and the first rejection are now logged.
- An event whose closed-check ran before close() could still land in the buffer after close()'s final flush and be lost unlogged. The check is repeated under the buffer lock, which the final flush takes after the flag is set. - start() on a closed processor threw RejectedExecutionException out of FlagsmithClient.Builder.build(), which happens when a FlagsmithConfig is reused after closing a client built from it. It now logs and does nothing. - build() started the flush timer before its local-evaluation checks, so a build that then failed left the timer running with no client to close it. The processor is now wired last; the offline-mode check moves up with the other offline checks.
- trackEvent buffered a null or blank event name, which the events API rejects; it now throws IllegalArgumentException, like the reserved '$' prefix already did. trackExposureEvent does the same for a blank feature name. A blank identifier is still logged and skipped, since an anonymous visitor is an ordinary runtime case, not a caller bug. - withEventsMaxBufferItems(0) switched off the size trigger, and with the timer also off the buffer grew without bound. build() now rejects a limit below 1 and a negative flush interval. - withEnableEvents(null) threw a NullPointerException from build(); it now leaves events disabled.
…mentFlag FlagsmithApiWrapper.identifyUserWithTraits returns null, rather than throwing, when the identities request times out or is interrupted. getExperimentFlag dereferenced that null, so an API slower than the 15s future timeout surfaced as a NullPointerException and bypassed the default flag handler. A null result now returns the default handler's flag, with no exposure recorded, and throws FlagsmithApiError when no handler is configured.
Capping in-flight batches over a fixed three-thread pool made the limit a throughput ceiling of about 3 x maxBufferItems per round trip, which throttled hardest the smaller the configured buffer: a healthy API dropped most events at a small buffer size, and even at defaults a burst dropped two thirds. The cap now counts events (10,000, ten default batches), tracked under the buffer lock and given back when a batch settles. The memory bound at defaults is unchanged, and throughput no longer depends on buffer size. Drops are reported at once, then at most every ten seconds with the count accumulated in between, so a saturated caller cannot emit an error line per flush. The completion callback also captured the whole batch list just for its size, keeping a second copy of every in-flight batch alive; it now captures the int.
close() waited requestTimeoutMillis x 2, and FlagsmithConfig always passed the SDK's default read timeout, ignoring the one the caller configured. Even at defaults the wait was 10s against a worst case of about 24s for one batch (connect + write + read, twice, plus backoff), so the final batch was routinely abandoned during an outage. The processor now derives the wait from its HTTP client: the call timeout when set, otherwise connect + write + read, for every attempt the retry policy allows, plus the backoff between them. With a timeout switched off nothing bounds a request, and close() waits as long as it does. The unreleased requestTimeoutMillis constructor parameter and getter go, since the client already carries the timeouts. The request processor is still shut down, not interrupted: interrupting a POST loses its batch, where letting it finish delivers it.
The re-check that stops an event racing close() from being stranded in the buffer had no test. A trait whose getter blocks parks the tracking thread between the first check and the lock while close() runs its final flush; removing the re-check makes it fail. FlagsmithClientTest.testCloseDoesNotWedgeLaterFlushes buffered nothing once tracking after close became a no-op, so it could not fail. The paths it meant to cover are pinned in EventProcessorTest by flush_completesWhenTheRequestProcessorIsAlreadyShutDown and trackEvent_isANoOpAfterClose.
… Error Serialising traits and metadata with valueToTree at buffer time let a map or list that contains itself throw a raw StackOverflowError out of trackEvent. The older flush-time serialisation had reported the same input as a JsonMappingException, so this was a regression from moving serialisation earlier. Each event's traits and metadata are now written with writeValueAsString, which reports every cycle as a checked JsonMappingException, and are held as RawValue that Jackson emits verbatim in the batch. The offending event is dropped and logged; the rest are unaffected. Nothing catches Error, and the class Javadoc now says so. Buffered JSON text is also more compact to hold than a tree.
Zaimwa9
force-pushed
the
feat/experimentation-support
branch
from
September 23, 2026 15:32
054ef64 to
d4e6185
Compare
Contributor
Author
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🔴 Hold the mergeThe event pipeline is well-covered for a single client, but its sender is shared through a reusable configuration and gets rebound during later client builds. This can send one environment's identity event data with another environment's key. All 12 completed Java/OkHttp CI checks passed.
🔴 Blockers
📝 Walkthrough
🧪 How to verify
Automate: add a two-client shared-configuration regression test that asserts distinct event processors and request headers. Product take: Experiment exposure data is useful product telemetry, but misattributing it across environments corrupts results and leaks identity metadata. This is a major capability once its ownership is made client-local. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The event buffer needs its own seat at the client table · reviewed at d4e6185 |
FlagsmithConfig built and held the EventProcessor, so clients sharing one config shared a processor: the last build rebound its API key, closing either client stopped events for both, and a custom API wrapper with its own config left the processor in use unstarted. The config now carries only the event settings; FlagsmithClient.build() creates, binds and starts a processor per client from the builder's configuration. An injected processor is still used as given.
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🔴 Hold the mergeThe default event processor is now isolated per client, but a configured custom processor is still rebound to whichever client was built last and then stopped when either client closes. The new retry policy also drops batches immediately for several 5xx responses despite promising a retry for server errors. All 12 completed test-matrix jobs passed.
🔴 Blockers
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: Experiment exposure data is a meaningful new capability, but incorrect environment attribution or avoidable event loss makes this unsafe to ship as-is. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The event queue is nearly ready for its close-up; it just needs to remember which client hired it · reviewed at 05d2fc2 |
A processor passed to withEventProcessor was rebound by every build(), so two clients from one config shared it: events went under the last client's key and closing either stopped it for both. build() now claims the processor once, atomically, and a second build throws. Also name the exact retried statuses (500, 502, 503, 504) in the retry policy's Javadoc instead of "a 5xx".
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe experimentation pipeline is well covered across its public APIs and lifecycle, and all completed CI jobs passed. Two delivery defects remain: the advertised 10,000-event in-flight limit can be exceeded, and several valid 5xx responses are dropped without the promised retry. Local focused tests could not be run because Maven is unavailable in this environment.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: Experiment exposure tracking is a meaningful capability, but its reliability guarantees are part of experiment quality. These two fixes matter before users rely on the resulting metrics. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The event buffer has excellent manners, but it needs to count its guests before opening the door · reviewed at 892e05c |
The in-flight guard only rejected a batch once the cap was already reached, so a batch could push the total past it. A flush now drops its batch when adding it would exceed the cap, but always sends when nothing is in flight so a buffer larger than the cap still gets through. Event batches now retry once on any 5xx, not only 500, 502, 503 and 504. The attempt count, and so the close() bound, is unchanged.
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe event pipeline lets a caller disable timed flushing and configure an unbounded threshold, allowing the in-memory event buffer to grow without its 10,000-event ceiling. Enrolled identities served a disabled experiment variant also still miss their exposure event, which would skew experiment reporting. The test matrix is still running.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: add the no-timer oversized-buffer and disabled enrolled-variant regressions. Product take: Experiment tracking expands measurement capability, but these gaps can corrupt experiment denominators and make a misconfigured high-volume client unsafe to run. 🧭 Assumptions & unverified claims
A promising experiment needs a denominator and a memory ceiling · reviewed at deea76e |
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR:
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a meaningful experimentation capability, but server-side callers need 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One last batch is still slipping past the finish line · reviewed at deea76e |
When the in-flight limit is reached, flush() now also waits for the batch the current buffer will become, so its future completes once every event buffered before the call has been sent or dropped. Later batches are not awaited, as in the Node SDK.
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe event pipeline is close, but it still permits more than the advertised 10,000 events to be retained and submitted during an outage. The recorded test matrix was still running, with no completed failures available.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Experiment reporting is a solid capability addition, but its availability safeguards need to match the advertised bound before relying on it in high-volume services. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The events queue has learned patience; it still needs to learn arithmetic. · reviewed at 7baec55 |
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The experiment event pipeline is well covered by the completed CI matrix, but a custom
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: add the distinct custom Product take: Solid experimentation support, including reliable background delivery and explicit opt-in behaviour. Correct event attribution matters directly to experiment results, so the remaining header override is material. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One header is still trying to be more helpful than invited · reviewed at 7baec55 |
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The event pipeline is well covered for normal batching and shutdown paths, but rebuilding an events-enabled client from the same builder abandons a live processor. CI is still running across the supported Java and OkHttp combinations.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: add the builder-reuse and oversized-batch regressions to the event processor suite. Product take: Experiment exposure reporting is a meaningful capability, but lifecycle leaks or unbounded event batches can distort reporting and keep services alive unexpectedly. Fixing the two lifecycle/back-pressure edges is proportionate before enabling it broadly. Big picture: Event processing is now owned by 🧭 Assumptions & unverified claims
A very capable event courier; it just needs one client owner at a time. · reviewed at 45804d2 |
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe opt-in events pipeline is well covered, but two delivery paths need correction before release: recovery can exceed the caller's configured batch size, and delimiter-based exposure de-duplication can discard a distinct exposure. CI is still running; no completed check result is available yet. The focused local test suite could not be run because Maven is not installed in this environment.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Experiment exposure data is central to measuring rollout outcomes, so silent drops or unexpectedly oversized recovery requests materially weaken the new capability. Once corrected, this is a solid addition to the SDK. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The experiments need their receipts, not a game of delimiter hide-and-seek · reviewed at ddab0cf |
The events API rejects bodies over 2 MB (axum's default limit) with a 413, which is never retried, so a 10,000-event catch-up batch would be lost whole. With the default maxBufferItems, a catch-up batch is now no larger than a normal one.
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe events backpressure path can suppress a later exposure after the original exposure has already been evicted, leaving experiment reporting without either event. CI is still running; the completed PR-title check is neutral. Local test execution was unavailable because Maven and a JDK are not installed.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: Solid experimentation support, but this edge case can undercount exposures precisely during event-service backpressure, where preserving a later valid exposure matters most. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. One last queue gremlin remains in the exposure buffer. · reviewed at 960609a |
Contributor
Author
|
@themis-blindfold review |
⚖️ Themis review: ✅ Ship itThe events pipeline is scoped per client, keeps Flags API-only headers off the events request, and covers the important buffering, retry, shutdown, and experiment-exposure paths. CI is still running; the local Maven command is unavailable in this environment.
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: keep the events lifecycle and both OkHttp profiles in the required test matrix. Product take: A solid SDK capability: experiment exposure and custom-event delivery become available without changing the existing default behaviour. The opt-in design and focused lifecycle safeguards make this meaningful for experimentation users. 🧭 Assumptions & unverified claims
Events now know when to leave the party · reviewed at 02c8463 |
Member
|
@coderabbitai review |
gagantrivedi
approved these changes
Sep 30, 2026
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
2 participants
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.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Experimentation support, opt-in via
FlagsmithConfig.Builder.withEnableEvents(true). Nothing changes for existing users.Flaggainsvariant,reasonandexperiment { id, name, inExperiment }from remote identity evaluation. Local evaluation now setsreasonfrom the engine.EventProcessor: buffers events, dedupes exposures per flush window, POSTs{ "events": [...] }to{eventsUri}v1/eventsevery 10 s or at 1000 events. Retries a failed batch once on a connection error or 5xx, never on 4xx, then drops it; never re-queues. Transient traits are not sent.maxBufferItemsis also the batch size, as in the Node SDK. In-flight batches are capped at 2; while both are in flight, events keep buffering up to 1 000 (ormaxBufferItemsif larger) and go out as one batch when a slot frees, dropping the oldest beyond that. The 1 000 bound keeps a catch-up batch no larger than a default batch, well under the events API's 2 MB body limit (a 413 is not retried).FlagsmithClientmethods:getExperimentFlag,trackEvent,trackExposureEvent,flushEvents.close()now flushes, bounded by the client's configured timeouts.Retrygains an opt-instatusForcelistOnly: the existingisRetryretries a force-listed status regardless of the attempts budget, which would loop forever on a persistent 5xx. Default unchanged.RequestProcessorgainssubmit()returning aCompletableFuture.Spec differences:
close()derives its bound from the HTTP client's timeouts and the retry policy instead of arequestTimeoutMillisparameter. Retries cover the whole 5xx range.Docs live at docs.flagsmith.com; the README defers to them and needs no change.
How did you test this code?
mvn clean installandmvn clean install -P test-okhttp4: 479 passed each, 0 checkstyle violations. Engine conformance suite untouched and green.EventProcessorTest(buffering, dedupe matrix, headers/body, max-buffer flush, cross-thread flush completion, retry-then-drop for 5xx/4xx/connection failure, in-flight cap, per-event serialisation failures, close/start races, timer),FeatureStateModelTest,FlagsmithRetryTestadditions,FlagsmithClientTestadditions (fullgetExperimentFlaggate matrix, identity-API timeout, local evaluation,close()).