Refactor EventFactory into static methods. #311

mikecdavis · 2019-06-14T19:42:10Z

Summary

Remove EventFactory from Optimizely constructor.
Remove ClientVersion and ClientEngine from OptimizelyBuilder
Adds ClientEngineInfo for holding global engine value.
Defines new UserEvent, ConversionEvent and ImpressionEvent models
Redefines EventFactory to accept UserEvents via static methods
Defines UserEventFactory to create UserEvents.

This change is in preparation for batching event payloads so we first want to extract separate canonical representations of conversions and impressions disjoint from the EventBatch model used for sending to the logging backend.

This change is in preparation for an EventProcessor abstraction. #310 is blocked on this change for a cleaner implementation.

coveralls · 2019-06-14T19:52:14Z

Pull Request Test Coverage Report for Build 1100

184 of 194 (94.85%) changed or added relevant lines in 10 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.03%) to 89.719%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/event/internal/UserEventFactory.java	27	28	96.43%
core-api/src/main/java/com/optimizely/ab/event/internal/ClientEngineInfo.java	8	10	80.0%
core-api/src/main/java/com/optimizely/ab/Optimizely.java	15	18	83.33%
core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java	52	56	92.86%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java	1	95.1%
core-api/src/main/java/com/optimizely/ab/Optimizely.java	2	93.24%

Totals
Change from base Build 1094:	-0.03%
Covered Lines:	3412
Relevant Lines:	3803

💛 - Coveralls

loganlinn · 2019-06-17T20:08:07Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -309,22 +301,18 @@ public void track(@Nonnull String eventName,
            logger.warn("Event tags is null when non-null was expected. Defaulting to an empty event tags map.");
        }

