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}
| this.eventsUri = builder.eventsUri; | ||
|
|
||
| if (Boolean.TRUE.equals(builder.enableEvents)) { | ||
| eventProcessor = builder.eventProcessor != null |
There was a problem hiding this comment.
🔴 Blocker · 🏗️ Heavy lift
Keep event delivery scoped to one client.
Observed: FlagsmithConfig retains a single EventProcessor, and each client build later replaces its API wrapper. Predicted: two clients built from the same enabled config with different keys would send the first client's buffered identities and metadata under the second client's environment key; closing either client would also stop the shared sender. Create the processor per client (or make an enabled config single-use) so its API wrapper and lifecycle cannot be reassigned.
⚖️ 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 |
| */ | ||
| private static Retry buildRetry() { | ||
| Retry retry = new Retry(2); | ||
| retry.setStatusForcelist(new HashSet<>(Arrays.asList(500, 502, 503, 504))); |
There was a problem hiding this comment.
🟠 Major · ⚡ Quick win
Retry every server error promised by the event pipeline.
Observed: the force-list contains only 500, 502, 503, and 504 although this processor says it retries 5xx responses. Predicted: a 501, 505, 507, 508, 510, or 511 from the events endpoint or an intermediary would drop the batch after one request. Populate the policy with the full 500–599 range (or use a 5xx predicate) and cover a non-listed 5xx in the retry test.
⚖️ 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".
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
1 participant
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. In-flight events are capped at 10 000 so an outage cannot grow memory unboundedly.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.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: 477 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()).