Before the review, I need some feedback. Why savePing is saving pings synchronously? Is that needed?

Comment on attachment 712156 [details] [diff] [review] Patch Review of attachment 712156 [details] [diff] [review]: ----------------------------------------------------------------- savePings is saving synchronously for two reasons: 1. The tests require sync saves for various things. 2. We're not 100% certain that async saving will actually write everything to disk at shutdown. (i.e. is it guaranteed that the async disk writing gets done before we call exit()?) I'm pretty certain it does, actually, but nobody's taken the time to do it. So it'd be a matter of fixing the tests to not require sync saves--made significantly easier by TelemetryPing having a proper JS API these days--and then making the shutdown saves not require sync saves. A couple nits on the patch below. ::: toolkit/components/telemetry/TelemetryPing.js @@ +643,5 @@ > hping.add(new Date() - startTime); > > if (success) { > + this.saveFileForPing(ping).then(function(file) { > + OS.File.remove(file.path); The old code's remove call could throw; if OS.File.remove doesn't throw when the file doesn't exist, fine. Otherwise, you need to catch and ignore errors here. @@ +799,5 @@ > this._pingLoadsCompleted == this._pingsLoaded) { > Services.obs.notifyObservers(null, "telemetry-test-load-complete", null); > } > + > + setReadSavedPingSuccess(true); this.setReadSavedPingSuccess @@ +804,4 @@ > } catch (e) { > + // An error parsing the contents. > + OS.File.remove(file.path); > + setReadSavedPingSuccess(false); this.setReadSavedPingSuccess I think it's a little clearer to have the success flag set rather than two separate calls to setReadSavedPingSuccess. @@ +810,5 @@ > + > + addStreamToPendingPings: function addStreamToPendingPings(file, stream) { > + try { > + let string = NetUtil.readInputStreamToString(stream, stream.available(), { charset: "UTF-8" }); > + addToPendingPings(file, string); this.addToPendingPings @@ +814,5 @@ > + addToPendingPings(file, string); > + } catch (e) { > + // An error reading the file. > + OS.File.remove(file.path); > + setReadSavedPingSuccess(false); this.setReadSavedPingSuccess @@ +864,5 @@ > + let entries = directory.directoryEntries > + .QueryInterface(Ci.nsIDirectoryEnumerator); > + try { > + while (entries.hasMoreElements()) { > + this.loadHistograms(entries.nextFile, true); this.loadHistograms(entries.nextFile, sync); @@ +881,5 @@ > + yield iter.close(); > + > + if (entries) { > + for (let entry of entries) { > + this.loadHistograms(entry, false); Here too. @@ +887,5 @@ > + } > + } catch (e) { > + if (!(e instanceof OS.File.Error && e.becauseNoSuchFile)) { > + throw e; > + } Why are you checking for this?

Given the recent work on cleaning up Telemetry, the patch is certainly completely bitrotten. I will try and rescue that patch once bug 913070 has landed.

I completely forgot about this bug, once that bug lands, I could try and rework on this.

Comment on attachment 8357760 [details] [diff] [review] Use OS.File in Telemetry, v1 Review of attachment 8357760 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryFile.jsm @@ +15,2 @@ > > +let {Services, Deprecated, NetUtil, OS} = imports; Side-note: The source code will look nicer if you open "OS" into |File|, |Path|, |Constants|. This style is not used throughout our code, though. @@ -19,5 @@ > const PR_CREATE_FILE = 0x8; > const PR_TRUNCATE = 0x20; > const PR_EXCL = 0x80; > const RW_OWNER = parseInt("0600", 8); > const RWX_OWNER = parseInt("0700", 8); Let's rather use the constants of OS.Constants.libc. @@ +69,2 @@ > let pingString = JSON.stringify(ping); > + let array = new TextEncoder("UTF-8").encode(pingString); Actually, if you just pass the string to writeAtomic, it will be encoded to utf-8. @@ +94,3 @@ > */ > savePendingPings: function(sessionPing) { > + var p = [this.savePing(sessionPing, true)]; |let|, rather than |var|. @@ +98,4 @@ > pendingPings.forEach(function sppcb(e, i, a) { > + p.push(this.savePing(e, false)); > + }, this); > + It might be slightly clearer to use one of the following: let p = [sessionPing, ...pendingPings].map( (e) => this.savePing(e, false) ); or for (let e of [sessionPing, ...pendingPings]) { p.push(this.savePing(e, false)); } I'm not insisting, though, in particular since I don't know the number of items in pendingPings. @@ +102,2 @@ > pendingPings = []; > + return all(p); Did you mean Promise.all(p)? @@ +111,2 @@ > */ > cleanupPingFile: function(ping) { Could you either fix the issue mentioned in the FIXME or leave the FIXME comment? @@ +128,5 @@ > + var p = []; > + > + return getPingDirectory().then((directory) => { > + let entries = directory.directoryEntries > + .QueryInterface(Ci.nsIDirectoryEnumerator); While we're here, let's make this a OS.File.DirectoryIterator. Also, Task.jsm style will make things more readable. @@ +136,5 @@ > + p.push(this.loadHistograms(entries.nextFile, onLoad)); > + } > + } finally { > + entries.close(); > + return all(p); Promise.all, maybe? @@ +187,2 @@ > } > }, Unless there's a good reason to move stuff around, leave the code where it is, otherwise we lose code history information. @@ -270,2 @@ > pingsLoaded = 0; > - pingLoadsCompleted = 0; Why did you remove pingLoadsCompleted? @@ +226,3 @@ > }; > > +function getPingDirectory() { It would be nice to have two functions: one that just returns the path and one that creates the directory if it doesn't exist. Also, keep somewhere a boolean indicating whether the directory has already been created, this will save us disk I/O. @@ +229,2 @@ > let directory = Services.dirsvc.get("ProfD", Ci.nsILocalFile).clone(); > directory.append("saved-telemetry-pings"); Path.join(Constants.Path.profileDir, "saved-telemetry-pings") @@ +229,5 @@ > let directory = Services.dirsvc.get("ProfD", Ci.nsILocalFile).clone(); > directory.append("saved-telemetry-pings"); > + > + return OS.File.makeDir(directory.path, { unixMode: RWX_OWNER }). > + then(() => directory, () => directory); Don't catch errors. By default, OS.File.makeDir succeeds if the directory already exists, and we want to be informed of other errors. ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +728,2 @@ > } > + this.doPing(server, data, onSuccess.bind(this), onError.bind(this)); Could you take the opportunity to port this to Task.jsm style? Task.spawn(function*() { try { yield this.doPing(server, data) this.sendPingsFromIterator(...); } catch (ex) { // ... } }.bind(this)); This will require a simple rewrite of doPing. Also, it's almost always a good idea to return the promise. @@ +741,1 @@ > } Same here, could you return the promise? Also, you don't need that Cu.reportError. @@ +904,5 @@ > // the reason is check to make sure it's not a test-ping. > this.send("overdue-flush", this._server); > } > + }, Cu.reportError); > + This would also look nicer in Task.jsm style – which means rewriting loadSavedPings to return a promise. You also don't need the Cu.reportError. @@ +931,5 @@ > }, > > savePendingPings: function savePendingPings() { > let sessionPing = this.getSessionPayloadAndSlug("saved-session"); > + TelemetryFile.savePendingPings(sessionPing).then(null, Cu.reportError); No Cu.reportError, return the promise. @@ -992,5 @@ > }, > > - enableLoadSaveNotifications: function enableLoadSaveNotifications() { > - TelemetryFile.shouldNotifyUponSave = true; > - }, If you remove that method from the implementation, you should also remove it from the API. @@ -1012,5 @@ > }, > > - testPing: function testPing(server) { > - this.sendIdlePing(true, server); > - }, If you remove that method from the implementation, you should also remove it from the API. @@ +1069,5 @@ > // it on the next backgrounding). Not deleting it is faster, so that's what we do. > case "application-background": > if (Telemetry.canSend) { > let ping = this.getSessionPayloadAndSlug("saved-session"); > + TelemetryFile.savePing(ping, true).then(null, Cu.reportError); This Cu.reportError is not necessary. ::: toolkit/components/telemetry/nsITelemetryPing.idl @@ +47,5 @@ > + * Save histograms to a file. > + * > + * @param aFile - File to load from. > + */ > + void testSaveHistograms(in nsIFile aFile); Could you add "(for testing only)" or something such? @@ +57,2 @@ > */ > + void testLoadHistograms(in nsIFile aFile); Could you add "(for testing only)" or something such?

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #7) > Comment on attachment 8357760 [details] [diff] [review] > Use OS.File in Telemetry, v1 > > Review of attachment 8357760 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ -270,2 @@ > > pingsLoaded = 0; > > - pingLoadsCompleted = 0; > > Why did you remove pingLoadsCompleted? Because it's not used anymore.

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #8) > Because it's not used anymore. I kind of assumed that, but it was used before your patch, wasn't it?

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #10) > (In reply to Roberto Agostino Vitillo (:rvitillo) from comment #8) > > Because it's not used anymore. > > I kind of assumed that, but it was used before your patch, wasn't it? It was used to trigger a callback after all pings had been loaded from a file but since we are using promises now we don't need it anymore.

Comment on attachment 8360379 [details] [diff] [review] Use OS.File in Telemetry, v2 Review of attachment 8360379 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryFile.jsm @@ +16,5 @@ > +Cu.import("resource://gre/modules/Deprecated.jsm"); > +Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > +Cu.import("resource://gre/modules/FileUtils.jsm"); > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); You need to import into something, so please keep the |let imports = {}| above. @@ +21,3 @@ > > +const Telemetry = Cc["@mozilla.org/base/telemetry;1"] > + .getService(Ci.nsITelemetry); Services.telemetry would work, too. @@ +60,5 @@ > /** > * Save a single ping to a file. > * > * @param {object} ping The content of the ping to save. > * @param {nsIFile} file The destination file. You don't need a nsIFile here. Please use a string. @@ +70,3 @@ > let pingString = JSON.stringify(ping); > + return OS.File.writeAtomic(file.path, pingString, {tmpPath: file.path + ".tmp", > + noOverwrite: !overwrite}); Note for later: we should check the size of the file. It might be useful to lz4 it. Food for a followup bug. @@ +93,5 @@ > */ > savePendingPings: function(sessionPing) { > + let p = pendingPings.reduce((p, ping) => { > + p.push(this.savePing(ping, false)); > + return p;}, [this.savePing(sessionPing, true)]); I'm not a big fan of reduce in JS, but I won't insist. @@ +208,5 @@ > > ///// Utility functions > > +function getPingDirectoryName() { > + return OS.Path.join(OS.Constants.Path.profileDir, "saved-telemetry-pings"); Nit: It's a path rather than a name. @@ +211,5 @@ > +function getPingDirectoryName() { > + return OS.Path.join(OS.Constants.Path.profileDir, "saved-telemetry-pings"); > +} > + > +function getPingSaveFileName(ping) { Same here. @@ +213,5 @@ > +} > + > +function getPingSaveFileName(ping) { > + return OS.Path.join(getPingDirectoryName(), ping.slug); > +} Might as well make these getters or constants. @@ +237,1 @@ > } I doubt that you need FileUtils here. @@ +257,5 @@ > + onLoad(true); > + }).then(null, function onError(){ > + onLoad(false); > + return OS.File.remove(file.path); > + }); Since you're converting stuff to Task.jsm, let's do it here, too. ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +16,5 @@ > #ifndef MOZ_WIDGET_GONK > Cu.import("resource://gre/modules/LightweightThemeManager.jsm"); > #endif > Cu.import("resource://gre/modules/ThirdPartyCookieProbe.jsm"); > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); You should use Promise.jsm. @@ +139,5 @@ > + /** > + * Save histograms to a file. > + * Used only for testing purposes. > + * > + * @param aFile - File to load from. Please specify the type of the param: nsIFile or path? @@ +789,5 @@ > + > + if (success) > + deferred.resolve(); > + else > + deferred.reject(event); Style nit: we prefer if (success) { .. } else { .. } @@ -934,5 @@ > - TelemetryFile.testLoadHistograms(file, sync, (success => > - { > - let success_histogram = Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS"); > - success_histogram.add(success); > - })); You seem to have removed all instances of READ_SAVED_PING_SUCCESS. Why is that? @@ +964,4 @@ > }, > > + testSaveHistograms: function testSaveHistograms(file) { > + return TelemetryFile.savePingToFile(this.getSessionPayloadAndSlug("saved-session"), file, true); Please keep the original layout. @@ -1080,5 @@ > break; > case "profile-before-change2": > this.uninstall(); > - if (Telemetry.canSend) { > - this.savePendingPings(); So why did you move this to uninstall()? ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +18,5 @@ > Cu.import("resource://gre/modules/LightweightThemeManager.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/TelemetryPing.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); Please use Promise.jsm. @@ +69,2 @@ > function wrapWithExceptionHandler(f) { > function wrapper() { Let's take the opportunity to modernize this code into function wrapper(...args) { try { f(...args); } ... } @@ +282,2 @@ > > function runInvalidJSONTest() { Let's make this (and all other test items) calls to add_task(). To follow conventions, each test item should then be called (test_foo), e.g. add_task(function* test_invalidJSON() { // Task.jsm-style code ... }); @@ +432,2 @@ > // ensure that test runs to completion > do_register_cleanup(function () do_check_true(gFinished)); Once you have migrated stuff to add_task, the above line will probably be useless. @@ +462,1 @@ > } Let's replace this by a set of calls to add_task(), one per test. @@ +462,3 @@ > } > + > +function startServer() { Since this is only called once, you may not need a function for this. @@ +470,5 @@ > +function stopServer() { > + httpserver.stop(do_test_finished); > +} > + > +function Request() { Documentation would be necessary. This looks overcomplicated, but then, I'm not sure what it's for. ::: toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js @@ +28,5 @@ > updateAppInfo(); > > // Check that when run with no previous build ID stored, we update the pref but do not > // put anything into the metadata. > +add_task(function testFirstRun() { By convention, this should be test_firstRun. Same thing for the other tests. Also, this should be a function*.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #12) > @@ +21,3 @@ > > > > +const Telemetry = Cc["@mozilla.org/base/telemetry;1"] > > + .getService(Ci.nsITelemetry); > > Services.telemetry would work, too. Using Services.telmetry causes an exception to be thrown; ReferenceError: Telemetry is not defined > @@ -934,5 @@ > > - TelemetryFile.testLoadHistograms(file, sync, (success => > > - { > > - let success_histogram = Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS"); > > - success_histogram.add(success); > > - })); > > You seem to have removed all instances of READ_SAVED_PING_SUCCESS. Why is > that? I refactored them into addToPendingPings to don’t have to pass callbacks around and to reduce code duplication. > @@ +964,4 @@ > > }, > > > > + testSaveHistograms: function testSaveHistograms(file) { > > + return TelemetryFile.savePingToFile(this.getSessionPayloadAndSlug("saved-session"), file, true); > > Please keep the original layout. I am not really sure what you mean; I changed the name from saveHistograms to testSaveHistograms to be consistent with the naming of the other functions used only within the test suite. > @@ -1080,5 @@ > > break; > > case "profile-before-change2": > > this.uninstall(); > > - if (Telemetry.canSend) { > > - this.savePendingPings(); > > So why did you move this to uninstall()? Because it seems to be conceptually part of the uninstallation process. It also makes the event handling of "profile-after-change" and "profile-after-change2" consistent with each other. Is there a reason why it shouldn’t be merged with uninstall?

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #13) > (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from > comment #12) > > @@ +21,3 @@ > > > > > > +const Telemetry = Cc["@mozilla.org/base/telemetry;1"] > > > + .getService(Ci.nsITelemetry); > > > > Services.telemetry would work, too. > > Using Services.telmetry causes an exception to be thrown; ReferenceError: > Telemetry is not defined Services.telemetry or did you make the typo above? Anyway, that suggestion is definitely not important. > I refactored them into addToPendingPings to don’t have to pass callbacks > around and to reduce code duplication. It is my understanding that we want to upload this histogram, as all other histograms. If you believe that this is not the case, could you clear this with froydnj? > > @@ +964,4 @@ > > > }, > > > > > > + testSaveHistograms: function testSaveHistograms(file) { > > > + return TelemetryFile.savePingToFile(this.getSessionPayloadAndSlug("saved-session"), file, true); > > > > Please keep the original layout. > > I am not really sure what you mean; I changed the name from saveHistograms > to testSaveHistograms to be consistent with the naming of the other > functions used only within the test suite. According to splinter, you changed from 3 lines to 1 single loooong line. > Because it seems to be conceptually part of the uninstallation process. It > also makes the event handling of "profile-after-change" and > "profile-after-change2" consistent with each other. Is there a reason why it > shouldn’t be merged with uninstall? Not really, just curious.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #14) > > I refactored them into addToPendingPings to don’t have to pass callbacks > > around and to reduce code duplication. > > It is my understanding that we want to upload this histogram, as all other > histograms. If you believe that this is not the case, could you clear this > with froydnj? Yes, we want to upload that histogram but I didn't just remove the (2) instances of READ_SAVED_PING_SUCCESS, I moved them to a single location: addToPendingPings: testLoadHistograms() -> loadHistograms() -> addToPendingPings() -> histogram.add() loadSavedPing() -> loadHistograms() -> addToPendingPings() -> histogram.add() where histogram is Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS"). Instead of passing a callback through all those functions, we just increment that particular histogram directly in addToPendingPings.

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #15) > Yes, we want to upload that histogram but I didn't just remove the (2) > instances of READ_SAVED_PING_SUCCESS, I moved them to a single location: > addToPendingPings: > > testLoadHistograms() -> loadHistograms() -> addToPendingPings() -> > histogram.add() > loadSavedPing() -> loadHistograms() -> addToPendingPings() -> histogram.add() > > where histogram is Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS"). > Instead of passing a callback through all those functions, we just increment > that particular histogram directly in addToPendingPings. Ah, right, I had missed it.

Comment on attachment 8363582 [details] [diff] [review] Use OS.File in Telemetry, v3 Review of attachment 8363582 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with a few minor changes, some more documentation on the tests you have refactored and the answer to a few questions. ::: toolkit/components/telemetry/TelemetryFile.jsm @@ +125,4 @@ > */ > + loadSavedPings: function() { > + return Task.spawn(function*() { > + let directory = yield createPingDirectory(); I'd rather you didn't create the directory just for the sake of reading: - create the iterator as you do below; - use iter.exists() to find out whether the directory exists; - if the directory doesn't exist, well, there is nothing to do. @@ +143,5 @@ > * > * Once loaded, the saved pings can be accessed (destructively only) > * through |popPendingPings|. > * > + * @param {OS.File.DirectoryIterator.Entry} file The file to load. I'd rather you passed directly the path. This would be more generic. @@ +218,2 @@ > > +function createPingDirectory() { Maybe getPingDirectory(), since it generally doesn't create the directory. @@ +231,2 @@ > > +function addToPendingPings(file) { Please pass the path, rather than the file. ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +1036,4 @@ > }, > > cacheProfileDirectory: function cacheProfileDirectory() { > // This method doesn't do anything anymore Note: once we get rid of the .idl, we should also get rid of this method. ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ -46,2 @@ > > -function test_expired_histogram() { Why did you remove that test? @@ -345,5 @@ > - httpserver.stop(do_test_finished); > - // Even though we have had four successful pings when this handler is > - // run, we only had three successful pings when the histograms were > - // saved. > - checkPayload(request, "saved-session", 3); I don't see that being tested anymore. Is it just because we don't have a sync version anymore? @@ -354,5 @@ > -} > - > -function checkHistogramsAsync(request, response) { > - registerPingHandler(checkPersistedHistogramsAsync); > - checkPayload(request, "test-ping", 3); Same question. @@ +402,1 @@ > It would be great if you could now document each test: what does it test? how does the non-trivial stuff work? @@ +402,2 @@ > > +add_task(function* test_ExpiredHistogram() { Nit: test_expiredHistogram @@ -513,2 @@ > > - addWrappedObserver(nonexistentServerObserver, "telemetry-test-xhr-complete"); Have you removed that test? ::: toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js @@ +28,5 @@ > updateAppInfo(); > > // Check that when run with no previous build ID stored, we update the pref but do not > // put anything into the metadata. > +add_task(function* test_sirstRun() { firstRun, maybe?

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #18) > Comment on attachment 8363582 [details] [diff] [review] > Use OS.File in Telemetry, v3 > > Review of attachment 8363582 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: toolkit/components/telemetry/TelemetryPing.jsm > @@ +1036,4 @@ > > }, > > > > cacheProfileDirectory: function cacheProfileDirectory() { > > // This method doesn't do anything anymore > > Note: once we get rid of the .idl, we should also get rid of this method. I removed that method since the patch now depends on Bug 960966. > > ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js > @@ -46,2 @@ > > > > -function test_expired_histogram() { > > Why did you remove that test? I didn't remove it, I moved it down with the other tasks. Since the tasks are executed in order, putting them all together at the bottom seems more readable to me. > > @@ -345,5 @@ > > - httpserver.stop(do_test_finished); > > - // Even though we have had four successful pings when this handler is > > - // run, we only had three successful pings when the histograms were > > - // saved. > > - checkPayload(request, "saved-session", 3); > > I don't see that being tested anymore. Is it just because we don't have a > sync version anymore? Right, I removed all sync tests. > @@ -354,5 @@ > > -} > > - > > -function checkHistogramsAsync(request, response) { > > - registerPingHandler(checkPersistedHistogramsAsync); > > - checkPayload(request, "test-ping", 3); > > Same question. I moved that tests down with the other ones. > @@ -513,2 @@ > > > > - addWrappedObserver(nonexistentServerObserver, "telemetry-test-xhr-complete"); > > Have you removed that test? That test is now called test_noServerPing().

Rebased on Bug 960966

Comment on attachment 8364471 [details] [diff] [review] Use OS.File in Telemetry, v5 Review of attachment 8364471 [details] [diff] [review]: ----------------------------------------------------------------- Let's go :)

Backed out for xpcshell failures. https://hg.mozilla.org/integration/fx-team/rev/681f9c28ed07 https://tbpl.mozilla.org/php/getParsedLog.php?id=33628244&tree=Fx-Team

Restoring the TelemetryPing.uninstall() logic as it was originally fixes the issue. https://tbpl.mozilla.org/?tree=Try&rev=1061f0d8a4c6