Skip to content

feat: Integrated ODPManager with UserContext and Optimizely client #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Nov 21, 2022
Merged

feat: Integrated ODPManager with UserContext and Optimizely client #490

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
525ad99
Integrated ODPManager with UserContext and Optimizely
zashraf1985 Oct 27, 2022
5d3cb21
fixed an issue in DefaultODPApiManager
zashraf1985 Oct 27, 2022
0a1bf21
Calling close on ODPEventManager when Optimizely closes
zashraf1985 Oct 28, 2022
0c74a18
Incorporated review feedback.
zashraf1985 Nov 2, 2022
42b2425
Changed GraphQl query to use variables for better sanitization.
zashraf1985 Nov 2, 2022
79263fc
Incorporated more review feedback.
zashraf1985 Nov 4, 2022
e573a6a
Added tests for `Optimizely`
zashraf1985 Nov 8, 2022
6d88b86
Completed the unit tests.
zashraf1985 Nov 9, 2022
68aa5ff
Added Non blocking implementation for fetching segments.
zashraf1985 Nov 11, 2022
29f2b09
Incorporated review feedback
zashraf1985 Nov 17, 2022
49a9686
removed redundant methods from cache
zashraf1985 Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 70 additions & 13 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import com.optimizely.ab.event.internal.*;
import com.optimizely.ab.event.internal.payload.EventBatch;
import com.optimizely.ab.notification.*;
import com.optimizely.ab.odp.ODPEvent;
import com.optimizely.ab.odp.ODPManager;
import com.optimizely.ab.optimizelyconfig.OptimizelyConfig;
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager;
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService;
Expand Down Expand Up @@ -96,6 +98,9 @@ public class Optimizely implements AutoCloseable {
@Nullable
private final UserProfileService userProfileService;

@Nullable
private final ODPManager odpManager;

private Optimizely(@Nonnull EventHandler eventHandler,
@Nonnull EventProcessor eventProcessor,
@Nonnull ErrorHandler errorHandler,
Expand All @@ -104,7 +109,8 @@ private Optimizely(@Nonnull EventHandler eventHandler,
@Nonnull ProjectConfigManager projectConfigManager,
@Nullable OptimizelyConfigManager optimizelyConfigManager,
@Nonnull NotificationCenter notificationCenter,
@Nonnull List<OptimizelyDecideOption> defaultDecideOptions
@Nonnull List<OptimizelyDecideOption> defaultDecideOptions,
@Nullable ODPManager odpManager
) {
this.eventHandler = eventHandler;
this.eventProcessor = eventProcessor;
Expand All @@ -115,6 +121,15 @@ private Optimizely(@Nonnull EventHandler eventHandler,
this.optimizelyConfigManager = optimizelyConfigManager;
this.notificationCenter = notificationCenter;
this.defaultDecideOptions = defaultDecideOptions;
this.odpManager = odpManager;

if (odpManager != null) {
odpManager.getEventManager().start();
if (getProjectConfig() != null) {
updateODPSettings();
}
addUpdateConfigNotificationHandler(configNotification -> { updateODPSettings(); });
}
}

/**
Expand All @@ -128,8 +143,6 @@ public boolean isValid() {
return getProjectConfig() != null;
}



/**
* Checks if eventHandler {@link EventHandler} and projectConfigManager {@link ProjectConfigManager}
* are Closeable {@link Closeable} and calls close on them.
Expand All @@ -141,6 +154,9 @@ public void close() {
tryClose(eventProcessor);
tryClose(eventHandler);
tryClose(projectConfigManager);
if (odpManager != null) {
tryClose(odpManager);
}
}

//======== activate calls ========//
Expand Down Expand Up @@ -674,9 +690,9 @@ public OptimizelyJSON getFeatureVariableJSON(@Nonnull String featureKey,
*/
@Nullable
public OptimizelyJSON getFeatureVariableJSON(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {

return getFeatureVariableValueForType(
featureKey,
Expand All @@ -688,10 +704,10 @@ public OptimizelyJSON getFeatureVariableJSON(@Nonnull String featureKey,

@VisibleForTesting
<T> T getFeatureVariableValueForType(@Nonnull String featureKey,
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes,
@Nonnull String variableType) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes,
@Nonnull String variableType) {
if (featureKey == null) {
logger.warn("The featureKey parameter must be nonnull.");
return null;
Expand Down Expand Up @@ -878,7 +894,7 @@ public OptimizelyJSON getAllFeatureVariables(@Nonnull String featureKey,
}
} else {
logger.info("User \"{}\" was not bucketed into any variation for feature flag \"{}\". " +
"The default values are being returned.", userId, featureKey);
"The default values are being returned.", userId, featureKey);
}

Map<String, Object> valuesMap = new HashMap<String, Object>();
Expand Down Expand Up @@ -1142,7 +1158,7 @@ public OptimizelyConfig getOptimizelyConfig() {
* @param userId The user ID to be used for bucketing.
* @param attributes: A map of attribute names to current user attribute values.
* @return An OptimizelyUserContext associated with this OptimizelyClient.
*/
*/
public OptimizelyUserContext createUserContext(@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (userId == null) {
Expand Down Expand Up @@ -1413,6 +1429,41 @@ public <T> int addNotificationHandler(Class<T> clazz, NotificationHandler<T> han
return notificationCenter.addNotificationHandler(clazz, handler);
}

@Nullable
public ODPManager getODPManager() {
return odpManager;
}

public void sendODPEvent(@Nullable String type, @Nonnull String action, @Nullable Map<String, String> identifiers, @Nullable Map<String, Object> data) {
if (odpManager != null) {
ODPEvent event = new ODPEvent(type, action, identifiers, data);
odpManager.getEventManager().sendEvent(event);
} else {
logger.error("ODP event send failed (ODP is not enabled)");
}
}

public void identifyUser(@Nonnull String userId) {
ODPManager odpManager = getODPManager();
if (odpManager != null) {
odpManager.getEventManager().identifyUser(userId);
}
}

public void identifyUser(@Nullable String vuid, @Nullable String userId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if need this api here. If we can determine if userId is vuid or fsUserId, we do not need this call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ODPManager odpManager = getODPManager();
if (odpManager != null) {
odpManager.getEventManager().identifyUser(vuid, userId);
}
}

private void updateODPSettings() {
if (odpManager != null && getProjectConfig() != null) {
ProjectConfig projectConfig = getProjectConfig();
odpManager.updateSettings(projectConfig.getHostForODP(), projectConfig.getPublicKeyForODP(), projectConfig.getAllSegments());
}
}

//======== Builder ========//

/**
Expand Down Expand Up @@ -1467,6 +1518,7 @@ public static class Builder {
private UserProfileService userProfileService;
private NotificationCenter notificationCenter;
private List<OptimizelyDecideOption> defaultDecideOptions;
private ODPManager odpManager;

// For backwards compatibility
private AtomicProjectConfigManager fallbackConfigManager = new AtomicProjectConfigManager();
Expand Down Expand Up @@ -1562,6 +1614,11 @@ public Builder withDefaultDecideOptions(List<OptimizelyDecideOption> defaultDeci
return this;
}

public Builder withODPManager(ODPManager odpManager) {
this.odpManager = odpManager;
return this;
}

// Helper functions for making testing easier
protected Builder withBucketing(Bucketer bucketer) {
this.bucketer = bucketer;
Expand Down Expand Up @@ -1636,7 +1693,7 @@ public Optimizely build() {
defaultDecideOptions = Collections.emptyList();
}

return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions);
return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions, odpManager);
}
}
}
82 changes: 79 additions & 3 deletions core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
*/
package com.optimizely.ab;

import com.optimizely.ab.config.Variation;
import com.optimizely.ab.odp.ODPManager;
import com.optimizely.ab.odp.ODPSegmentCallback;
import com.optimizely.ab.odp.ODPSegmentManager;
import com.optimizely.ab.odp.ODPSegmentOption;
import com.optimizely.ab.optimizelydecision.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -54,6 +57,15 @@ public OptimizelyUserContext(@Nonnull Optimizely optimizely,
@Nonnull Map<String, ?> attributes,
@Nullable Map<String, OptimizelyForcedDecision> forcedDecisionsMap,
@Nullable List<String> qualifiedSegments) {
this(optimizely, userId, attributes, forcedDecisionsMap, qualifiedSegments, true);
}

public OptimizelyUserContext(@Nonnull Optimizely optimizely,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes,
@Nullable Map<String, OptimizelyForcedDecision> forcedDecisionsMap,
@Nullable List<String> qualifiedSegments,
@Nullable Boolean shouldIdentifyUser) {
this.optimizely = optimizely;
this.userId = userId;
if (attributes != null) {
Expand All @@ -66,6 +78,10 @@ public OptimizelyUserContext(@Nonnull Optimizely optimizely,
}

this.qualifiedSegments = Collections.synchronizedList( qualifiedSegments == null ? new LinkedList<>(): qualifiedSegments);

if (shouldIdentifyUser == null || shouldIdentifyUser) {
optimizely.identifyUser(userId);
}
}

public OptimizelyUserContext(@Nonnull Optimizely optimizely, @Nonnull String userId) {
Expand All @@ -85,7 +101,7 @@ public Optimizely getOptimizely() {
}

public OptimizelyUserContext copy() {
return new OptimizelyUserContext(optimizely, userId, attributes, forcedDecisionsMap, qualifiedSegments);
return new OptimizelyUserContext(optimizely, userId, attributes, forcedDecisionsMap, qualifiedSegments, false);
}

/**
Expand Down Expand Up @@ -282,6 +298,67 @@ public void setQualifiedSegments(List<String> qualifiedSegments) {
this.qualifiedSegments.addAll(qualifiedSegments);
}

/**
* Fetch all qualified segments for the user context.
* <p>
* The segments fetched will be saved and can be accessed at any time by calling {@link #getQualifiedSegments()}.
*/
public void fetchQualifiedSegments() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we return boolean (true/false for success/failure) for blocking fetchSegment calls. Clients can check the status before continuing with decide call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fetchQualifiedSegments(Collections.emptyList());
}

/**
* Fetch all qualified segments for the user context.
* <p>
* The segments fetched will be saved and can be accessed at any time by calling {@link #getQualifiedSegments()}.
*
* @param segmentOptions A set of options for fetching qualified segments.
*/
public void fetchQualifiedSegments(@Nonnull List<ODPSegmentOption> segmentOptions) {
ODPManager odpManager = optimizely.getODPManager();
if (odpManager != null) {
synchronized (odpManager) {
setQualifiedSegments(odpManager.getSegmentManager().getQualifiedSegments(userId, segmentOptions));
}
}
}

/**
* Fetch all qualified segments for the user context in a non-blocking manner. This method will fetch segments
* in a separate thread and invoke the provided callback when results are available.
* <p>
* The segments fetched will be saved and can be accessed at any time by calling {@link #getQualifiedSegments()}.
*
* @param callback A callback to invoke when results are available.
* @param segmentOptions A set of options for fetching qualified segments.
*/
public void fetchQualifiedSegments(ODPSegmentCallback callback, List<ODPSegmentOption> segmentOptions) {
ODPManager odpManager = optimizely.getODPManager();
if (odpManager == null) {
logger.error("Audience segments fetch failed (ODP is not enabled");
callback.invokeCallback();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also pass the status boolean (success/failure) in the callback parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} else {
odpManager.getSegmentManager().getQualifiedSegments(userId, segments -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to keep this direct call to odpManager, I suggest to make "optimizely.identifyUser" call also direct call (for consistency). I still prefer moving all to optimizely :) I'm concerned about the increasing complexity of UserContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the direct call and made a wrapper in optimizely

if (segments != null) {
setQualifiedSegments(segments);
}
callback.invokeCallback();
}, segmentOptions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This async solution looks good!

}
}

/**
* Fetch all qualified segments for the user context in a non-blocking manner. This method will fetch segments
* in a separate thread and invoke the provided callback when results are available.
* <p>
* The segments fetched will be saved and can be accessed at any time by calling {@link #getQualifiedSegments()}.
*
* @param callback A callback to invoke when results are available.
*/
public void fetchQualifiedSegments(ODPSegmentCallback callback) {
fetchQualifiedSegments(callback, Collections.emptyList());
}

// Utils

@Override
Expand Down Expand Up @@ -309,5 +386,4 @@ public String toString() {
", attributes='" + attributes + '\'' +
'}';
}

}
1 change: 1 addition & 0 deletions core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ public List<Experiment> getExperiments() {
return experiments;
}

@Override
public Set<String> getAllSegments() {
return this.allSegments;
}
Expand Down
3 changes: 3 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import javax.annotation.Nullable;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* ProjectConfig is an interface capturing the experiment, variation and feature definitions.
Expand Down Expand Up @@ -69,6 +70,8 @@ Experiment getExperimentForKey(@Nonnull String experimentKey,

List<Experiment> getExperiments();

Set<String> getAllSegments();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this addition to the interface will require changes to all existing customization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If users are using their own ProjectConfigManagers? yes! But this is the only way to add more functionality to ProjectConfig. I believe almost no one will be customizing the whole ProjectConfigManager. If there are a handful of customers who are even doing it, ATS will go out as a major version change anyway. We can mention this in the release notes as a breaking change. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zashraf1985 We'll go for a major version change if we have to. We better keep all the breaking changes required.


List<Experiment> getExperimentsForEventKey(String eventKey);

List<FeatureFlag> getFeatureFlags();
Expand Down
2 changes: 2 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/internal/Cache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,6 @@ public interface Cache<T> {
void save(String key, T value);
T lookup(String key);
void reset();
Integer getMaxSize();
Integer getCacheTimeoutSeconds();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding these to Cache interface? We do not care about these when clients implement their own cache with own size and timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed these for some unit tests. I cant figure out any other way to verify some tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these methods and tests checking for these values.

}
10 changes: 10 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/internal/DefaultLRUCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,16 @@ public void reset() {
}
}

@Override
public Integer getMaxSize() {
return maxSize;
}

@Override
public Integer getCacheTimeoutSeconds() {
return (int) (timeoutMillis / 1000);
}

private class CacheEntity {
public T value;
public Long timestamp;
Expand Down
4 changes: 2 additions & 2 deletions core-api/src/main/java/com/optimizely/ab/odp/ODPApiManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
*/
package com.optimizely.ab.odp;

import java.util.List;
import java.util.Set;

public interface ODPApiManager {
String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck);
String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, Set<String> segmentsToCheck);

Integer sendEvents(String apiKey, String apiEndpoint, String eventPayload);
}
Loading