Optimizely X v1 integration #1

brennan · 2017-09-18T19:05:46Z

@f2prateek Don't intend to merge and release quite yet, but perhaps we can do a code review together while I'm here.

…or testing

ladanazita · 2017-09-19T18:09:33Z

I think it would be good to add log statements

f2prateek · 2017-09-19T18:10:59Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+  OptimizelyXIntegration(final Analytics analytics) {
+
+    listener =
+        new NotificationListener() {


This is the core functionality of the integration. It shouldn't be an anonymous class.

f2prateek · 2017-09-19T18:12:04Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+
+  OptimizelyXIntegration(final Analytics analytics) {
+
+    listener =


This listener is unused.... you need to pass it to Optimizely so that it can actually call it.

f2prateek · 2017-09-19T18:14:17Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

-    this.context = context;
-    this.logger = logger;
-  }
+public class OptimizelyXIntegration extends Integration<NotificationListener> {


Why should this return the listener? I think this should return Void.

Should be something like public class OptimizelyXIntegration extends Integration<Void> implements OptimizelyEventListener { yeah?

functionally yes. the exact code wouldn't work because OptimizelyEventListener is in the old version only; while the new version has NotificationListener it's an abstract class and not an interface, so it needs to be structured differently.

ladanazita · 2017-09-19T18:10:56Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+    listener =
+        new NotificationListener() {
+          @Override
+          public void onEventTracked(


If I understood this correctly, this listener on iOS listens for an Optimizely event to be tracked - which we are not concerned about. I think we need to only listen for the onExperimentActivated event since the main use case that customers are looking for is the ability to send events to Optimizely without having to instrument native Optimizely track calls

Users will still have to implement native Optimizely track and activate method calls - the NotificationListener class is called only when either track or activate in Optimizely's native API is invoked. That said, I agree that we should only listen for "activate" events! Users will probably already have Segment track events set up to track user events and goal completions.

ladanazita · 2017-09-19T18:14:49Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+              Long eventValue,
+              LogEvent logEvent) {
+
+            if (!isNullOrEmpty(attributes)) {


Just so I understand, you calling identify with the Optimizely user attributes, correct?

Yes! But dovetailing with your comment above, I agree that all we really care about is tracking when an experiment was activated. With this in mind, I think we should just remove our call to identify with Optimizely attributes, since users will prob be calling identify via Segment as well.

Jaime at Optimizely mentions that our mutual clients would like the ability to send attributes/traits of users to Optimizely so that Optimizely can use those attributes/traits for targeting. However, we can't really do much here because Optimizely X doesn't have a way to set these user attributes outside of an activate experiment event.

I'm not sure if it would be helpful but we could set a user property for the given user with the variation and experiment viewed

Hmmm that might be tricky, because a single user can be associated with many different experiments - I think this may result in trait bloat in other downstream tools and warehouses.

ladanazita · 2017-09-20T18:09:29Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+  public void track(TrackPayload track) {
+    super.track(track);
+
+    if (!isNullOrEmpty(track.userId())) {


We may need to consider only tracking anonymous users, since an experiment is likely to be set early on, before an identify is called.

…eck for valid OptimizelyClient

ladanazita · 2017-09-25T01:02:33Z

src/test/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXTest.java


  @Before public void setUp() {
-  }
+    initMocks(this);
+    PowerMockito.mock(OptimizelyClient.class);


I think you also have to mock the manager and when Optimizely is initialized, return the mocked client. Something maybe like

manager = mock(OptimizelyManager.class); when(manager.getOptimizely()).thenReturn(client);

ladanazita · 2017-09-25T01:10:46Z

src/test/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXTest.java

+    Traits traits = new Traits()
+            .putValue("userId", "123");
+    integration.track(new TrackPayloadBuilder().properties(properties).traits(traits).event("event").build());
+


You may have to do something like when(client.isValid()).thenReturn(true);

Awaiting feedback from Prateek on the new approach in the main lib before completing tests - these tests are still valid for the previous version of the integration.

ladanazita · 2017-09-25T01:17:17Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+        if (!client.isValid()) {
+          logger.verbose("The OptimizelyClient instance is invalid. Please check your Optimizely project id.");
+          try {
+            Thread.sleep(60000);


Why pause execution for this time interval?

Recursive method that polls the validity of the OptimizelyClient instance once per minute until it's valid. This value is arbitrary, but we should seek feedback from Jaime on a more appropriate interval.

ladanazita · 2017-09-25T01:17:31Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+          logger.verbose("The OptimizelyClient instance is invalid. Please check your Optimizely project id.");
+          try {
+            Thread.sleep(60000);
+          } catch ( java.lang.InterruptedException e) {


Should this log the error?

brennan · 2017-09-28T18:06:19Z

@f2prateek @ladanazita Just E2E tested, and I think this is ready for a final code review. Thanks!

ladanazita · 2017-09-28T22:09:35Z

build.gradle

    mavenCentral()
  }

-  provided 'com.segment.analytics.android:analytics:4.0.0'
+  compile 'com.optimizely.ab:android-sdk:+'


For Android we have refrained from using + - best practice dictates to specify version numbers as + can lead to unpredictable builds - not sure if we have a strong opinion here

ladanazita · 2017-09-28T22:10:34Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+  private List<TrackPayload> trackEvents = new ArrayList<>();
+  private final Handler handler = new Handler();
+
+  public static Factory factory(OptimizelyManager manager) {


I don't think this is ever used

It's used by customers when register the factory with Segment.

…date tests

f2prateek · 2017-10-02T16:20:29Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+    listen = settings.getBoolean("listen", true);
+
+    if (client.isValid()) {
+      isClientValid = true;


Instead of using a separate variable, why not just use the client object with client.isValid()?

This was intentional for efficiency, and to ensure we're only checking the client object directly on our polling thread. Let me know if this is not the right approach!

f2prateek · 2017-10-02T16:21:52Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+      if (!isClientValid) {
+        logger.verbose("Optimizely not initialized. Enqueueing action: %s", track);
+        if (queueSize < 100) {
+          trackEvents.add(track);


The two blocks have shared code. Should refactor it to be DRY. Also should log if a message is being dropped.

ladanazita · 2017-10-03T00:05:24Z

...main/java/com/segment/analytics/android/integrations/optimizelyx/OptimizelyXIntegration.java

+      if (!nonInteraction) {
+        properties.remove("nonInteraction");
+      }
+      analytics.track("Experiment Viewed", properties, options);


I think you will also need to pass in Optimizely X : no so the event does not attempt to go back into Optimizely

Good catch! That's already specified in the options object passed into the event.

* Fixes to main lib, update tests to check track queue flush method

brennan added 2 commits September 17, 2017 16:49

Add core int library/working on tests

9cb70c8

Remove superfluous test deps; stub Experiment and Variation objects f…

e8af7db

…or testing

brennan requested a review from f2prateek September 18, 2017 19:05

Fix linter errors; add identify method to tests

dac01ca

f2prateek reviewed Sep 19, 2017

View reviewed changes

ladanazita reviewed Sep 19, 2017

View reviewed changes

brennan added 3 commits September 19, 2017 14:12

Refactor main library; working on updating tests

4b417d7

Fix tests; fix typos in main lib

0ce7bd0

Remove unused import statement/class

694dd9f

ladanazita reviewed Sep 20, 2017

View reviewed changes

brennan added 3 commits September 20, 2017 11:49

Add trackKnownUsers setting and tests

9d34362

Simplify trackKnownUser logic and update tests

43c7ec1

Store track events if Optimizely isn't initialized; add polling to ch…

44dfd09

…eck for valid OptimizelyClient

ladanazita reviewed Sep 25, 2017

View reviewed changes

brennan added 8 commits September 24, 2017 20:30

Optimize imports; remove superfluous logging; optimize thread

fea7867

Update polling thread; fix tests

1025aab

Update pollOptimizelyClient method

bd64238

Update polling thread, tests, add defaultAttribute mapping

04d2403

Add test for polling Optimizely client

f4ec1f0

Test for flushing queued track events

d7382b8

Add test for attributes and eventTags

74346db

Update tests and fix polling method in main lib

9af3c37

Add logging; improve track logic

5e2e0a5

ladanazita reviewed Sep 28, 2017

View reviewed changes

Add settings for nonInteraction and to conditionally set listener; up…

9f25eb7

…date tests

brennan added 2 commits September 29, 2017 15:12

Fix nonInteraction logic; fix listener initialization; update tests

98d1020

Default nonInteraction setting to true; update tests

6928f8c

f2prateek reviewed Oct 2, 2017

View reviewed changes

brennan added 2 commits October 2, 2017 11:11

Clean up code logic

5d66543

Pin Optimizely SDK version to latest, 1.4.0

c0f844f

ladanazita reviewed Oct 3, 2017

View reviewed changes

Fixes to main library; updates to tests (#2)

f32b85c

* Fixes to main lib, update tests to check track queue flush method

f2prateek merged commit 3b505df into master Oct 3, 2017


		OptimizelyXIntegration(final Analytics analytics) {

		listener =

Optimizely X v1 integration #1

Optimizely X v1 integration #1

Uh oh!

Conversation

brennan commented Sep 18, 2017

Uh oh!

ladanazita commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brennan Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brennan Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brennan commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brennan Sep 19, 2017 •

edited

Loading

brennan Sep 25, 2017 •

edited

Loading