Only delete actually invalid data when collecting pings

Previous behaviour was to delete the whole ping when that happened.

This is a nice change either way, we should always delete as little
data as possible. However this becomes even more necessary if we decide
to go ahead with the changes in the previous commit.

Author: brizental

glean/src/core/metrics/database.ts

@@ -42,38 +42,6 @@ export function generateReservedMetricIdentifiers(name: string): {
   };
 }
 
-/**
- * Verifies if a given value is a valid Metrics object.
- *
- * @param v The value to verify
- * @returns Whether or not `v` is a valid Metrics object.
- */
-export function isValidInternalMetricsRepresentation(v: unknown): v is Metrics {
-  if (isObject(v)) {
-    // The root keys should all be metric types.
-    for (const metricType in v) {
-      const metrics = v[metricType];
-      if (isObject(metrics)) {
-        for (const metricIdentifier in metrics) {
-          if (!validateMetricInternalRepresentation(metricType, metrics[metricIdentifier])) {
-            log(
-              LOG_TAG,
-              `Invalid metric representation found for metric "${metricIdentifier}"`,
-              LoggingLevel.Debug
-            );
-            return false;
-          }
-        }
-      } else {
-        return false;
-      }
-    }
-    return true;
-  } else {
-    return false;
-  }
-}
-
 /**
  * Creates the metrics payload from a metrics object with metrics in their internal representation.
  *
@@ -260,24 +228,60 @@ class MetricsDatabase {
    * @returns The ping payload found for the given parameters or an empty object
    *          in case no data was found or the data that was found, was invalid.
    */
