Issue 394 - hit statistics tool data collection

lib/filterHits.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+let {Services} = Cu.import("resource://gre/modules/Services.jsm", null);
+let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
+let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", null);
+
+let {Prefs} = require("prefs");
+let {Utils} = require("utils");
+let {MILLIS_IN_DAY} = require("downloader");
+let {FilterNotifier} = require("filterNotifier");
+let {DownloadableSubscription} = require("subscriptionClasses");
+
+/**
+ * This class collect filter hits statistics in SQLite database
+ * and send them to the server when user opt-in for that

[Wladimir Palant, 2016/02/29 14:35:55]

Nit: collect => collects, in SQLite => in an SQLite, send => sends, when user
opt-in => if the user opts into (also, period at the end of sentence please).

[saroyanm, 2016/03/18 18:23:13]

Done.

+ * @class
+ */
+let FilterHits = exports.FilterHits =
+{
+  /**
+   * Indicates the timeframe between pushes
+   * @type Number
+   */
+  _pushInterval: MILLIS_IN_DAY * 7,

[Wladimir Palant, 2016/02/29 14:35:50]

That's a constant, it really shouldn't be declared as a property. How about:

  const PUSH_INTERVAL = MILLIS_IN_DAY * 7;

(at the top of the file of course).

[saroyanm, 2016/03/18 18:23:30]

Done.

+
+  /**
+   * The value of storage statement normal execution constant 
+   * @type Number
+   */
+  _finished: Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED,

[Wladimir Palant, 2016/02/29 14:35:48]

Same here, constants shouldn't be properties.

[saroyanm, 2016/03/18 18:23:23]

Done.

+
+  /**
+   * Indicates whether the data is being sent to the server 
+   * @type Boolean
+   */
+  _sending: false,
+
+  /**
+   * Called on module startup.
+   */
+  init: function()
+  {
+    Prefs.addListener(function(name)
+    {
+      if (name == "sendstats")
+      {
+        setTimeout(function()

[Wladimir Palant, 2016/02/29 14:35:57]

Nit: Creating a number of timers running in parallel (and potentially clearing
the filter hits multiple times) isn't nice. Usual pattern is:

if (this._resetTimeout)
  clearTimeout(this._resetTimeout);
this._resetTimeout = setTimeout(() => {
  ...
});

[saroyanm, 2016/03/18 18:23:12]

Done.

+        {
+          if (!Prefs[name])

[Wladimir Palant, 2016/02/29 14:35:56]

Here as well, just use Prefs.sendstats please - it's not some generic pref you
are looking at.

[saroyanm, 2016/03/18 18:23:21]

Done.

+            this.resetFilterHits();
+        }.bind(this), 1000);
+      }
+    }.bind(this));

[Wladimir Palant, 2016/02/29 14:35:53]

bind(this) shouldn't be necessary in most cases, just use arrow functions:

Prefs.addListener(name => {
  ...
});

[saroyanm, 2016/03/18 18:23:31]

Done.

+
+    let now = new Date().getTime();
+    if (now && (now - Prefs.sendstats_last_push > this._pushInterval))
+      this.sendFilterHitsToServer();

[Wladimir Palant, 2016/02/29 14:35:55]

Please use Date.now(). Also, it's a safe bet that it will return something
meaningful (meaning: not zero).

We shouldn't be sending the data immediately on startup, the browser is busy
already. We should rather wait a few minutes (5?). Also, if the user keeps the
browser open forever we will never send right now. I guess that the logic should
go like this:

if (now - last_push >= PUSH_INTERVAL)
  send();
else
  setTimeout(send, PUSH_INTERVAL - now + last_push);

Finally, what will happen if the user sets his clock to next year, sends the
data and then resets the clock to the correct time? Prefs.sendstats_last_push
will point to next year and we will wait a year before sending again. If you see
Prefs.sendstats_last_push being higher than now you need to correct it.

Ideally, we would just use Downloader here, it already has all the necessary
checks. This would require adding ability to send data there, also some way to
specify that 204 is ok but otherwise it should work just fine. Various
parameters like addonName would move from the payload into the URL but that
should be ok.

[saroyanm, 2016/03/18 18:23:16]

I'm not sure if it's a good idea to change the Downloader in the way that it
also will send data, but I added ability to set a callback function that will be
triggered instead of _download function, so we can separate the logic of sending
the data from the logic of the time calculation, so we can send the data in the
format we want. 
Also Downloader requires initialDelay which will fire every time user will
launch the browser and checkInterval will be retested, so I assume it's not
possible to make all the integration of sending the data in the Downloader. 
I hope that my current implementation make sense, I've made it similar to how
notifications initialize the Downloader, but not really sure if my
implementation is correct.

Also the features that I use from Downloader I assume it's easy to implement in
Filter hits statistics, so please let me know if it's only about the initial
check interval and recurrent check I think we shouldn't do anything in
Downloader, so yes I'll be happy to revert all the changes in Downloader and
implement that both in the Filter hits.

+  },
+
+  /**
+   * Increases the filter hit count
+   * @param {Filter} filter
+   * @param {String} host 
+   */
+  increaseFilterHits: function(filter, host)
+  {
+    let subscriptions = filter.subscriptions;
+    let downloadableSubscriptions = [];
+    for (let i = 0; i < subscriptions.length; i++)
+    {
+      if (subscriptions[i] instanceof DownloadableSubscription)
+        downloadableSubscriptions.push(subscriptions[i].url);
+    }
+
+    if (downloadableSubscriptions.length == 0)
+      return;
+
+    let storageFile = this.getStorageFile();
+    if (!storageFile)
+      return;
+
+    let connection = Services.storage.openDatabase(storageFile);
+    this.checkCreateTable(connection);

[Wladimir Palant, 2016/02/29 14:35:51]

Should we be doing this on every filter hit? Initializing the connection should
be a one-time thing, after that we should merely be using it.

Probably a good idea to use memoization here, something like this:

get connection()
{
  let connection = ...;
  Object.defineProperty(this, "connection", {
    enumerable: true,
    value: connection
  });
  return connection;
},

This will make sure we don't initialize the database unless we actually need it.

[saroyanm, 2016/03/18 18:23:24]

Good point, I've cached connection opening, but still checking for the table, I
assume your comment were related to the upper row(97).

+
+    let statementText = "INSERT OR REPLACE INTO filterhits " +
+      "(filter, host, firstParty, hitCount, lastHit, subscriptions) VALUES " +
+      "(:filter, :host, :firstParty, coalesce ((SELECT hitCount " +
+      "FROM filterhits WHERE filter=:filter AND host=:host AND " +
+      "firstParty=:firstParty), 0) + 1, :lastHit, :subscriptions)";
+
+    let statement = connection.createStatement(statementText);
+    statement.params.filter = filter.text;
+    statement.params.host = host;
+    statement.params.lastHit = filter.lastHit;
+    statement.params.firstParty = filter.thirdParty != "thirdParty";

[Wladimir Palant, 2016/02/29 14:35:52]

This is a filter flag you are checking here, it only tells you whether the
filter is supposed to match first-party, third-party or any website. We already
know that from the filter text. It tells you nothing about this particular match
however.

You'll need an additional parameter to increaseFilterHit I guess, simply pass
thirdParty from the content policy.

[saroyanm, 2016/03/18 18:23:27]

Done.

+    statement.params.subscriptions = JSON.stringify(downloadableSubscriptions);
+
+    statement.executeAsync(
+    {
+      handleError: function(aError)

[Wladimir Palant, 2016/02/29 14:35:45]

Nit: we don't use Hungarian notation for parameters, it should be error here.
Similarly, reason rather than aReason below.

[saroyanm, 2016/03/18 18:23:14]

Done.

+      {
+        Cu.reportError(aError.message);
+      },
+
+      handleCompletion: function(aReason)
+      {
+        if (aReason != this._finished)
+          Cu.reportError("Writing of filter hits canceled or aborted.");

[Wladimir Palant, 2016/02/29 14:35:58]

Maybe add aReason to the text, so that we can look up which one it was?

[saroyanm, 2016/03/18 18:23:28]

Done.

+      }.bind(this)

[Wladimir Palant, 2016/02/29 14:35:49]

Please get rid of bind(this) here as well.

[saroyanm, 2016/03/18 18:23:22]

Done.

+    });
+
+    connection.asyncClose();
+  },
+
+  /**
+   * Remove all local collected filter hits data
+   */
+  resetFilterHits: function()
+  {
+    let storageFile = this.getStorageFile();
+    if (!storageFile)
+      return;
+
+    let connection = Services.storage.openDatabase(storageFile);
+    connection.executeSimpleSQL("DROP TABLE IF EXISTS filterhits");

[Wladimir Palant, 2016/02/29 14:35:56]

Probably better to leave the database initialized and merely remove all entries,
otherwise you'll always have to check whether the table exists.

[saroyanm, 2016/03/18 18:23:29]

Done.

+  },
+
+  /**
+   * Send local stored filter hit data to the 
+   * service specified in Prefs.sendstats_url
+   */
+  sendFilterHitsToServer: function()
+  {
+    if (!Prefs.sendstats)
+      return;
+
+    let storageFile = this.getStorageFile();
+    if (!storageFile)
+      return;
+
+    let connection = Services.storage.openDatabase(storageFile);
+    this.checkCreateTable(connection);
+
+    let statement = connection.createStatement("SELECT * FROM filterhits");
+    statement.executeAsync(
+    {
+      handleResult: function(aResultSet)
+      {
+        let filters = Object.create(null);
+        for (let row = aResultSet.getNextRow(); row;
+             row = aResultSet.getNextRow())
+        {
+
+          let filterText = row.getResultByName("filter");
+          let host = row.getResultByName("host");
+          let filterType = (row.getResultByName("firstParty") == 1 ? 

[Wladimir Palant, 2016/02/29 14:35:52]

a) Not filterType, matchType.
b) Don't compare to 1, true can be represented as different integers. Just
(row.getResultByName("thirdParty") ? "thirdParty" : "firstParty") will do.

[saroyanm, 2016/03/18 18:23:22]

Done.

+                            "firstParty" : "thirdParty");
+          let hitCount = row.getResultByName("hitCount");
+          let lastHit = row.getResultByName("lastHit");
+          let subscriptions = row.getResultByName("subscriptions");
+          
+          if (!(filterText in filters))
+           filters[filterText] = Object.create(null);
+          
+          let filter = filters[filterText];
+          
+          if (!filter[subscriptions])
+            filter[subscriptions] = subscriptions;

[Wladimir Palant, 2016/02/29 14:35:53]

You probably meant filter.subscriptions here.

In general, I wonder whether it's a good idea to store subscriptions with each
single entry. It should really be an additional table, it can be called
filtersubscriptions.

[saroyanm, 2016/03/18 18:23:26]

Done, but not sure if current implementation is what you meant, because it's not
proper relation, I could create a separate table that will grow with each
filter, but I decided to stick with current solution, not to make the table grow
while we expect user not have more than 5 filter lists usually, but let me know
if you consider the 3 table solution better, so I can update the implementation.

+
+          if (!(filterType in filter))
+            filter[filterType] = Object.create(null);
+          
+          if (!(host in filter[filterType]))

[Wladimir Palant, 2016/02/29 14:35:50]

This check is pointless, filter/type/host combination is guaranteed to be unique
- so just store the data without checking.

[saroyanm, 2016/03/18 18:23:16]

Done.

+            filter[filterType][host] = {"hits": hitCount, latest: lastHit};
+        }
+
+        let request = new XMLHttpRequest();
+        request.open("POST", Prefs.sendstats_url);
+        request.setRequestHeader("Content-Type", "application/json");
+        request.addEventListener("load", function(event)

[Wladimir Palant, 2016/02/29 14:35:58]

You should handle "error" event as well, and be it in order to log an error
message.

[saroyanm, 2016/03/18 18:23:27]

Done.

+        {
+          FilterHits._sending = false;

[Wladimir Palant, 2016/02/29 14:35:57]

Why are you setting this member? It isn't being used for anything. If you need
it you should reset it in case of error as well however.

[saroyanm, 2016/03/18 18:23:15]

Done.

+          if (request.status >= 200 && request.status < 300)
+          {
+            Prefs.sendstats_last_push = new Date().getTime();
+            FilterHits.resetFilterHits();
+          }
+          else
+          {
+            Cu.reportError("Couldn't send filter hit statistics to server.");

[Wladimir Palant, 2016/02/29 14:35:54]

I think that this should be more precise - server responded with a status code
that we didn't expect. The message should actually include that status code in
order to simplify debugging.

[saroyanm, 2016/03/18 18:23:24]

Done.

+          }
+        }, false);
+
+        let {addonName, addonVersion, application, 
+            applicationVersion, platform, platformVersion} = require("info");
+        let data = {
+          version: 1,
+          timeSincePush: Prefs.sendstats_last_push,

[Wladimir Palant, 2016/02/29 14:35:54]

That's the time of last push, not the time *since* the last push ;)

Also, it seems that the results of
https://intraforum.adblockplus.org/t/q4-kick-off-telemetry-filter-hit-statist...
didn't make it into the issue description. This should be rounded to hours,
along these lines:

  function roundToHours(timestamp)
  {
    return Math.round(timestamp / MILLIS_IN_HOUR) * MILLIS_IN_HOUR;
  }

The same function should be applied to filter.lastHit, *before* storing it in
the database.

[saroyanm, 2016/03/18 18:23:30]

Done, but I can't understand why we need to use this function also on
filter.lastHit ?
Also I assume you meant to multiply the rounded hour by the amount of seconds in
hour.

+          addonName: addonName,
+          addonVersion: addonVersion,
+          application: application,
+          applicationVersion: applicationVersion,
+          platform: platform,
+          platformVersion: platformVersion,
+          filters: filters
+        };
+
+        this._sending = true;
+        request.send(JSON.stringify(data));
+      }.bind(this),
+      handleError: function(aError)
+      {
+        Cu.reportError(aError.message);
+      },
+      handleCompletion: function(aReason)
+      {
+        if (aReason != this._finished)
+          Cu.reportError("Loading of filter hits canceled or aborted.");
+      }.bind(this)
+    });
+    connection.asyncClose();
+  },
+
+  /**
+   * @return nsIFile SQLite database file with filter hits data
+   */
+  getStorageFile: function()
+  {
+    return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]);

