Issue 394 - hit statistics tool data collection

lib/filterHits.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 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 {Prefs} = require("prefs");
+let {Utils} = require("utils");
+let {MILLIS_IN_DAY} = require("downloader");
+let {FilterNotifier} = require("filterNotifier");
+let {DownloadableSubscription} = require("subscriptionClasses");
+
+/**
+ * This class reads filter hits statistics from SQLite database,
+ * manages them in memory and writes them back.
+ * @class
+ */
+let FilterHits = exports.FilterHits =
+{
+  /**
+   * Data that shoud be sent to the server
+   * @type Object
+   */
+  filters: Object.create(null),
+
+  /**
+   * Time since last push
+   * @type Number
+   */

[kzar, 2015/04/14 16:33:30]

I guess we don't need to document these private variables, they're not part of
an API that we need to generate documentation for and they're all fairly
self-explanatory.

[saroyanm, 2015/04/16 10:42:59]

Please have a look on current discussion:
http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/...
I think documenting shouldn't harm anyway and from consistency point of view I
think it make sense to leave them here.

[kzar, 2015/04/16 12:46:26]

I disagree on this one, I've been told not to over-comment before.

[saroyanm, 2015/04/16 14:08:45]

I guess sometimes would be nice to have link to that discussion, to understand
if that is general and cover this case, or special case.
So we have two thoughts, let's keep it like this for now and align with
Wladimir, as well.

+  _lastPush: 0,
+
+  /**
+   * Indicates the timeframe between pushes
+   * @type Number
+   */
+  _pushInterval: MILLIS_IN_DAY * 7,
+
+  /**
+   * Indicates whether the data is being loaded from storage
+   * @type Boolean
+   */
+  _loading: false,
+
+  /**
+   * Indicates whether the data is being saved to storage
+   * @type Boolean
+   */
+  _saving: false,
+
+  /**
+   * Indicates whether the data is being sent to the server
+   * @type Boolean
+   */
+  _sending: false,
+
+  /**
+   * Increases the filter hit count

[kzar, 2015/04/14 16:33:30]

Perhaps "Increases the hit count for the given filter"?

[saroyanm, 2015/04/16 10:42:59]

No actually the filter in the parameter is the filter object that has been hit,
so I'm using that object to update the "filters" object in the memory.

[kzar, 2015/04/16 12:46:26]

I think you've slightly missed my point here. We are increasing the hit count
for the specified filter, as opposed to all filters for example. (Sure the hit
count is not stored in the filter itself. Feel free to clarify where the hit
count we're increasing is stored too if you like, but that's something else.)

Anyway it's not hugely important either way I suppose.

+   * @param {Filter} filter
+   * @param {Window} window  Window that the match originated in (required to get host)
+   */
+  increaseFilterHits: function(filter, wnd)

[kzar, 2015/04/14 16:33:30]

Maybe we should just make host the parameter here, as that's all we need,
instead of passing the whole Window object through?

[saroyanm, 2015/04/16 10:42:59]

Done.

+  {
+    let subscriptions = filter.subscriptions;
+    let inDownloadableSubscription = false;
+    for (let i = 0; i < subscriptions.length; i++)
+    {
+      if (subscriptions[i] instanceof DownloadableSubscription)
+      {
+        inDownloadableSubscription = true;
+        break;
+      }
+    }
+
+    if (!inDownloadableSubscription)
+      return;
+
+    if (!(filter.text in this.filters))
+      this.filters[filter.text] = Object.create(null);
+
+    let statFilter = this.filters[filter.text];
+    let filterType = filter.thirdParty ? "thirdParty" : "firstParty";
+
+    if (!(filterType in statFilter))
+      statFilter[filterType] = Object.create(null);
+
+    if (!("subscriptions" in statFilter))
+      statFilter.subscriptions = [];
+
+    for (let i = 0; i < subscriptions.length; i++)
+    {
+      if (subscriptions[i] instanceof DownloadableSubscription
+          && statFilter.subscriptions.indexOf(subscriptions[i].url) == -1)
+        statFilter.subscriptions.push(subscriptions[i].url);
+    }
+
+    let wndLocation = Utils.getOriginWindow(wnd).location.href;
+    let host = Utils.unwrapURL(wndLocation).host;
+
+    if (!(host in statFilter[filterType]))
+      statFilter[filterType][host] = {hits: 1, latest: filter.lastHit};

[kzar, 2015/04/14 16:33:30]

I think we're supposed to use braces consistently within if...else statements.
So as you need the braces for the else clause, add them for the if clause too.

[saroyanm, 2015/04/16 10:42:59]

I'm not sure if be honest, I can see your point, but we don't have any guideline
that explicitly says that we should go one or other way, please have a look in
the link below:
https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/contentPolicy.js...

Both for this and for documenting would be really nice to have a guideline, if
be honest not sure, I'll be okey in both ways.

[kzar, 2015/04/16 12:46:26]

I've definitely been told that the braces should be consistent within if...else
clauses on other code reviews.

[saroyanm, 2015/04/16 14:08:45]

Would be nice to have some reference for future, feel free to send link with
relevant discussion. 
Anyway I've updated for consistency as you suggested.

+    else
+    {
+      statFilter[filterType][host].hits++;
+      statFilter[filterType][host].latest = filter.lastHit;
+    }
+  },
+
+  resetFilterHits: function()
+  {
+    this.filters = Object.create(null);
+    this.saveFilterHitsToDatabase();
+  },
+
+  sendFilterHitsToServer: function()
+  {
+    if (!Prefs.sendstats)
+      return;
+
+    let request = new XMLHttpRequest();
+    request.open("POST", Prefs.sendstats_url);
+    request.setRequestHeader("Content-Type", "application/json");
+    request.addEventListener("load", function(event)
+    {
+      FilterHits._sending = false;
+      if (request.status >= 200 && request.status < 300)
+      {
+        FilterHits._lastPush = new Date().getTime();
+        FilterHits.resetFilterHits();
+      }
+      else

[kzar, 2015/04/14 16:33:30]

Again here with the if...else brace consistency. Probably elsewhere too, I'll
stop adding comments for these.

[saroyanm, 2015/04/16 10:42:59]

Same as my reply above.

+        Cu.reportError("Could not send filter hit statistics to Adblock Plus server.");

[kzar, 2015/04/14 16:33:30]

If sending fails and it's the server's fault / a connection problem will we
retry later?

[saroyanm, 2015/04/16 10:42:59]

Yes, but it depends on filterStorage action. 

Here is how the saving works now:
We have setDirty indicator which decide when we should save the data to the
patterns.ini and when the data should be sent to the server:
http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/...

this indicator reset after successfuly save to patterns.ini. 
So in general it should dependent whether data was saved to patterns.ini. So in
case the data is successfully saved to patterns.ini we will have next request to
send the data to server after 500 hits, in case it's not been saved we will send
the data after each hit in case we are not waiting for the response from the
server, please note also we will try to save the data to patterns.ini until it's
been saved successfully.

+    }, false);
+
+    let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");

[kzar, 2015/04/14 16:33:30]

Possible to add a newline in this list? It seems like quite a long line.

[saroyanm, 2015/04/16 10:42:59]

Couldn't find any documentation that explicitly says so but looks like it
possible to have multiple lines.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-assign...

Done.

[kzar, 2015/04/16 12:46:26]

It is possible, I was just being polite. (British thing I guess.) Thanks :)

[saroyanm, 2015/04/16 14:08:45]

Ha ha :) That's funny :D

