mozilla · brizental · Jan 11, 2022 · Jan 3, 2022 · Jan 3, 2022 · Jan 6, 2022
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.30.0...main)
 
+* [#1065](https://github.com/mozilla/glean.js/pull/1065): Delete minimal amount of data when invalid data is found while collecting ping.
+  * Previous behaviour was to delete the whole ping when invalid data was found on the database,
+  new behaviour only deletes the actually invalid data and leave the rest of the ping intact.
+* [#1065](https://github.com/mozilla/glean.js/pull/1065): Only import metric types into the library when they are used either by the user or Glean itself.
+  * Previously the code required to deserialize metric data from the database was always imported by the library even if the metric type was never used by the client. This effort will decrease the size of the Glean.js bundles that don't import all the metric types.
 # v0.30.0 (2022-01-10)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.29.0...v0.30.0)

diff --git a/glean/package-lock.json b/glean/package-lock.json
diff --git a/glean/src/core/context.ts b/glean/src/core/context.ts
@@ -7,6 +7,8 @@ import type EventsDatabase from "./metrics/events_database/index.js";
 import type PingsDatabase from "./pings/database.js";
 import type ErrorManager from "./error/index.js";
 import type Platform from "../platform/index.js";
+import type { Metric } from "./metrics/metric.js";
+import type { JSONValue } from "./utils.js";
 import Dispatcher from "./dispatcher.js";
 import log, { LoggingLevel } from "./log.js";
 import type { Configuration } from "./config.js";
@@ -45,6 +47,13 @@ export class Context {
   private _initialized = false;
   // Whether or not Glean is in testing mode.
   private _testing = false;
+  // A map of metric types and their constructors.
+  // This map is dynamically filled everytime a metric type is constructed.
+  //
+  // If a metric is not in this map it cannot be deserialized from the database.
+  private _supportedMetrics: {
+    [type: string]: new (v: unknown) => Metric<JSONValue, JSONValue>
+  } = {};
 
   // The moment the current Glean.js session started.
   private _startTime: Date;
@@ -242,4 +251,27 @@ export class Context {
   static isPlatformSet(): boolean {
     return !!Context.instance._platform;
   }
+
+  static getSupportedMetric(type: string): (new (v: unknown) => Metric<JSONValue, JSONValue>) | undefined {
+    return Context.instance._supportedMetrics[type];
+  }
+
+  /**
+   * Adds a new constructor to the supported metrics map.
+   *
+   * If the metric map already contains this constructor, this is a no-op.
+   *
+   * @param type A string identifying the given metric type.
+   * @param ctor The metric constructor.
+   */
+  static addSupportedMetric(
+    type: string,
+    ctor: new (v: unknown) => Metric<JSONValue, JSONValue>
+  ): void {
+    if (type in Context.instance._supportedMetrics) {
+      return;
+    }
+
+    Context.instance._supportedMetrics[type] = ctor;
+  }
 }
diff --git a/glean/src/core/glean.ts b/glean/src/core/glean.ts
@@ -29,7 +29,7 @@ namespace Glean {
   // The below properties are exported for testing purposes.
   //
   // Instances of Glean's core metrics.
-  export const coreMetrics = new CoreMetrics();
+  export let coreMetrics = new CoreMetrics();
   // Instances of Glean's core pings.
   export const corePings = new CorePings();
   // An instance of the ping uploader.
@@ -463,6 +463,10 @@ namespace Glean {
     uploadEnabled = true,
     config?: ConfigurationInterface
   ): Promise<void> {
+    // Core metrics need to be re-initialized so that
+    // the supportedMetrics map is re-created.
+    coreMetrics = new CoreMetrics();
+
     Context.testing = true;
 
     setPlatform(TestPlatform);

diff --git a/glean/src/core/metrics/database.ts b/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);

diff --git a/glean/src/core/metrics/index.ts b/glean/src/core/metrics/index.ts
@@ -3,9 +3,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import type { JSONValue} from "../utils.js";
-import { isUndefined, testOnlyCheck} from "../utils.js";
+import { isUndefined, testOnlyCheck } from "../utils.js";
 import type { Lifetime } from "./lifetime.js";
 import type { ErrorType } from "../error/error_type.js";
+import type { Metric } from "./metric.js";
 import { getValidDynamicLabel } from "./types/labeled.js";
 import { Context } from "../context.js";
 
@@ -53,7 +54,14 @@ export abstract class MetricType implements CommonMetricData {
   readonly disabled: boolean;
   dynamicLabel?: string;
 
-  constructor(type: string, meta: CommonMetricData) {
+  constructor(
+    type: string,
+    meta: CommonMetricData,
+    metricCtor?: new (v: unknown) => Metric<JSONValue, JSONValue>
+  ) {
+    if (metricCtor) {
+      Context.addSupportedMetric(type, metricCtor);
+    }
     this.type = type;
 
     this.name = meta.name;

diff --git a/glean/src/core/metrics/types/boolean.ts b/glean/src/core/metrics/types/boolean.ts
@@ -30,7 +30,7 @@ export class BooleanMetric extends Metric<boolean, boolean> {
  */
 class BooleanMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("boolean", meta);
+    super("boolean", meta, BooleanMetric);
   }
 
   /**

diff --git a/glean/src/core/metrics/types/counter.ts b/glean/src/core/metrics/types/counter.ts
@@ -42,7 +42,7 @@ export class CounterMetric extends Metric<number, number> {
  */
 class CounterMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("counter", meta);
+    super("counter", meta, CounterMetric);
   }
 
   /**

diff --git a/glean/src/core/metrics/types/datetime.ts b/glean/src/core/metrics/types/datetime.ts
@@ -168,7 +168,7 @@ class DatetimeMetricType extends MetricType {
   timeUnit: TimeUnit;
 
   constructor(meta: CommonMetricData, timeUnit: string) {
-    super("datetime", meta);
+    super("datetime", meta, DatetimeMetric);
     this.timeUnit = timeUnit as TimeUnit;
   }
 

diff --git a/glean/src/core/metrics/types/quantity.ts b/glean/src/core/metrics/types/quantity.ts
@@ -41,7 +41,7 @@ export class QuantityMetric extends Metric<number, number> {
  */
 class QuantityMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("quantity", meta);
+    super("quantity", meta, QuantityMetric);
   }
 
   /**

diff --git a/glean/src/core/metrics/types/rate.ts b/glean/src/core/metrics/types/rate.ts
@@ -60,7 +60,7 @@ export class RateMetric extends Metric<Rate, Rate> {
  */
 class RateMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("rate", meta);
+    super("rate", meta, RateMetric);
   }
 
   /**

diff --git a/glean/src/core/metrics/types/string.ts b/glean/src/core/metrics/types/string.ts
@@ -41,7 +41,7 @@ export class StringMetric extends Metric<string, string> {
  */
 class StringMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("string", meta);
+    super("string", meta, StringMetric);
   }
 
   /**

diff --git a/glean/src/core/metrics/types/string_list.ts b/glean/src/core/metrics/types/string_list.ts
@@ -51,7 +51,7 @@ export class StringListMetric extends Metric<string[], string[]> {
  */
 class StringListMetricType extends MetricType {
   constructor(meta: CommonMetricData) {
-    super("string_list", meta);
+    super("string_list", meta, StringListMetric);
   }
 
   /**