Bug 1710828 - Record error in case value of incorrect type is given to metric recording function

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 * [#1233](https://github.com/mozilla/glean.js/pull/1233): Add optional `buildDate` argument to `initialize` configuration. The build date can be generated by glean_parser.
 * [#1233](https://github.com/mozilla/glean.js/pull/1233): Update glean_parser to version 5.1.0.
+* [#1217](https://github.com/mozilla/glean.js/pull/1217): Record `InvalidType` error when incorrectly type values are passed to metric recording functions.
 
 # v0.32.0 (2022-03-01)
 

glean/src/core/error/error_type.ts

@@ -14,4 +14,6 @@ export enum ErrorType {
   InvalidState = "invalid_state",
   // For when the value to be recorded overflows the metric-specific upper range
   InvalidOverflow = "invalid_overflow",
+  // For when the value passed to a recording function is not of the correct type.
+  InvalidType = "invalid_type",
 }

glean/src/core/error/index.ts

@@ -16,7 +16,7 @@ import log from "../log.js";
  */
 function createLogTag(metric: MetricType): string {
   const capitalizedType = metric.type.charAt(0).toUpperCase() + metric.type.slice(1);
-  return `core.Metrics.${capitalizedType}`;
+  return `core.metrics.${capitalizedType}`;
 }
 
 /**

glean/src/core/metrics/metric.ts

@@ -2,10 +2,35 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-import log, { LoggingLevel } from "../log.js";
 import type { JSONValue } from "../utils.js";
+import type { MetricType } from "./index.js";
+import { Context } from "../context.js";
+import { ErrorType } from "../error/error_type.js";
 
-const LOG_TAG = "core.Metrics.Metric";
+export enum MetricValidation {
+  Success,
+  Error
+}
+
+export type MetricValidationResult =
+  { type: MetricValidation.Success } |
+  { type: MetricValidation.Error, errorMessage: string, errorType?: ErrorType };
+
+export class MetricValidationError extends Error {
+  constructor(message?: string, readonly type = ErrorType.InvalidType) {
+    super(message);
+    try {
+      this.name = "MetricValidationError";
+    } catch {
+      // This fails in Qt.
+      // See https://bugreports.qt.io/browse/QTBUG-101298
+    }
+  }
+
+  async recordError(metric: MetricType) {
+    await Context.errorManager.record(metric, this.type, this.message);
+  }
+}
 
 /**
  * The Metric class describes the shared behaviour amongst concrete metrics.
@@ -19,19 +44,14 @@ const LOG_TAG = "core.Metrics.Metric";
  *    - Is the format in which this metric will be represented in the ping payload.
  *    - This format must be the exact same as described in [the Glean schema](https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/master/schemas/glean/glean/glean.1.schema.json).
  */
-
 export abstract class Metric<
   InternalRepresentation extends JSONValue,
   PayloadRepresentation extends JSONValue
-  > {
+> {
   protected _inner: InternalRepresentation;
 
   constructor(v: unknown) {
-    if (!this.validate(v)) {
-      throw new Error("Unable to create new Metric instance, value is in unexpected format.");
-    }
-
-    this._inner = v;
+    this._inner = this.validateOrThrow(v);
   }
 
   /**
@@ -46,30 +66,40 @@ export abstract class Metric<
   /**
    * Sets this metrics value.
    *
-   * @param v The value to set, must be in the exact internal representation of this metric.
-   * @throws In case the metric is not in the expected format.
+   * @param v The value to set.
    */
-  set(v: unknown): void {
-    if (!this.validate(v)) {
-      log(
-        LOG_TAG,
-        `Unable to set metric to ${JSON.stringify(v)}. Value is in unexpected format. Ignoring.`,
-        LoggingLevel.Error
-      );
-      return;
+  set(v: InternalRepresentation): void {
+    this._inner = v;
+  }
+
+  /**
+   * Validates a given value using the validation function and throws in case it is not valid.
+   *
+   * @param v The value to verify.
+   * @returns `v` if it is valid.
+   */
+  validateOrThrow(v: unknown): InternalRepresentation {
+    const validation = this.validate(v);
+    if (validation.type === MetricValidation.Error) {
+      throw new MetricValidationError(validation.errorMessage, validation.errorType);
     }
 
-    this._inner = v;
+    // This is safe, we have just validated the type above.
+    return v as InternalRepresentation;
   }
 
   /**
    * Validates that a given value is in the correct format for this metrics internal representation.
    *
+   * # Note
+   *
+   * This function should only check for validations
+   * that would prevent a metric from being recorded.
+   *
    * @param v The value to verify.
-   * @returns A special Typescript value (which compiles down to a boolean)
-   *          stating whether `v` is of the correct type.
+   * @returns Whether or not validation was successfull.
    */
-  abstract validate(v: unknown): v is InternalRepresentation;
+  abstract validate(v: unknown): MetricValidationResult;
 
   /**
    * Gets this metrics value in its payload representation.

glean/src/core/metrics/types/boolean.ts

@@ -5,7 +5,9 @@
 import type { CommonMetricData } from "../index.js";
 import { MetricType } from "../index.js";
 import { Context } from "../../context.js";
-import { Metric } from "../metric.js";
+import type { MetricValidationResult } from "../metric.js";
+import { MetricValidationError } from "../metric.js";
+import { Metric, MetricValidation } from "../metric.js";
 import { isBoolean, testOnlyCheck } from "../../utils.js";
 
 const LOG_TAG = "core.metrics.BooleanMetricType";
@@ -15,9 +17,17 @@ export class BooleanMetric extends Metric<boolean, boolean> {
     super(v);
   }
 
-  validate(v: unknown): v is boolean {
-    return isBoolean(v);
+  validate(v: unknown): MetricValidationResult {
+    if (!isBoolean(v)) {
+      return {
+        type: MetricValidation.Error,
+        errorMessage: `Expected boolean value, got ${JSON.stringify(v)}`
+      };
+    } else {
+      return { type: MetricValidation.Success };
+    }
   }
+
   payload(): boolean {
     return this._inner;
   }
@@ -41,8 +51,14 @@ class InternalBooleanMetricType extends MetricType {
         return;
       }
 
-      const metric = new BooleanMetric(value);
-      await Context.metricsDatabase.record(this, metric);
+      try {
+        const metric = new BooleanMetric(value);
+        await Context.metricsDatabase.record(this, metric);
+      } catch(e) {
+        if (e instanceof MetricValidationError) {
+          await e.recordError(this);
+        }
+      }
     });
   }
 

glean/src/core/metrics/types/counter.ts

@@ -4,11 +4,13 @@
 
 import type { CommonMetricData } from "../index.js";
 import type { JSONValue } from "../../utils.js";
+import type { MetricValidationResult } from "../metric.js";
+import { saturatingAdd, isUndefined, testOnlyCheck } from "../../utils.js";
 import { MetricType } from "../index.js";
-import { isUndefined, isInteger, testOnlyCheck } from "../../utils.js";
 import { Context } from "../../context.js";
-import { Metric } from "../metric.js";
-import { ErrorType } from "../../error/error_type.js";
+import { Metric, MetricValidationError } from "../metric.js";
+import log from "../../log.js";
+import { validatePositiveInteger } from "../utils.js";
 
 const LOG_TAG = "core.metrics.CounterMetricType";
 
@@ -17,21 +19,18 @@ export class CounterMetric extends Metric<number, number> {
     super(v);
   }
 
-  validate(v: unknown): v is number {
-    if (!isInteger(v)) {
-      return false;
-    }
-
-    if (v <= 0) {
-      return false;
-    }
-
-    return true;
+  validate(v: unknown): MetricValidationResult {
+    return validatePositiveInteger(v, false);
   }
 
   payload(): number {
     return this._inner;
   }
+
+  saturatingAdd(amount: unknown): void {
+    const correctAmount = this.validateOrThrow(amount);
+    this._inner = saturatingAdd(this._inner, correctAmount);
+  }
 }
 
 /**
@@ -64,37 +63,31 @@ export class InternalCounterMetricType extends MetricType {
       amount = 1;
     }
 
-    if (amount <= 0) {
-      await Context.errorManager.record(
-        this,
-        ErrorType.InvalidValue,
-        `Added negative and zero value ${amount}`
-      );
-      return;
+    try {
+      const transformFn = ((amount) => {
+        return (v?: JSONValue): CounterMetric => {
+          const metric = new CounterMetric(amount);
+          if (v) {
+            try {
+              // Throws an error if v in not valid input.
+              metric.saturatingAdd(v);
+            } catch {
+              log(
+                LOG_TAG,
+                `Unexpected value found in storage for metric ${this.name}: ${JSON.stringify(v)}. Overwriting.`
+              );
+            }
+          }
+          return metric;
+        };
+      })(amount);
+
+      await Context.metricsDatabase.transform(this, transformFn);
+    } catch(e) {
+      if (e instanceof MetricValidationError) {
+        await e.recordError(this);
+      }
     }
-
-    const transformFn = ((amount) => {
-      return (v?: JSONValue): CounterMetric => {
-        let metric: CounterMetric;
-        let result: number;
-        try {
-          metric = new CounterMetric(v);
-          result = metric.get() + amount;
-        } catch {
-          metric = new CounterMetric(amount);
-          result = amount;
-        }
-
-        if (result > Number.MAX_SAFE_INTEGER) {
-          result = Number.MAX_SAFE_INTEGER;
-        }
-
-        metric.set(result);
-        return metric;
-      };
-    })(amount);
-
-    await Context.metricsDatabase.transform(this, transformFn);
   }
 
   add(amount?: number): void {

glean/src/core/metrics/types/datetime.ts

@@ -6,7 +6,9 @@ import type { CommonMetricData } from "../index.js";
 import { MetricType } from "../index.js";
 import TimeUnit from "../../metrics/time_unit.js";
 import { Context } from "../../context.js";
-import { Metric } from "../metric.js";
+import type { MetricValidationResult } from "../metric.js";
+import { MetricValidationError } from "../metric.js";
+import { Metric, MetricValidation } from "../metric.js";
 import { isNumber, isObject, isString, testOnlyCheck } from "../../utils.js";
 
 const LOG_TAG = "core.metrics.DatetimeMetricType";
@@ -43,7 +45,11 @@ export class DatetimeMetric extends Metric<DatetimeInternalRepresentation, strin
     super(v);
   }
 
-  static fromDate(v: Date, timeUnit: TimeUnit): DatetimeMetric {
+  static fromDate(v: unknown, timeUnit: TimeUnit): DatetimeMetric {
+    if(!(v instanceof Date)) {
+      throw new MetricValidationError(`Expected Date object, got ${JSON.stringify(v)}`);
+    }
+
     return new DatetimeMetric({
       timeUnit,
       timezone: v.getTimezoneOffset(),
@@ -78,19 +84,25 @@ export class DatetimeMetric extends Metric<DatetimeInternalRepresentation, strin
     return this._inner.date;
   }
 
-  validate(v: unknown): v is DatetimeInternalRepresentation {
+  validate(v: unknown): MetricValidationResult {
     if (!isObject(v) || Object.keys(v).length !== 3) {
-      return false;
+      return {
+        type: MetricValidation.Error,
+        errorMessage: `Expected Glean datetime metric object, got ${JSON.stringify(v)}`
+      };
     }
 
     const timeUnitVerification = "timeUnit" in v && isString(v.timeUnit) && Object.values(TimeUnit).includes(v.timeUnit as TimeUnit);
     const timezoneVerification = "timezone" in v && isNumber(v.timezone);
     const dateVerification = "date" in v && isString(v.date) && v.date.length === 24 && !isNaN(Date.parse(v.date));
     if (!timeUnitVerification || !timezoneVerification || !dateVerification) {
-      return false;
+      return {
+        type: MetricValidation.Error,
+        errorMessage: `Invalid property on datetime metric, got ${JSON.stringify(v)}`
+      };
     }
 
-    return true;
+    return { type: MetricValidation.Success };
   }
 
   /**
@@ -214,8 +226,15 @@ export class InternalDatetimeMetricType extends MetricType {
       break;
     }
 
-    const metric = DatetimeMetric.fromDate(value, this.timeUnit);
-    await Context.metricsDatabase.record(this, metric);
+    try {
+      const metric = DatetimeMetric.fromDate(value, this.timeUnit);
+      await Context.metricsDatabase.record(this, metric);
+    } catch(e) {
+      if (e instanceof MetricValidationError) {
+        await e.recordError(this);
+      }
+    }
+
   }
 
   set(value?: Date): void {

glean/src/core/metrics/types/labeled.ts

@@ -7,7 +7,8 @@ import type CounterMetricType from "./counter.js";
 import type BooleanMetricType from "./boolean.js";
 import type StringMetricType from "./string.js";
 import type { JSONValue } from "../../utils.js";
-import { Metric } from "../metric.js";
+import type { MetricValidationResult } from "../metric.js";
+import { Metric, MetricValidation } from "../metric.js";
 import { Context } from "../../context.js";
 import { ErrorType } from "../../error/error_type.js";
 
@@ -23,8 +24,8 @@ export class LabeledMetric extends Metric<JSONValue, JSONValue> {
     super(v);
   }
 
-  validate(v: unknown): v is JSONValue {
-    return true;
+  validate(_v: unknown): MetricValidationResult {
+    return { type: MetricValidation.Success };
   }
 
   payload(): JSONValue {