-  private async getAndValidatePingData(ping: string, lifetime: Lifetime): Promise<Metrics> {
+  private async getCorrectedPingData(ping: string, lifetime: Lifetime): Promise<Metrics> {
     const store = this._chooseStore(lifetime);
     const data = await store.get([ping]);
     if (isUndefined(data)) {
       return {};
     }
 
-    if (!isValidInternalMetricsRepresentation(data)) {
+    if (!isObject(data)) {
       log(
         LOG_TAG,
-        `Unexpected value found for ping "${ping}" in "${lifetime}" store: ${JSON.stringify(data)}. Clearing.`,
+        `Invalid value found in storage for ping "${ping}". Deleting.`,
         LoggingLevel.Debug
       );
       await store.delete([ping]);
       return {};
     }
 
-    return data;
+    const correctedData: Metrics = {};
+    // All top keys should be metric types.
+    for (const metricType in data) {
+      const metrics = data[metricType];
+      if (!isObject(metrics)) {
+        log(
+          LOG_TAG,
+          `Unexpected data found in storage for metrics of type "${metricType}" in ping "${ping}". Deleting.`,
+          LoggingLevel.Debug
+        );
+        await store.delete([ping, metricType]);
+        continue;
+      }
+
+      for (const metricIdentifier in metrics) {
+        if (!validateMetricInternalRepresentation(metricType, metrics[metricIdentifier])) {
+          log(
+            LOG_TAG,
+            `Invalid value found in storage for metric "${metricIdentifier}". Deleting.`,
+            LoggingLevel.Debug
+          );
+
+          await store.delete([ping, metricType, metricIdentifier]);
+          continue;
+        }
+
+        if (!correctedData[metricType]) {
+          correctedData[metricType] = {};
+        }
+
+        // Coersion is fine here, `validateMetricInternalRepresentation`
+        // validated that this is of the correct type.
+        correctedData[metricType][metricIdentifier] = metrics[metricIdentifier] as JSONValue;
+      }
+    }
+
+    return correctedData;
   }
 
   private processLabeledMetric(snapshot: Metrics, metricType: string, metricId: string, metricData: JSONValue) {
@@ -313,9 +317,9 @@ class MetricsDatabase {
    *          `undefined` in case the ping doesn't contain any recorded metrics.
    */
   async getPingMetrics(ping: string, clearPingLifetimeData: boolean): Promise<Metrics | undefined> {
-    const userData = await this.getAndValidatePingData(ping, Lifetime.User);
-    const pingData = await this.getAndValidatePingData(ping, Lifetime.Ping);
-    const appData = await this.getAndValidatePingData(ping, Lifetime.Application);
+    const userData = await this.getCorrectedPingData(ping, Lifetime.User);
+    const pingData = await this.getCorrectedPingData(ping, Lifetime.Ping);
+    const appData = await this.getCorrectedPingData(ping, Lifetime.Application);
 
     if (clearPingLifetimeData && Object.keys(pingData).length > 0) {
       await this.clear(Lifetime.Ping, ping);

glean/tests/unit/core/metrics/database.spec.ts

@@ -4,7 +4,7 @@
 
 import assert from "assert";
 
-import Database, { generateReservedMetricIdentifiers, isValidInternalMetricsRepresentation } from "../../../../src/core/metrics/database";
+import Database, { generateReservedMetricIdentifiers } from "../../../../src/core/metrics/database";
 import StringMetricType, { StringMetric } from "../../../../src/core/metrics/types/string";
 
 import type { JSONObject, JSONValue } from "../../../../src/core/utils";
@@ -259,48 +259,43 @@ describe("MetricsDatabase", function() {
   });
 
   describe("getPingMetrics", function() {
-    it("isValidInternalMetricsRepresentation validates correctly", function() {
-      // Invalids
-      assert.strictEqual(false, isValidInternalMetricsRepresentation("not even an object"));
-      assert.strictEqual(false, isValidInternalMetricsRepresentation({ 1: "an array-like object in not a ping!" }));
-      assert.strictEqual(false, isValidInternalMetricsRepresentation({
-        "aPing": {
-          "string": {
-            "too.nested": "not quite"
-          }
-        }
+    it("when incorrect data is found on the storage, it is deleted", async function() {
+      const db = new Database();
+      await db["appStore"].update(["aPing"], () => "not even an object");
+      assert.strictEqual(await db.getPingMetrics("aPing", false), undefined);
+      assert.strictEqual(await db["appStore"].get(["aPing"]), undefined);
+
+      await db["appStore"].update(["aPing"], () => ({
+        "string": "this should be an object"
       }));
-      assert.strictEqual(false, isValidInternalMetricsRepresentation({ "string": "almost!" }));
-      // Valids
-      assert.strictEqual(true, isValidInternalMetricsRepresentation({ "string": {} }));
-      assert.strictEqual(true, isValidInternalMetricsRepresentation({ "string": { "there.we": "go" } }));
-      assert.strictEqual(true, isValidInternalMetricsRepresentation({
+      assert.strictEqual(await db.getPingMetrics("aPing", false), undefined);
+      assert.deepStrictEqual(await db["appStore"].get(["aPing"]), {});
+
+      await db["appStore"].update(["aPing"], () => ({
         "string": {
-          "string.one": "foo",
-          "string.two": "bar",
-          "string.three": "baz",
+          "my.string": 1
         }
       }));
-      assert.strictEqual(true, isValidInternalMetricsRepresentation({
+      assert.strictEqual(await db.getPingMetrics("aPing", false), undefined);
+      assert.deepStrictEqual(await db["appStore"].get(["aPing"]), { "string": {} });
+
+      await db["appStore"].update(["aPing"], () => ({
         "string": {
-          "string.one": "foo",
-          "string.two": "bar",
-          "string.three": "baz",
+          "my.string": "a string"
         },
-        "boolean": {
-          "this.is": true,
-          "ping": false,
-          "looks": true
-        }
+        "wrong": "very wrong"
       }));
-      assert.strictEqual(true, isValidInternalMetricsRepresentation({}));
-    });
-
-    it("when incorrect data is found on the storage, it is deleted", async function() {
-      const db = new Database();
-      await db["appStore"].update(["aPing"], () => "not even a string");
-      assert.strictEqual(await db.getPingMetrics("aPing", false), undefined);
-      assert.strictEqual(await db["appStore"].get(["aPing"]), undefined);
+      assert.deepStrictEqual(await db.getPingMetrics("aPing", false), {
+        "string": {
+          "my.string": "a string"
+        }
+      });
+      assert.deepStrictEqual(await db["appStore"].get(["aPing"]),
+        {
+          "string": {
+            "my.string": "a string"
+          }
+        });
     });
 
     it("getting a ping with metric from only one lifetime works correctly", async function() {