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

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 {Array}<Object> pings An array of objects holding url / path pairs
-   * @param {String} aPingFilePath The path to the file holding the ping
+   *        for each ping to be sent. The URL represent the telemetry server the
-   *        contents, if if sent successfully the pingsender will delete it.
+   *        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
+   *        (subject, topic, data) and a topic of "process-finished" or
-            "process-failed" after pingsender completion.
+   *        "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) {
+  testRunPingSender(pings, observer) {
-    return TelemetrySendImpl.runPingSender(url, pingPath, 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);
   },
 };

toolkit/components/telemetry/pingsender/pingsender.cpp

@@ -9,6 +9,8 @@
 #include <fstream>
 #include <iomanip>
 #include <string>
+#include <vector>
+
 #include <zlib.h>
 
 #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[]) {
+  const string mUrl;
-  string url;
+  const string mPath;
-  string pingPath;
+};
 
-  if (argc == 3) {
+bool Ping::Send() const {
-    url = argv[1];
+  string ping(Read());
-    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));
 
   if (ping.empty()) {
     PINGSENDER_LOG("ERROR: Ping payload is empty\n");
-    return EXIT_FAILURE;
+    return false;
   }
 
   // Compress the ping using gzip.
@@ -190,18 +155,80 @@ 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<Ping> 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<n> to the specified URL<n> using an "
+        "HTTP POST\nmessage for each payload then delete the file after a "
+        "successful send.\n");
     return EXIT_FAILURE;
   }
 
-  if (!Post(url, gzipPing)) {
+  ChangeCurrentWorkingDirectory(argv[2]);
+
+  for (const auto& ping : pings) {
+    if (!ping.Send()) {
       return EXIT_FAILURE;
     }
 
-  // If the ping was successfully sent, delete the file.
+    if (!ping.Delete()) {
-  if (!pingPath.empty() && std::remove(pingPath.c_str())) {
+      PINGSENDER_LOG("ERROR: Could not delete the ping file\n");
-    // We failed to remove the pending ping file.
       return EXIT_FAILURE;
     }
+  }
 
   return EXIT_SUCCESS;
 }

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 = {
+  const data = generateTestPingData();
-    type: "test-pingsender-type",
-    id: TelemetryUtils.generateUUID(),
-    creationDate: new Date(1485810000).toISOString(),
-    version: 4,
-    payload: {
-      dummy: "stuff",
-    },
-  };
   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([{ url: errorUrl, path: pingPath }]);
-  TelemetrySend.testRunPingSender(errorUrl, 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/";
+  testSendingPings([pingPath]);
-  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;
-    }
-  });
 
   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],
+      [{ url: bannedUris[indx], path: pingPath }],
-      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();
 });