From 86e7bf1e468907ed24ebd33c632834d9adeff480 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Thu, 16 Jan 2020 19:09:49 +0000 Subject: [PATCH] Bug 1607908 - Add support for sending multiple pings via a single invocation of the pingsender r=chutten Differential Revision: https://phabricator.services.mozilla.com/D59705 --HG-- extra : moz-landing-system : lando --- .../telemetry/app/TelemetrySend.jsm | 23 +-- .../telemetry/pingsender/pingsender.cpp | 143 +++++++++++------- .../telemetry/tests/unit/test_PingSender.js | 98 ++++++++---- 3 files changed, 164 insertions(+), 100 deletions(-) diff --git a/toolkit/components/telemetry/app/TelemetrySend.jsm b/toolkit/components/telemetry/app/TelemetrySend.jsm index 0e785f00027f..10ec715bf59c 100644 --- a/toolkit/components/telemetry/app/TelemetrySend.jsm +++ b/toolkit/components/telemetry/app/TelemetrySend.jsm @@ -312,20 +312,22 @@ var TelemetrySend = { * This method is currently exposed here only for testing purposes as it's * only used internally. * - * @param {String} aUrl The telemetry server URL - * @param {String} aPingFilePath The path to the file holding the ping - * contents, if if sent successfully the pingsender will delete it. + * @param {Array} pings An array of objects holding url / path pairs + * for each ping to be sent. The URL represent the telemetry server the + * ping will be sent to and the path points to the ping data. The ping + * data files will be deleted if the pings have been submitted + * successfully. * @param {callback} observer A function called with parameters - (subject, topic, data) and a topic of "process-finished" or - "process-failed" after pingsender completion. + * (subject, topic, data) and a topic of "process-finished" or + * "process-failed" after pingsender completion. * * @throws NS_ERROR_FAILURE if we couldn't find or run the pingsender * executable. * @throws NS_ERROR_NOT_IMPLEMENTED on Android as the pingsender is not * available. */ - testRunPingSender(url, pingPath, observer) { - return TelemetrySendImpl.runPingSender(url, pingPath, observer); + testRunPingSender(pings, observer) { + return TelemetrySendImpl.runPingSender(pings, observer); }, }; @@ -951,7 +953,7 @@ var TelemetrySendImpl = { ); try { const pingPath = OS.Path.join(TelemetryStorage.pingDirectoryPath, pingId); - this.runPingSender(submissionURL, pingPath); + this.runPingSender([{ url: submissionURL, path: pingPath }]); } catch (e) { this._log.error("_sendWithPingSender - failed to submit ping", e); } @@ -1545,7 +1547,7 @@ var TelemetrySendImpl = { }; }, - runPingSender(url, pingPath, observer) { + runPingSender(pings, observer) { if (AppConstants.platform === "android") { throw Cr.NS_ERROR_NOT_IMPLEMENTED; } @@ -1556,12 +1558,13 @@ var TelemetrySendImpl = { let exe = Services.dirsvc.get("GreBinD", Ci.nsIFile); exe.append(exeName); + let params = pings.flatMap(ping => [ping.url, ping.path]); let process = Cc["@mozilla.org/process/util;1"].createInstance( Ci.nsIProcess ); process.init(exe); process.startHidden = true; process.noShell = true; - process.runAsync([url, pingPath], 2, observer); + process.runAsync(params, params.length, observer); }, }; diff --git a/toolkit/components/telemetry/pingsender/pingsender.cpp b/toolkit/components/telemetry/pingsender/pingsender.cpp index daa2d3d58e5a..01fdb63c298a 100644 --- a/toolkit/components/telemetry/pingsender/pingsender.cpp +++ b/toolkit/components/telemetry/pingsender/pingsender.cpp @@ -9,6 +9,8 @@ #include #include #include +#include + #include #include "pingsender.h" @@ -16,6 +18,7 @@ using std::ifstream; using std::ios; using std::string; +using std::vector; namespace PingSender { @@ -56,36 +59,6 @@ std::string GenerateDateHeader() { return string(buffer); } -/** - * Read the ping contents from the specified file - */ -static std::string ReadPing(const string& aPingPath) { - string ping; - ifstream file; - - file.open(aPingPath.c_str(), ios::in | ios::binary); - - if (!file.is_open()) { - PINGSENDER_LOG("ERROR: Could not open ping file\n"); - return ""; - } - - do { - char buff[4096]; - - file.read(buff, sizeof(buff)); - - if (file.bad()) { - PINGSENDER_LOG("ERROR: Could not read ping contents\n"); - return ""; - } - - ping.append(buff, file.gcount()); - } while (!file.eof()); - - return ping; -} - std::string GzipCompress(const std::string& rawData) { z_stream deflater = {}; @@ -153,33 +126,25 @@ std::string GzipCompress(const std::string& rawData) { return gzipData; } -} // namespace PingSender +class Ping { + public: + Ping(const string& aUrl, const string& aPath) : mUrl(aUrl), mPath(aPath) {} + bool Send() const; + bool Delete() const; -using namespace PingSender; + private: + string Read() const; -int main(int argc, char* argv[]) { - string url; - string pingPath; + const string mUrl; + const string mPath; +}; - if (argc == 3) { - url = argv[1]; - pingPath = argv[2]; - } else { - PINGSENDER_LOG( - "Usage: pingsender URL PATH\n" - "Send the payload stored in PATH to the specified URL using " - "an HTTP POST message\n" - "then delete the file after a successful send.\n"); - return EXIT_FAILURE; - } - - ChangeCurrentWorkingDirectory(pingPath); - - string ping(ReadPing(pingPath)); +bool Ping::Send() const { + string ping(Read()); if (ping.empty()) { PINGSENDER_LOG("ERROR: Ping payload is empty\n"); - return EXIT_FAILURE; + return false; } // Compress the ping using gzip. @@ -190,17 +155,79 @@ int main(int argc, char* argv[]) { // it compressed. if (gzipPing.empty()) { PINGSENDER_LOG("ERROR: Ping compression failed\n"); + return false; + } + + if (!Post(mUrl, gzipPing)) { + return false; + } + + return true; +} + +bool Ping::Delete() const { + return !mPath.empty() && !std::remove(mPath.c_str()); +} + +string Ping::Read() const { + string ping; + ifstream file; + + file.open(mPath.c_str(), ios::in | ios::binary); + + if (!file.is_open()) { + PINGSENDER_LOG("ERROR: Could not open ping file\n"); + return ""; + } + + do { + char buff[4096]; + + file.read(buff, sizeof(buff)); + + if (file.bad()) { + PINGSENDER_LOG("ERROR: Could not read ping contents\n"); + return ""; + } + + ping.append(buff, file.gcount()); + } while (!file.eof()); + + return ping; +} + +} // namespace PingSender + +using namespace PingSender; + +int main(int argc, char* argv[]) { + vector pings; + + if ((argc >= 3) && ((argc - 1) % 2 == 0)) { + for (int i = 1; i < argc; i += 2) { + Ping ping(argv[i], argv[i + 1]); + pings.push_back(ping); + } + } else { + PINGSENDER_LOG( + "Usage: pingsender URL1 PATH1 URL2 PATH2 ...\n" + "Send the payloads stored in PATH to the specified URL using an " + "HTTP POST\nmessage for each payload then delete the file after a " + "successful send.\n"); return EXIT_FAILURE; } - if (!Post(url, gzipPing)) { - return EXIT_FAILURE; - } + ChangeCurrentWorkingDirectory(argv[2]); - // If the ping was successfully sent, delete the file. - if (!pingPath.empty() && std::remove(pingPath.c_str())) { - // We failed to remove the pending ping file. - return EXIT_FAILURE; + for (const auto& ping : pings) { + if (!ping.Send()) { + return EXIT_FAILURE; + } + + if (!ping.Delete()) { + PINGSENDER_LOG("ERROR: Could not delete the ping file\n"); + return EXIT_FAILURE; + } } return EXIT_SUCCESS; diff --git a/toolkit/components/telemetry/tests/unit/test_PingSender.js b/toolkit/components/telemetry/tests/unit/test_PingSender.js index 0c62e020ff71..8cfba282742a 100644 --- a/toolkit/components/telemetry/tests/unit/test_PingSender.js +++ b/toolkit/components/telemetry/tests/unit/test_PingSender.js @@ -16,6 +16,38 @@ ChromeUtils.import("resource://gre/modules/TelemetryStorage.jsm", this); ChromeUtils.import("resource://gre/modules/TelemetryUtils.jsm", this); ChromeUtils.import("resource://gre/modules/Timer.jsm", this); +function generateTestPingData() { + return { + type: "test-pingsender-type", + id: TelemetryUtils.generateUUID(), + creationDate: new Date().toISOString(), + version: 4, + payload: { + dummy: "stuff", + }, + }; +} + +function testSendingPings(pingPaths) { + const url = "http://localhost:" + PingServer.port + "/submit/telemetry/"; + const pings = pingPaths.map(path => { + return { + url, + path, + }; + }); + TelemetrySend.testRunPingSender(pings, (_, topic, __) => { + switch (topic) { + case "process-finished": // finished indicates an exit code of 0 + Assert.ok(true, "Pingsender should be able to post to localhost"); + break; + case "process-failed": // failed indicates an exit code != 0 + Assert.ok(false, "Pingsender should be able to post to localhost"); + break; + } + }); +} + /** * Wait for a ping file to be deleted from the pending pings directory. */ @@ -49,15 +81,7 @@ add_task(async function setup() { add_task(async function test_pingSender() { // Generate a new ping and save it among the pending pings. - const data = { - type: "test-pingsender-type", - id: TelemetryUtils.generateUUID(), - creationDate: new Date(1485810000).toISOString(), - version: 4, - payload: { - dummy: "stuff", - }, - }; + const data = generateTestPingData(); await TelemetryStorage.savePing(data, true); // Get the local path of the saved ping. @@ -83,8 +107,8 @@ add_task(async function test_pingSender() { // Try to send the ping twice using the pingsender (we expect 404 both times). const errorUrl = "http://localhost:" + failingServer.identity.primaryPort + "/lookup_fail"; - TelemetrySend.testRunPingSender(errorUrl, pingPath); - TelemetrySend.testRunPingSender(errorUrl, pingPath); + TelemetrySend.testRunPingSender([{ url: errorUrl, path: pingPath }]); + TelemetrySend.testRunPingSender([{ url: errorUrl, path: pingPath }]); // Wait until we hit the 404 server twice. After that, make sure that the ping // still exists locally. @@ -95,25 +119,7 @@ add_task(async function test_pingSender() { ); // Try to send it using the pingsender. - const url = "http://localhost:" + PingServer.port + "/submit/telemetry/"; - TelemetrySend.testRunPingSender(url, pingPath, (_, topic, __) => { - switch (topic) { - case "process-finished": // finished indicates an exit code of 0 - Assert.equal( - true, - true, - "Pingsender should be able to post to localhost" - ); - break; - case "process-failed": // failed indicates an exit code != 0 - Assert.equal( - true, - false, - "Pingsender should be able to post to localhost" - ); - break; - } - }); + testSendingPings([pingPath]); let req = await PingServer.promiseNextRequest(); let ping = decodeRequestPayload(req); @@ -160,8 +166,7 @@ add_task(async function test_pingSender() { ]; for (let indx in bannedUris) { TelemetrySend.testRunPingSender( - bannedUris[indx], - pingPath, + [{ url: bannedUris[indx], path: pingPath }], (_, topic, __) => { switch (topic) { case "process-finished": // finished indicates an exit code of 0 @@ -190,6 +195,35 @@ add_task(async function test_pingSender() { await new Promise(r => failingServer.stop(r)); }); +add_task(async function test_pingSender_multiple_pings() { + // Generate two new pings and save them among the pending pings. + const data = [generateTestPingData(), generateTestPingData()]; + + for (const d of data) { + await TelemetryStorage.savePing(d, true); + } + + // Get the local path of the saved pings. + const pingPaths = data.map(d => + OS.Path.join(TelemetryStorage.pingDirectoryPath, d.id) + ); + + // Try to send them using the pingsender. + testSendingPings(pingPaths); + + // Check the pings + for (const d of data) { + let req = await PingServer.promiseNextRequest(); + let ping = decodeRequestPayload(req); + Assert.equal(ping.id, d.id, "Should have received the correct ping id."); + } + + // Check that the PingSender removed the pending pings. + for (const d of data) { + await waitForPingDeletion(d.id); + } +}); + add_task(async function cleanup() { await PingServer.stop(); });