Add plugins support for both legacy and new clients #206
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| import growthbook.sdk.java.multiusermode.configurations.EvaluationContext; | ||
| import growthbook.sdk.java.multiusermode.usage.FeatureUsageCallbackWithUser; | ||
| import growthbook.sdk.java.multiusermode.usage.TrackingCallbackWithUser; | ||
| import growthbook.sdk.java.plugin.PluginRegistry; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import javax.annotation.Nullable; | ||
| import java.net.MalformedURLException; | ||
|
|
@@ -42,10 +43,6 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| EvaluationContext context, | ||
| Class<ValueType> valueTypeClass | ||
| ) throws ClassCastException { | ||
| FeatureResult<ValueType> unknownFeatureResult = FeatureResult | ||
| .<ValueType>builder() | ||
| .value(null) | ||
|
|
@@ -66,9 +63,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .value(null) | ||
| .source(FeatureResultSource.CYCLIC_PREREQUISITE) | ||
| .build(); | ||
| dispatchFeatureUsage(context, key, featureResultWhenCircularDependencyDetected); | ||
|
|
||
| leaveCircularLoop(context); | ||
| return featureResultWhenCircularDependencyDetected; | ||
|
|
@@ -106,9 +101,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .source(FeatureResultSource.URL_OVERRIDE) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, urlFeatureResult); | ||
|
|
||
| return cacheResult(key, urlFeatureResult, context); | ||
| } | ||
|
|
@@ -117,9 +110,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| // Unknown key, return empty feature | ||
|
Collaborator
There was a problem hiding this comment. Since this PR is already refactoring |
||
| Map<String, Feature<?>> features = context.getGlobal().getFeatures(); | ||
| if (features == null || features.isEmpty() || !features.containsKey(key)) { | ||
| dispatchFeatureUsage(context, key, unknownFeatureResult); | ||
|
|
||
| return cacheResult(key, unknownFeatureResult, context); | ||
| } | ||
|
|
@@ -134,9 +125,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
|
|
||
| if (feature == null) { | ||
| // When key exists but there is no value, should be default value with null value | ||
| dispatchFeatureUsage(context, key, defaultValueFeature); | ||
| return cacheResult(key, defaultValueFeature, context); | ||
| } | ||
|
|
||
|
|
@@ -148,9 +137,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .source(FeatureResultSource.DEFAULT_VALUE) | ||
| .value(value) | ||
| .build(); | ||
| dispatchFeatureUsage(context, key, defaultValueFeatureForRules); | ||
| return cacheResult(key, defaultValueFeatureForRules, context); | ||
| } | ||
|
|
||
|
|
@@ -179,11 +166,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .source(FeatureResultSource.CYCLIC_PREREQUISITE) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, featureResultWhenCircularDependencyDetected); | ||
| return cacheResult(key, featureResultWhenCircularDependencyDetected, context); | ||
| } | ||
|
|
||
|
|
@@ -212,11 +195,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .source(FeatureResultSource.PREREQUISITE) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, featureResultWhenBlockedByPrerequisite); | ||
| return cacheResult(key, featureResultWhenBlockedByPrerequisite, context); | ||
| } | ||
| // non-blocking prerequisite eval failed: break out | ||
|
|
@@ -284,16 +263,26 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| // 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(); | ||
|
Collaborator
There was a problem hiding this comment. Same issue again — this just bolts more inline logic onto an already large evaluateFeature. Would you mind extracting the onTrack + fireExperimentViewed dispatch into a shared helper instead of growing the method further? Should keep things a bit tidier going forward, thanks! |
||
|
|
||
| // If this was a remotely evaluated experiment, fire the tracking callbacks | ||
| if (trackData != null) { | ||
| trackData.forEach(t -> { | ||
|
Collaborator
There was a problem hiding this comment. Since main already solved this in #216 with experimentEvaluator.fireRemoteEvaluationTracks(...) (dedupes exposures, null-guards payloads), it'd be worth rebasing onto that instead of adding a parallel helper here. Could you route the plugin's fireExperimentViewed through that existing path once you've rebased? Should save us from maintaining two versions of the same logic. |
||
| if (trackingCallBackWithUser != null) { | ||
| trackingCallBackWithUser.onTrack( | ||
| t.getExperiment(), | ||
| t.getResult().getExperimentResult(), | ||
| context.getUser() | ||
| ); | ||
| } | ||
| if (pluginRegistry != null) { | ||
| pluginRegistry.fireExperimentViewed( | ||
| t.getExperiment(), | ||
| t.getResult().getExperimentResult(), | ||
| context | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if (rule.getRange() == null) { | ||
|
|
@@ -325,9 +314,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .ruleId(rule.getId()) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, forcedRuleFeatureValue); | ||
|
|
||
| return cacheResult(key, forcedRuleFeatureValue, context); | ||
| } else { | ||
|
|
@@ -378,9 +365,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .experimentResult(result) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, experimentFeatureResult); | ||
| return cacheResult(key, experimentFeatureResult, context); | ||
| } | ||
| } else { | ||
|
|
@@ -399,9 +384,7 @@ public <ValueType> FeatureResult<ValueType> evaluateFeature( | |
| .value(value) | ||
| .build(); | ||
|
|
||
| dispatchFeatureUsage(context, key, defaultValueFeatureResult); | ||
|
|
||
| // Return (value = defaultValue or null, source = defaultValue) | ||
| return cacheResult(key, defaultValueFeatureResult, context); | ||
|
|
@@ -458,6 +441,21 @@ private void addFeatureToEvalStack(String featureKey, EvaluationContext context) | |
| context.getStack().getEvaluatedFeatures().add(featureKey); | ||
| } | ||
|
|
||
| private <ValueType> void dispatchFeatureUsage( | ||
|
Collaborator
There was a problem hiding this comment.
|
||
| EvaluationContext context, | ||
| String key, | ||
| FeatureResult<ValueType> result | ||
| ) { | ||
| FeatureUsageCallbackWithUser cb = context.getOptions().getFeatureUsageCallbackWithUser(); | ||
| if (cb != null) { | ||
| cb.onFeatureUsage(key, result, context.getUser()); | ||
| } | ||
| PluginRegistry registry = context.getOptions().getPluginRegistry(); | ||
| if (registry != null) { | ||
| registry.fireFeatureEvaluated(key, result, context); | ||
| } | ||
| } | ||
|
|
||
| private <ValueType> FeatureResult<ValueType> cacheResult(String key, FeatureResult<ValueType> result, EvaluationContext context) { | ||
| context.getStack().getMemoizedResults().putIfAbsent(key, result); | ||
| return result; | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evaluateExperimentis already one of the largest methods in the codebase andcarries 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 in
FeatureEvaluator, e.g.dispatchExperimentViewed(context, experiment, result). The method body thengains 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.