Add plugins support for both legacy and new clients - #206
Conversation
There was a problem hiding this comment.
Overall
Nice work — the plugin architecture itself is clean: no-op-default interface, a
best-effort PluginRegistry that isolates plugin failures, sensible no-op degradation,
and genuinely good test coverage (real MockWebServer, batch/timer/close/failure paths,
both single- and multi-user wiring). Most of my line comments are quality/consistency
nits. There are, however, three things I'd treat as blockers and a recurring theme worth
addressing in one pass.
Must-fix
-
Data privacy (default attribute export).
snapshotAttributes()ships the full
user attribute map — plushash_valueand the feature/experimentvalue— to the
ingest endpoint (US default host) on every event, opt-out rather than opt-in. For
GDPR users this makes the plugin hard to enable safely. Please make attribute export
opt-in (allowlist, empty by default) with a configurable region/host, and document
that enabling the plugin transfers user data to a third party. -
Remote-eval path is behind
mainand re-fires exposures. The inline
trackData.forEach(...)loop predates #216, which replaced it with
experimentEvaluator.fireRemoteEvaluationTracks(...)(de-dupes per assignment,
null-guards payloads). As written, plugins get duplicateexperiment_viewedon cached
remote-eval responses, unlike the localisExperimentTrackedpath. Rebase and route
plugin exposure through that shared, de-duplicated path. -
Memory/latency under load. The owned flush executor uses an unbounded queue, and
feature_evaluatedfires on every evaluation, so a slow/failing endpoint lets batches
pile up unbounded. On top of that,snapshotAttributes()does a fulldeepCopy()on
the evaluation thread and every buffered event retains its own copy. Needs a bounded
queue + drop policy, and the attribute capture off the hot path (an allowlist also
shrinks this a lot).
Recurring theme — keep the evaluators small, catch narrowly
- The plugin hooks are inlined into the already-huge
evaluateExperiment/
evaluateFeature, even though this PR already introduceddispatchFeatureUsage(...)
in the same file. Add adispatchExperimentViewed(...)helper and route all three
sites through helpers so the methods shrink rather than grow — and trim the redundant
"what" comments while you're touching those blocks. catch (Throwable)is used broadly (plugin flush,submitFlush,TrackingEvent.toJson,
PluginRegistry). Several are redundant (flushBatchalready swallows its
IOException), andThrowablehidesErrors/bugs. Narrow toException, handle each
failure once, and log rather than silently dropping.
Lifecycle / API
init()does nothing; all setup (threads, http client) is in the constructor, so a
disabled or never-registered plugin still spins up executors. Move setup intoinit()
and keep the constructor side-effect-free.close()bundles ~5 concerns — extractfinalFlush()/shutdownScheduler()/…. On the
GrowthBookside, the newclose()should probably implementAutoCloseable, and the
if (pluginRegistry != null)guards on afinalfield are dead.TrackingPluginConfigexposes both raw nullable getters andresolved*()— easy to
grab the wrong one; hide the raw ones.batchSizehas no upper clamp.
Nits
ValueType vs idiomatic <V>; hand-rolled Builder vs the Lombok @Builder used
elsewhere; eventType String vs enum; stripTrailingSlash/isEmpty duplicating
StringUtils/RemoteEvalRequestBuilder.normalizeUrl; // --- internal --- divider;
single-letter param names; per-event sdk_language/sdk_version that could live on the
batch envelope.
We completely understand if you're busy at the moment. If it's helpful, we'd be happy to work on these improvements ourselves and open PRs for you to review when you have time.
| context.getStack().getEvaluatedFeatures().add(featureKey); | ||
| } | ||
|
|
||
| private <ValueType> void dispatchFeatureUsage( |
There was a problem hiding this comment.
ValueType isn't idiomatic Java. The convention for type parameters is a
single uppercase letter (T, or V for a value), not a descriptive word — the JLS
naming guideline exists precisely so type variables are visually distinct from
class names.
There was a problem hiding this comment.
Kept ValueType here deliberately: the whole evaluator layer (ExperimentEvaluator/FeatureEvaluator signatures) uses <ValueType>, so switching just this helper to V would make the file inconsistent. Happy to do a repo-wide rename to the single-letter convention as a separate cleanup PR if you'd prefer.
# Conflicts: # lib/build.gradle # lib/src/main/java/growthbook/sdk/java/GrowthBook.java # lib/src/main/java/growthbook/sdk/java/evaluators/FeatureEvaluator.java # lib/src/main/java/growthbook/sdk/java/model/GBContext.java # lib/src/main/java/growthbook/sdk/java/multiusermode/GrowthBookClient.java # lib/src/main/java/growthbook/sdk/java/multiusermode/configurations/Options.java
- TrackingEvent: match Go event fields (experiment_id, variation_value, feature_value, source, in_experiment, hash_used, on, off, feature_id, experiment_name), epoch-millis timestamp, event_type enum, Lombok @builder; drop the user-attribute export (GDPR) — Go sends none. - Simplify GrowthBookPlugin: remove the EvaluationContext overloads whose only purpose was attribute capture; PluginRegistry.fire* drop the context. - PluginRegistry: filter null plugins, catch Exception (not Throwable) so Errors propagate, drop plugins whose init() fails, consistent isEmpty guards. - GrowthBookTrackingPlugin: create resources in init() (side-effect-free constructor, no thread leak when disabled/unregistered); bounded flush queue with drop-newest; split close() into helpers; handle flush failure at the source. - TrackingPluginConfig: StringUtils.isBlank, hide raw getters behind resolved*(), clamp batchSize. - ExperimentEvaluator: extract dispatchExperimentViewed helper used by both the local and remote-eval paths.
- Carry the PluginRegistry on EvaluationContext (per evaluation) instead of mutating the shared, caller-owned Options. GrowthBookClient now owns its registry, so reusing one Options across clients no longer lets one client's evaluations or shutdown() reach another client's plugins. Registry removed from Options. - FeatureEvaluator: the global forced-override path now calls dispatchFeatureUsage, so callbacks and plugins receive feature_evaluated for forced values, matching the URL-override and normal paths. (Memoized and exception-fallback returns are intentionally left: memoized already fired on first evaluation; the catch block is an error fallback, not a real usage.)
MockWebServer (okhttp) intermittently failed on CI with NoClassDefFoundError: okhttp3/internal/Util despite okhttp 4.11.0 resolving cleanly. Swap the plugin HTTP tests to a small JDK com.sun.net.httpserver based RecordingHttpServer and drop the mockwebserver test dependency, so the tests no longer couple to okhttp internals on the test classpath.
…nt loss A timer (or eager) flush drained the buffer and released the lock before submitFlush() registered the batch as in-flight. A concurrent close() could observe an empty buffer and zero in-flight batches, shut down the executor and HTTP client, and return before the batch was submitted — the batch was then rejected/failed and its events silently lost, violating the synchronous-flush contract. Move beginFlush() inside the buffer lock in enqueue() and scheduledFlush() so close() always observes and waits for the batch. Adds a timer-vs-close regression test.
… seam The previous test could pass even when the timer never fired (close()'s finalFlush would send the buffered event), so it did not prove the drain-then-close ordering. Add a narrowly-scoped test seam invoked on the timer thread between reserving a batch in-flight and submitting it, and use two latches to pin the interleaving: the timer drains + reserves, parks at the boundary, close() runs concurrently, then the timer submits. The test asserts close() stayed alive until the reserved batch was submitted and that the batch was delivered — it fails on the pre-fix ordering (reserve after unlock) and passes with the reservation under the lock.
Three non-blocking lifecycle items from review: - closeTimeout now bounds the synchronous final POST (via an OkHttp call timeout on a client derived from the configured one), not just the async in-flight wait, so close()/destroy() can't block past the budget on a hung final request. Doc corrected. - enqueue() re-checks closed under the buffer lock: a request racing a concurrent shutdown now drops its event cleanly instead of orphaning it in the buffer or scheduling on a stopped scheduler. - Document "one tracking plugin instance per SDK client" and warn when init() is called more than once (the tell-tale of a shared instance), since a shared plugin's first close() disables tracking for all owners. Adds regression tests (real plugin + JDK server): closeTimeout bounds a hung final flush, and a shared-plugin-across-instances test documenting the constraint. RecordingHttpServer gains a response-delay hook.
2 participants
Summary
Adds support for GrowthBook plugins and ports the built-in tracking plugin pattern for the legacy client.
What changed
GrowthBook/GBContext) and multi-user (GrowthBookClient/Options) modesTrackingCallback/FeatureUsageCallbackGrowthBookTrackingPluginthat batchesexperiment_viewedandfeature_evaluatedevents and POSTs them to the ingest endpointWire contract
POST {ingestorHost}/eventshttps://us1.gb-ingest.com{ "client_key": "...", "events": [...] }Content-Type: application/json,User-Agent: growthbook-java-sdk/{version}experiment_viewed,feature_evaluatedbatchSize=100,batchTimeout=10sclose()performs a synchronous final flush