+    let data = {
+      version: 1,
+      timeSincePush: this._lastPush,
+      addonName: addonName,
+      addonVersion: addonVersion,
+      application: application,
+      applicationVersion: applicationVersion,
+      platform: platform,
+      platformVersion: platformVersion,
+      filters: this.filters
+    };
+
+    this._sending = true;
+    request.send(JSON.stringify(data));
+  },
+
+  getStorageFile: function()
+  {
+    return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]);
+  },
+
+  checkCreateTable: function(connection)
+  {
+    if (!connection.tableExists("filterhits"))
+      connection.executeSimpleSQL("CREATE TABLE filterhits (id INTEGER PRIMARY KEY, filters TEXT, date INTEGER)");
+  },
+
+  /**
+   * Load Filter hits from database
+   */
+  loadFilterHitsFromDatabase: function()
+  {
+    let storageFile = this.getStorageFile();
+    if (!storageFile)
+      return;
+
+    let connection = Services.storage.openDatabase(storageFile);
+    this.checkCreateTable(connection);
+
+    let statement = connection.createStatement("SELECT * FROM filterhits");
+    if (!this._loading)
+    {
+      this._loading = true;
+      statement.executeAsync(
+      {
+        handleResult: function(results)
+        {
+          let row = results.getNextRow();
+          if (row)
+          {
+            let filters = row.getResultByName("filters");
+            let lastDate = row.getResultByName("date");
+            FilterHits.filters = JSON.parse(filters);

[kzar, 2015/04/14 16:33:30]

We are serialising some of the data into JSON and then storing that in the
database? Shouldn't we instead serialise everything and store a JSON file or
avoid serialising anything and properly structure the database?

[saroyanm, 2015/04/16 10:42:59]

The idea was to use Storage API which is actually SQLlite database.
Maybe using json file will be more clear, but make sense to discuss using
filestorage with @Wladimir, as it was his idea to use Storage API.
But meanwhile I don't understand why we need proper structure if we are not
going to make search in that ? Can you please let me know why you will prefare
some other structure instead of serializing and deserializing JSON object in
this particular case ?

[kzar, 2015/04/16 12:46:26]

Well it just seems like a hack to mix them both to me. Either serialize
everything as JSON and store that or use a database and structure it properly.
I've seen too much hacked together PHP+MySQL code in the past where data is just
serialized and shoved into some string DB field to think this is a good idea.
Maybe Wladimir will disagree, probably best to see what he thinks here as it's a
potentially big change.

+            FilterHits._lastPush = lastDate;
+          }
+        },
+
+        handleError: function(error)
+        {
+          Cu.reportError(error.message);
+        },
+
+        handleCompletion: function(reason)
+        {
+          if (reason != Ci.mozIStorageStatementCallback.REASON_FINISHED)
+            Cu.reportError("Loading of filter hits canceled or aborted.");
+          FilterHits._loading = false;
+        }
+      });
+    }
+
+    connection.asyncClose();
+  },
+
+  /**
+   * Save Filter hits to database
+   */
+  saveFilterHitsToDatabase: function()
+  {
+    let now = new Date().getTime();
+    if (!this._lastPush)
+      this._lastPush = now;
+
+    if (!this._sending && now - this._lastPush > this._pushInterval)

[kzar, 2015/04/14 16:33:30]

I think that perhaps the logic here for sending the data belongs elsewhere. It
seems counter intuitive that a function to save data to the database would
occasionally trigger posting data. (Also it means we do not honour the push
interval very accurately.)

[saroyanm, 2015/04/16 10:42:59]

Yes you are right this should go elsewhere, 
but before moving it somewhere I would like to consider two scenarios:
1. This can go to filterListener.js right before saveFilterHitsToDatabase metod
call.
The interval will work same way as it described in my reply above.
2. This can go to increaseFilterHits method:
We will check for the date on every hit, but it will have small impact on
performance, I think maybe we should consider also small performance difference
in this case, even if we will not honor the push interval very careful anyway
the data will be sent, please note if the user will not turn on his computer for
several days the data will again not be sent, so I think it doesn't make really
sense to be so cautious regarding the interval. 

So this is two scenarios I'm considering. Please let me know what you think,
before performing this changes, because could be I'm not really understand why
we are trying to be so insistent regarding the 1 week interval.

[kzar, 2015/04/16 12:46:26]

The geometrical mean calculation on the server takes into account the submission
interval. If the submission interval varies a lot the results will be less
accurate. I'm not sure how much this matters, or what the best approach is here
though however. I think it's best to see what Wladimir thinks.

+    {
+      this.sendFilterHitsToServer();

[kzar, 2015/04/14 16:33:30]

If we are going to perform the web request here shouldn't it be at the end of
the function at least? It seems even weirder that a function to save data to the
database _doesn't_ save data to the database when it decides it should post some
data.

[saroyanm, 2015/04/16 10:42:59]

Yes it doesn't because after the successful posting the data should be reset, I
can't see point why we should save them back it's additional action that can
cost some performance, not a big deal, but I would rather maybe vote for this
solution.

[kzar, 2015/04/16 12:46:26]

I guess we can ignore this one for now as it will no longer be relevant once
data posting is triggered from elsewhere.

[saroyanm, 2015/04/16 14:08:45]

Yes agree.

+      return;
+    }
+
+    let storageFile = this.getStorageFile();
+    if (!storageFile)
+      return;
+
+    let connection = Services.storage.openDatabase(storageFile);
+    this.checkCreateTable(connection);
+
+    let statement = connection.createStatement("INSERT OR REPLACE INTO filterhits (id, filters, date) VALUES(0, :filters, :date)");
+    statement.params.filters = JSON.stringify(this.filters);
+    statement.params.date = this._lastPush;
+    if (!this._saving)
+    {
+      this._saving = true;
+      statement.executeAsync(
+      {
+        handleError: function(aError)
+        {
+          Cu.reportError(aError.message);
+        },
+
+        handleCompletion: function(aReason)
+        {
+          if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED)
+            Cu.reportError("Writing of filter hits canceled or aborted.");
+          FilterHits._saving = false;
+        }
+      });
+    }
+
+    connection.asyncClose();
+  }
+};
+
+FilterNotifier.addListener(function(action)
+{
+  if (action == "load" && Prefs.sendstats)
+    FilterHits.loadFilterHitsFromDatabase();
+});