Add plugins support for both legacy and new clients#206
Conversation
vazarkevych
left a comment
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.
| trackingCallBackWithUser.onTrack(experiment, result, context.getUser()); | ||
| } | ||
|
|
||
| PluginRegistry pluginRegistry = context.getOptions().getPluginRegistry(); |
There was a problem hiding this comment.
evaluateExperiment is already one of the largest methods in the codebase and
carries a lot of line-by-line "what" comments that restate the code. This change
adds another inline block to it, which pushes it further in the wrong direction.
Two things would keep it in check:
-
Extract the dispatch instead of inlining it — mirror the
dispatchFeatureUsage(...)
helper this same PR already introduced inFeatureEvaluator, e.g.
dispatchExperimentViewed(context, experiment, result). The method body then
gains one call, not a new null-check block, and both evaluators stay consistent. -
While touching this block, trim the redundant comments (the ones that just
re-describe the key formula / the callback) rather than growing the method. The
goal here should be fewer lines and less noise, not more.
Net: the plugin hook is welcome, but let's add it via a small helper and take the
opportunity to shrink this method rather than extend it.
| 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.
| @@ -117,9 +110,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |||
| // Unknown key, return empty feature | |||
There was a problem hiding this comment.
Since this PR is already refactoring evaluateFeature (swapping the inline
callback calls for dispatchFeatureUsage), it's a good moment to drop the
redundant comments in the method while you're in there. Most of them just restate
the next line ("Unknown key, return empty feature", "When key exists but there is
no value…", etc.) and add noise to an already long method.
| // Call the tracking callback with all the track data | ||
| List<TrackData<ValueType>> trackData = rule.getTracks(); | ||
| TrackingCallbackWithUser trackingCallBackWithUser = context.getOptions().getTrackingCallBackWithUser(); | ||
| PluginRegistry pluginRegistry = context.getOptions().getPluginRegistry(); |
There was a problem hiding this comment.
Same issue again — this just bolts more inline logic onto an already monstrous
evaluateFeature. Extract the onTrack + fireExperimentViewed dispatch into a
shared helper instead of growing the method further.
| if (trackData != null && trackingCallBackWithUser != null) { | ||
| trackData.forEach(t -> | ||
| if (trackData != null) { | ||
| trackData.forEach(t -> { |
There was a problem hiding this comment.
main already replaced this inline track loop with
experimentEvaluator.fireRemoteEvaluationTracks(...) (#216), which de-dupes exposures
and null-guards the payloads. After rebasing, route the plugin fireExperimentViewed
through that path rather than this one.
| return stripTrailingSlash(ingestorHost); | ||
| } | ||
|
|
||
| public int resolvedBatchSize() { |
There was a problem hiding this comment.
resolvedBatchSize() guards against <= 0 but not an unbounded upper value — a large
batchSize means the buffer never eager-flushes and just grows (compounds the unbounded
flush-queue issue). Worth clamping to a sane max.
| * so a failing plugin becomes a no-op rather than aborting registration. | ||
| */ | ||
| @Slf4j | ||
| public final class PluginRegistry { |
There was a problem hiding this comment.
The catch (Throwable) in initAll/fire*/closeAll is too broad. Best-effort
isolation is the right intent, but Throwable also swallows Errors (OOM,
StackOverflow) and keeps looping — at that point the JVM is likely dying and we're just
logging a warning. Catch Exception instead, so plugin bugs are isolated but Errors
propagate.
| return plugins.isEmpty(); | ||
| } | ||
|
|
||
| public void initAll() { |
There was a problem hiding this comment.
initAll() logs an init failure but keeps the plugin in the list, so it still receives
onExperimentViewed/onFeatureEvaluated/close. That pushes the "stay safe after a
failed init" burden onto every plugin (self-guard flags). Consider tracking init
failures and skipping those plugins for subsequent dispatch, so a plugin that failed to
initialize simply stops receiving events.
| private final List<GrowthBookPlugin> plugins; | ||
|
|
||
| public PluginRegistry(@Nullable List<GrowthBookPlugin> plugins) { | ||
| if (plugins == null || plugins.isEmpty()) { |
There was a problem hiding this comment.
Null entries in the input list aren't filtered — a null plugin becomes a logged
NPE on every dispatch; cheap to filter in the constructor.
| } | ||
|
|
||
| public void closeAll() { | ||
| for (GrowthBookPlugin plugin : plugins) { |
There was a problem hiding this comment.
fire* short-circuit with if (plugins.isEmpty()) return; but initAll/closeAll
don't — harmless, just inconsistent.
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