[Wladimir Palant, 2016/02/29 14:35:51]

Please don't create files all over the user's profile, this should be in our
directory:

  let file = IO.resolveFilePath(Prefs.data_directory);
  file.append("adblockplus.sqlite");

FilterStorage invests some more effort into this, shouldn't be necessary here
however - this is a non-essential feature. Just make sure we don't spam the
Error Console if IO.resolveFilePath() returns null or we cannot create the
database because of file permission issues. One error message is ok, just not on
every filter hit.

[saroyanm, 2016/03/18 18:23:14]

Done.

+  },
+
+  /**
+   * Creates table in local SQLite database for storing 
+   * filter hit data if the does not exists yet
+   */
+  checkCreateTable: function(connection)
+  {
+    if (!connection.tableExists("filterhits"))
+    {
+      connection.executeSimpleSQL("CREATE TABLE filterhits (filter TEXT, " +
+        "host TEXT, firstParty INTEGER, hitCount INTEGER, lastHit INTEGER, " +

[Wladimir Palant, 2016/02/29 14:35:44]

We always have thirdParty as a flag, please use it here as well. Having to write
|statement.firstParty = !thirdParty| doesn't make much sense.

[saroyanm, 2016/03/18 18:23:25]

Done.

+        "subscriptions TEXT, PRIMARY KEY(filter, host, firstParty))");
+      Prefs.sendstats_last_push = new Date().getTime();
+    }
+  },
+};
+
+FilterHits.init();