-        // create the conversion event request parameters, then dispatch
-        LogEvent conversionEvent = eventFactory.createConversionEvent(
+        UserEvent event = UserEventFactory.createConversionEvent(
            projectConfig,
            userId,
            eventType.getId(),
            eventType.getKey(),
            copiedAttributes,


might consider moving the copying of attribute into the factory method -- i never understood why it was here

Those attributes get passed around to DecisionService and to Notifications. I think a refactor is a good idea, but will do so in a follow-up PR.

loganlinn · 2019-06-17T20:13:11Z

core-api/src/main/java/com/optimizely/ab/event/internal/ClientEngineInfo.java

+public class ClientEngineInfo {
+    private static final Logger logger = LoggerFactory.getLogger(ClientEngineInfo.class);
+
+    public static final EventBatch.ClientEngine DEFAULT = EventBatch.ClientEngine.JAVA_SDK;


side-notes:

it would be nice to move that enum out of EventBatch eventually

it would be nice to if we set this automatically -- there are some fairly well-known ways to test whether current runtime is Android. ultimately, this shouldn't be user-defined.

Yeah I think you can do that for android in general, but then Mobile vs TV is a little more tricky and we do that already here. This is a step into making it less clunky and having to pass that info everywhere.

loganlinn · 2019-06-17T20:16:00Z

core-api/src/main/java/com/optimizely/ab/event/internal/ConversionEvent.java

+        this.eventKey = eventKey;
+        this.revenue = revenue;
+        this.value = value;
+        this.tags = tags;


should we default to Collections.emptyMap()?

This is enforced in the Optimizely#track call.

loganlinn · 2019-06-17T20:16:53Z

core-api/src/main/java/com/optimizely/ab/event/internal/ConversionEvent.java

+
+    private ConversionEvent(String eventId, String eventKey, Number revenue, Number value, Map<String, ?> tags) {
+        this.eventId = eventId;
+        this.eventKey = eventKey;


is there a case where it is helpful to allow eventId == null && eventKey == null aside from having an empty constructor?

This is enforced in the Optimizely#track call.

loganlinn · 2019-06-17T20:18:49Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

            projectConfig,
            userId,
            eventType.getId(),
            eventType.getKey(),
            copiedAttributes,
            eventTags);

+        // create the conversion event request parameters, then dispatch
+        LogEvent conversionEvent = EventFactory.createLogEvent(Collections.singletonList(event));


looks like there's a signature included in this PR that would support EventFactory.createLogEvent(event);

Ha yeah, I didn't go back and update all these :)

loganlinn · 2019-06-17T20:23:11Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

-            this.clientEngine = clientEngine;
+        @Deprecated
+        public Builder withClientEngine(EventBatch.ClientEngine clientEngine) {
+            logger.info("Explicitly setting the ClientEngine is no longer supported.");


can we temporarily bridge to the new ClientEngineInfo class until future release to avoid silent breakage?

also, this should mention what to use instead and, ideally, the version when it was marked as deprecated

loganlinn · 2019-06-17T20:25:04Z

core-api/src/main/java/com/optimizely/ab/event/internal/ConversionEvent.java

+            this.tags = tags;
+            return this;
+        }
+


nit: convenience method for setting single tag would be nice

public Builder withTag(String tag, Object value) { this.tags = (this.tags != null) ? this.tags : new HashMap<>(); this.tags.put(tag, value); return this; }

This would probably just be used in testing, but I'll review.

loganlinn · 2019-06-17T20:28:11Z

core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java


 import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;

 public class EventFactory {
    private static final Logger logger = LoggerFactory.getLogger(EventFactory.class);
-    static final String EVENT_ENDPOINT = "https://logx.optimizely.com/v1/events";  // Should be part of the datafile
-    static final String ACTIVATE_EVENT_KEY = "campaign_activated";
+    public static final String EVENT_ENDPOINT = "https://logx.optimizely.com/v1/events";  // Should be part of the datafile


side-note: I was hoping to make this DEFAULT_EVENT_ENDPOINT and add a LogEventFactory that parametrizes this as well as the other constants we are passing to new LogEvent() constructor. I expect that ability to customize the URL will be helpful/necessary in future.

This is a valid point, but I want to address it when that feature is supported.

loganlinn · 2019-06-17T20:32:11Z

core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java

-    public EventFactory(EventBatch.ClientEngine clientEngine, String clientVersion) {
-        this.clientEngine = clientEngine;
-        this.clientVersion = clientVersion;
+    public static LogEvent createLogEvent(List<UserEvent> userEvents) {


worth documenting this functionality, specifically around returning null

loganlinn · 2019-06-17T21:11:00Z

core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java

-            .setTags(eventTags)
-            .setType(eventType.getKey())
-            .setValue(EventTagUtils.getNumericValue(eventTags))
+    public static Visitor createConversionEvent(UserEvent userEvent) {


this should be private (like createImpressionEvent above), no?

mikecdavis · 2019-06-18T16:40:42Z

After SDK CoP and discussion with @loganlinn I'm going to re-work the UserEvent model to remove the "union" behavior it's serving today and instead define a UserContext encapsulating the top-level, shared attributes and have a ConversionEvent and ImpressionEvent take a UserContext in their respective builders.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

aliabbasrizvi · 2019-06-18T23:41:39Z

core-api/src/main/java/com/optimizely/ab/event/internal/UserContext.java

+/**
+ * UserContext stores the user and project context timestamp and global id.
+ *
+ * Alt name EventContext?


I'd prefer EventContext. Hard for me to associate UUID and timestamp with a userId.

We can still have UserContext which holds userId and attributes (and perhaps more things in the future) and then user UserContext in EventContext.

eh, I can split this out into UserContext and EventContext

aliabbasrizvi · 2019-06-19T20:57:11Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java


-            if (logger.isDebugEnabled()) {
-                logger.debug(


I personally found this message useful. Any plans to restore this or moving this to AsyncEventHandler?

That's a good idea i can move it there.

aliabbasrizvi · 2019-06-19T20:58:32Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -309,22 +301,18 @@ public void track(@Nonnull String eventName,
            logger.warn("Event tags is null when non-null was expected. Defaulting to an empty event tags map.");
        }

-        // create the conversion event request parameters, then dispatch
-        LogEvent conversionEvent = eventFactory.createConversionEvent(
+        UserEvent event = UserEventFactory.createConversionEvent(


nit. May be call this userEvent to be consistent with how you named it above.

aliabbasrizvi · 2019-06-19T20:59:20Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

-            this.clientEngine = clientEngine;
+        @Deprecated
+        public Builder withClientEngine(EventBatch.ClientEngine clientEngine) {
+            logger.info("Deprecated. Setting ClientEngine is now set via ClientEngineInfo.");


nit. Sounds incorrect. Perhaps drop the word Setting.

Updated to "Deprecated. In the future, set ClientEngine via ClientEngineInfo#setClientEngine."

aliabbasrizvi · 2019-06-19T21:03:27Z

core-api/src/main/java/com/optimizely/ab/event/internal/ClientEngineInfo.java

+    public static void setClientEngine(EventBatch.ClientEngine clientEngine) {
+        if (clientEngine == null) {
+            logger.warn("ClientEngine cannot be null, defaulting to {}", ClientEngineInfo.clientEngine.getClientEngineValue());
+            return;


Where is the default set?

Default is set above. Initial default is JAVA_SDK, but can get updated at any time to another valid option.

aliabbasrizvi

Looks good.

aliabbasrizvi · 2019-06-20T05:03:23Z

core-api/src/main/java/com/optimizely/ab/event/internal/UserContext.java

+/**
+ * UserContext stores the user and project context timestamp and global id.
+ *
+ * Alt name EventContext?


nit. This comment may be out of date now?

Mike Davis added 6 commits June 14, 2019 09:09

Refactor EventFactory.

f2ec556

Add licenses and doc strings.

0977316

Add tests for new models.

0053db7

Rename to make diff less noisy.

12dc67c

Clean up diff more.

8538292

Clean up diff more.

6020d93

mikecdavis requested review from loganlinn and aliabbasrizvi June 14, 2019 19:42

mikecdavis changed the title ~~Mikecdaivs/oasis 4895 event factory refact~~ Refactor EventFactory into static methods. Jun 14, 2019

loganlinn reviewed Jun 17, 2019

View reviewed changes

Mike Davis added 2 commits June 18, 2019 09:45

Change UserEvent to an interface and define UserContext.

2cf54c3

Add doc and better info message.

3a2e645

mikecdavis requested a review from loganlinn June 18, 2019 22:26

aliabbasrizvi reviewed Jun 19, 2019

View reviewed changes

Mike Davis added 2 commits June 19, 2019 17:23

Move UUID and timestamp to base class.

a889f03

Address PR feedback.

4fd0d30

aliabbasrizvi approved these changes Jun 20, 2019

View reviewed changes

Update comment.

b681578

mikecdavis merged commit bdb3b4e into master Jun 20, 2019

mikecdavis deleted the mikecdaivs/OASIS-4895-event-factory-refact branch June 20, 2019 19:06

Refactor EventFactory into static methods. #311

Refactor EventFactory into static methods. #311

Uh oh!

Conversation

mikecdavis commented Jun 14, 2019

Summary

Uh oh!

coveralls commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1100

💛 - Coveralls

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!

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!

loganlinn Jun 17, 2019 • 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!

mikecdavis commented Jun 18, 2019

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jun 14, 2019 •

edited

Loading

loganlinn Jun 17, 2019 •

edited

Loading