From 89de6e9fdeaca1eef99f3b256222950503aa0973 Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Thu, 26 Apr 2012 16:33:38 -0700 Subject: [PATCH] Optimizations with reporting using arrays instead of separate requests, bugfixes --- .../com/todoroo/astrid/data/ABTestEvent.java | 2 +- .../service/AstridDependencyInjector.java | 8 +-- .../astrid/service/StartupService.java | 9 +-- .../ABTestEventReportingService.java | 57 ++++++++----------- 4 files changed, 31 insertions(+), 45 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 06aa942af..4644d7c49 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -89,7 +89,7 @@ public class ABTestEvent extends AbstractModel { TABLE, "reported"); // 0 if not yet reported, 1 if reported successfully /** List of all properties for this model */ - public static final Property[] PROPERTIES = generateProperties(Task.class); + public static final Property[] PROPERTIES = generateProperties(ABTestEvent.class); diff --git a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java index 5df150afd..919abb57b 100644 --- a/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java +++ b/astrid/src/com/todoroo/astrid/service/AstridDependencyInjector.java @@ -65,9 +65,9 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // com.todoroo.astrid.dao injectables.put("database", Database.class); - injectables.put("taskDao", new TaskDao()); - injectables.put("metadataDao", new MetadataDao()); - injectables.put("tagDataDao", new TagDataDao()); + injectables.put("taskDao", TaskDao.class); + injectables.put("metadataDao", MetadataDao.class); + injectables.put("tagDataDao", TagDataDao.class); injectables.put("storeObjectDao", StoreObjectDao.class); injectables.put("updateDao", UpdateDao.class); injectables.put("userDao", UserDao.class); @@ -103,7 +103,7 @@ public class AstridDependencyInjector extends AbstractDependencyInjector { // AB testing injectables.put("abChooser", ABChooser.class); injectables.put("abTests", new ABTests()); - injectables.put("abTestEventDao", new ABTestEventDao()); + injectables.put("abTestEventDao", ABTestEventDao.class); injectables.put("abTestEventReportingService", ABTestEventReportingService.class); injectables.put("abTestInvoker", ABTestInvoker.class); injectables.put("featureFlipper", FeatureFlipper.class); diff --git a/astrid/src/com/todoroo/astrid/service/StartupService.java b/astrid/src/com/todoroo/astrid/service/StartupService.java index 9e83483b1..8a18995bc 100644 --- a/astrid/src/com/todoroo/astrid/service/StartupService.java +++ b/astrid/src/com/todoroo/astrid/service/StartupService.java @@ -184,7 +184,7 @@ public class StartupService { // perform startup activities in a background thread final int finalLatestVersion = latestSetVersion; - initializeABTesting(latestSetVersion == 0); + abChooser.makeChoicesForAllTests(latestSetVersion == 0, taskService.getUserActivationStatus()); new Thread(new Runnable() { public void run() { @@ -220,6 +220,7 @@ public class StartupService { featureFlipper.updateFeatures(); abTestEventDao.createRelativeDateEvents(); + abTestEventReportingService.pushAllUnreportedABTestEvents(); } }).start(); @@ -232,12 +233,6 @@ public class StartupService { hasStartedUp = true; } - private void initializeABTesting(boolean newUser) { - abTestEventReportingService.initialize(); - abTestEventReportingService.pushAllUnreportedABTestEvents(); - abChooser.makeChoicesForAllTests(newUser, taskService.getUserActivationStatus()); - } - /** * @param context * @param e error that was raised diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 9d14263b7..d1793d7b3 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -8,10 +8,10 @@ import org.json.JSONObject; import android.util.Log; -import com.todoroo.andlib.data.DatabaseDao.ModelUpdateListener; import com.todoroo.andlib.data.TodorooCursor; import com.todoroo.andlib.service.Autowired; import com.todoroo.andlib.service.DependencyInjectionService; +import com.todoroo.andlib.sql.Order; import com.todoroo.andlib.sql.Query; import com.todoroo.astrid.dao.ABTestEventDao; import com.todoroo.astrid.data.ABTestEvent; @@ -36,38 +36,11 @@ public final class ABTestEventReportingService { DependencyInjectionService.getInstance().inject(this); } - public void initialize() { - abTestEventDao.addListener(new ModelUpdateListener() { - @Override - public void onModelUpdated(ABTestEvent model) { - if (model.getValue(ABTestEvent.REPORTED) == 1) - return; - - pushABTestEvent(model); - } - }); - } - - public void pushABTestEvent(final ABTestEvent model) { - new Thread(new Runnable() { - public void run() { - try { - JSONArray payload = new JSONArray().put(jsonFromABTestEvent(model)); - abTestInvoker.post(payload); - model.setValue(ABTestEvent.REPORTED, 1); - abTestEventDao.saveExisting(model); - } catch (JSONException e) { - handleException(e); - } catch (IOException e) { - handleException(e); - } - }; - }).start(); - } - public void pushAllUnreportedABTestEvents() { final TodorooCursor unreported = abTestEventDao.query(Query.select(ABTestEvent.PROPERTIES) - .where(ABTestEvent.REPORTED.eq(0))); + .where(ABTestEvent.REPORTED.eq(0)) + .orderBy(Order.asc(ABTestEvent.TEST_NAME)) + .orderBy(Order.asc(ABTestEvent.TIME_INTERVAL))); if (unreported.getCount() > 0) { new Thread(new Runnable() { @Override @@ -102,7 +75,7 @@ public final class ABTestEventReportingService { payload.put(KEY_VARIANT, model.getValue(ABTestEvent.TEST_VARIANT)); payload.put(KEY_NEW_USER, model.getValue(ABTestEvent.NEW_USER) > 0 ? true : false); payload.put(KEY_ACTIVE_USER, model.getValue(ABTestEvent.ACTIVATED_USER) > 0 ? true : false); - payload.put(KEY_DAYS, model.getValue(ABTestEvent.TIME_INTERVAL)); + payload.put(KEY_DAYS, new JSONArray().put(model.getValue(ABTestEvent.TIME_INTERVAL))); long date = model.getValue(ABTestEvent.DATE_RECORDED) / 1000L; payload.put(KEY_DATE, date); @@ -112,10 +85,28 @@ public final class ABTestEventReportingService { private static JSONArray jsonArrayFromABTestEvents(TodorooCursor events) throws JSONException { JSONArray result = new JSONArray(); + + String lastTestKey = ""; + JSONObject testAcc = null; for (events.moveToFirst(); !events.isAfterLast(); events.moveToNext()) { ABTestEvent model = new ABTestEvent(events); - result.put(jsonFromABTestEvent(model)); + + if (!model.getValue(ABTestEvent.TEST_NAME).equals(lastTestKey)) { + if (testAcc != null) + result.put(testAcc); + testAcc = jsonFromABTestEvent(model); + lastTestKey = model.getValue(ABTestEvent.TEST_NAME); + } else { + int interval = model.getValue(ABTestEvent.TIME_INTERVAL); + if (testAcc != null) { // this should never happen, just stopping the compiler from complaining + JSONArray daysArray = testAcc.getJSONArray(KEY_DAYS); + daysArray.put(interval); + } + } + } + if (testAcc != null) + result.put(testAcc); return result; }