Bug 1682574 - Implement error recording (#313)

Author: Beatriz Rizental

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.12.0...main)
 
+* [#313](https://github.com/mozilla/glean.js/pull/313): Implement error recording mechanism and error checking testing API.
+
 # v0.12.0 (2021-05-11)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.11.0...v0.12.0)

glean/src/core/context.ts

@@ -7,6 +7,7 @@ import Dispatcher from "./dispatcher.js";
 import type MetricsDatabase from "./metrics/database";
 import type EventsDatabase from "./metrics/events_database";
 import type PingsDatabase from "./pings/database";
+import type ErrorManager from "./error";
 
 /**
  * This class holds all of the Glean singleton's state and internal dependencies.
@@ -29,6 +30,10 @@ export class Context {
   private _metricsDatabase!: MetricsDatabase;
   private _eventsDatabase!: EventsDatabase;
   private _pingsDatabase!: PingsDatabase;
+  // The reason this was added to the Context,
+  // is to avoid a circular dependency between
+  // the ErrorManager's module and the CounterMetricType module.
+  private _errorManager!: ErrorManager;
 
   private _applicationId!: string;
   private _initialized: boolean;
@@ -113,6 +118,14 @@ export class Context {
     Context.instance._pingsDatabase = db;
   }
 
+  static get errorManager(): ErrorManager {
+    return Context.instance._errorManager;
+  }
+
+  static set errorManager(db: ErrorManager) {
+    Context.instance._errorManager = db;
+  }
+
   static get applicationId(): string {
     return Context.instance._applicationId;
   }

glean/src/core/error/error_type.ts

@@ -0,0 +1,17 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+/**
+ * The possible error types for metric recording.
+ */
+export enum ErrorType {
+  // For when the value to be recorded does not match the metric-specific restrictions
+  InvalidValue = "invalid_value",
+  // For when the label of a labeled metric does not match the restrictions
+  InvalidLabel = "invalid_label",
+  // For when the metric caught an invalid state while recording
+  InvalidState = "invalid_state",
+  // For when the value to be recorded overflows the metric-specific upper range
+  InvalidOverflow = "invalid_overflow",
+}

glean/src/core/error/index.ts

@@ -0,0 +1,86 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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 type { MetricType } from "../metrics";
+import type { ErrorType } from "./error_type.js";
+import CounterMetricType from "../metrics/types/counter";
+import { combineIdentifierAndLabel, stripLabel } from "../metrics/types/labeled";
+
+/**
+ * For a given metric, get the metric in which to record errors.
+ *
+ * # Important
+ *
+ * Errors do not adhere to the usual "maximum label" restriction.
+ *
+ * @param metric The metric to record an error for.
+ * @param error The error type to record.
+ *
+ * @returns The metric to record to.
+ */
+function getErrorMetricForMetric(metric: MetricType, error: ErrorType): CounterMetricType {
+  const identifier = metric.baseIdentifier();
+  const name = stripLabel(identifier);
+
+  // We don't use the labeled metric type here,
+  // because we want to bypass the max number of allowed labels.
+  return new CounterMetricType({
+    name: combineIdentifierAndLabel(error, name),
+    category: "glean.error",
+    lifetime: "ping",
+    // TODO: Also add the metric ping to the list. Depends on Bug 1710838.
+    sendInPings: metric.sendInPings,
+    disabled: false,
+  });
+}
+
+export default class ErrorManager {
+  /**
+   * Records an error into Glean.
+   *
+   * Errors are recorded as labeled counters in the `glean.error` category.
+   *
+   * @param metric The metric to record an error for.
+   * @param error The error type to record.
+   * @param message The message to log. This message is not sent with the ping.
+   *        It does not need to include the metric id, as that is automatically
+   *        prepended to the message.
+   * @param numErrors The number of errors of the same type to report.
+   */
+  async record (
+    metric: MetricType,
+    error: ErrorType,
+    message: string,
+    numErrors = 1
+  ): Promise<void> {
+    const errorMetric = getErrorMetricForMetric(metric, error);
+    console.warn(`${metric.baseIdentifier()}: ${message}`);
+    if (numErrors > 0) {
+      await CounterMetricType._private_addUndispatched(errorMetric, numErrors);
+    } else {
+      // TODO: Throw error only when in test mode. Depends on Bug 1682771.
+    }
+  }
+
+  /**
+   * Gets the number of recorded errors for the given metric and error type.
+   *
+   * @param metric The metric to get the number of errors for.
+   * @param error The error type to get count of.
+   * @param ping The ping from which we want to retrieve the number of recorded errors.
+   *        Defaults to the first value in `sendInPings`.
+   *
+   * @returns The number of errors reported.
+   */
+  async testGetNumRecordedErrors (
+    metric: MetricType,
+    error: ErrorType,
+    ping?: string
+  ): Promise<number> {
+    const errorMetric = getErrorMetricForMetric(metric, error);
+    const numErrors = await errorMetric.testGetValue(ping);
+    return numErrors || 0;
+  }
+}
+

glean/src/core/glean.ts

@@ -15,7 +15,7 @@ import UUIDMetricType from "./metrics/types/uuid.js";
 import DatetimeMetricType, { DatetimeMetric } from "./metrics/types/datetime.js";
 import CorePings from "./internal_pings.js";
 import { registerPluginToEvent, testResetEvents } from "./events/utils.js";
-
+import ErrorManager from "./error/index.js";
 import type Platform from "../platform/index.js";
 import TestPlatform from "../platform/test/index.js";
 import { Lifetime } from "./metrics/lifetime.js";
@@ -197,6 +197,7 @@ class Glean {
     Context.metricsDatabase = new MetricsDatabase(Glean.platform.Storage);
     Context.eventsDatabase = new EventsDatabase(Glean.platform.Storage);
     Context.pingsDatabase = new PingsDatabase(Glean.platform.Storage);
+    Context.errorManager = new ErrorManager();
 
     Glean.instance._pingUploader = new PingUploader(correctConfig, Glean.platform, Context.pingsDatabase);
 

glean/src/core/metrics/index.ts

@@ -5,8 +5,10 @@
 import type { JSONValue } from "../utils.js";
 import type { Lifetime } from "./lifetime.js";
 import type MetricsDatabase from "./database.js";
+import type { ErrorType } from "../error/error_type.js";
 import { isUndefined } from "../utils.js";
 import { getValidDynamicLabel } from "./types/labeled.js";
+import { Context } from "../context.js";
 
 export interface Metrics {
   [aMetricType: string]: {
@@ -108,4 +110,17 @@ export abstract class MetricType implements CommonMetricData {
   shouldRecord(uploadEnabled: boolean): boolean {
     return (uploadEnabled && !this.disabled);
   }
+
+  /**
+   * Returns the number of errors recorded for the given metric.
+   *
+   * @param errorType The type of the error recorded.
+   * @param ping represents the name of the ping to retrieve the metric for.
+   *        Defaults to the first value in `sendInPings`.
+   *
+   * @returns the number of errors recorded for the metric.
+   */
+  async testGetNumRecordedErrors(errorType: string, ping: string = this.sendInPings[0]): Promise<number> {
+    return Context.errorManager.testGetNumRecordedErrors(this, errorType as ErrorType, ping);
+  }
 }

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

@@ -8,6 +8,7 @@ import { MetricType } from "../index.js";
 import { isUndefined, isNumber } from "../../utils.js";
 import { Context } from "../../context.js";
 import { Metric } from "../metric.js";
+import { ErrorType } from "../../error/error_type.js";
 
 export class CounterMetric extends Metric<number, number> {
   constructor(v: unknown) {
@@ -64,8 +65,11 @@ class CounterMetricType extends MetricType {
     }
 
     if (amount <= 0) {
-      // TODO: record error once Bug 1682574 is resolved.
-      console.warn(`Attempted to add an invalid amount ${amount}. Ignoring.`);
+      await Context.errorManager.record(
+        instance,
+        ErrorType.InvalidValue,
+        `Added negative and zero value ${amount}`
+      );
       return;
     }
 

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

@@ -3,11 +3,12 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import type { CommonMetricData } from "../index.js";
+import type { ExtraMap } from "../events_database.js";
 import { MetricType } from "../index.js";
-import type { ExtraMap} from "../events_database.js";
 import { RecordedEvent } from "../events_database.js";
-import { getMonotonicNow } from "../../utils.js";
+import { getMonotonicNow, truncateStringAtBoundaryWithError } from "../../utils.js";
 import { Context } from "../../context.js";
+import { ErrorType } from "../../error/error_type.js";
 
 const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
 
@@ -45,10 +46,9 @@ class EventMetricType extends MetricType {
         truncatedExtra = {};
         for (const [name, value] of Object.entries(extra)) {
           if (this.allowedExtraKeys.includes(name)) {
-            truncatedExtra[name] = value.substr(0, MAX_LENGTH_EXTRA_KEY_VALUE);
+            truncatedExtra[name] = await truncateStringAtBoundaryWithError(this, value, MAX_LENGTH_EXTRA_KEY_VALUE);
           } else {
-            // TODO: bug 1682574 - record an error.
-            console.error(`Invalid key index ${name}`);
+            await Context.errorManager.record(this, ErrorType.InvalidValue, `Invalid key index: ${name}`);
             continue;
           }
         }

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

@@ -9,6 +9,8 @@ import type StringMetricType from "./string.js";
 import type MetricsDatabase from "../database";
 import type { JSONValue } from "../../utils.js";
 import { Metric } from "../metric.js";
+import { Context } from "../../context.js";
+import { ErrorType } from "../../error/error_type.js";
 
 /**
  * This is an internal metric representation for labeled metrics.
@@ -75,13 +77,26 @@ export function combineIdentifierAndLabel(
   return `${metricName}/${label}`;
 }
 
+/**
+ * Strips the label from a metric identifier.
+ *
+ * This is a no-op in case the identifier does not contain a label.
+ *
+ * @param identifier The identifier to strip a label from.
+ *
+ * @returns The identifier without the label.
+ */
+export function stripLabel(identifier: string): string {
+  return identifier.split("/")[0];
+}
+
 /**
  * Checks if the dynamic label stored in the metric data is
  * valid. If not, record an error and store data in the "__other__"
  * label.
  *
  * @param metricsDatabase the metrics database.
- * @param metric the metric metadata.
+ * @param metric the metric to record to.
  *
  * @returns a valid label that can be used to store data.
  */
@@ -114,13 +129,19 @@ export async function getValidDynamicLabel(metricsDatabase: MetricsDatabase, met
   if (numUsedKeys >= MAX_LABELS) {
     hitError = true;
   } else if (metric.dynamicLabel.length > MAX_LABEL_LENGTH) {
-    console.error(`label length ${metric.dynamicLabel.length} exceeds maximum of ${MAX_LABEL_LENGTH}`);
     hitError = true;
-    // TODO: record error in bug 1682574
+    await Context.errorManager.record(
+      metric,
+      ErrorType.InvalidLabel,
+      `Label length ${metric.dynamicLabel.length} exceeds maximum of ${MAX_LABEL_LENGTH}.`
+    );
   } else if (!LABEL_REGEX.test(metric.dynamicLabel)) {
-    console.error(`label must be snake_case, got '${metric.dynamicLabel}'`);
     hitError = true;
-    // TODO: record error in bug 1682574
+    await Context.errorManager.record(
+      metric,
+      ErrorType.InvalidLabel,
+      `Label must be snake_case, got '${metric.dynamicLabel}'.`
+    );
   }
 
   return (hitError)

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

@@ -7,6 +7,7 @@ import { MetricType } from "../index.js";
 import { isNumber } from "../../utils.js";
 import { Context } from "../../context.js";
 import { Metric } from "../metric.js";
+import { ErrorType } from "../../error/error_type.js";
 
 export class QuantityMetric extends Metric<number, number> {
   constructor(v: unknown) {
@@ -59,12 +60,15 @@ class QuantityMetricType extends MetricType {
     }
 
     if (value < 0) {
-      console.warn(`Attempted to set an invalid value ${value}. Ignoring.`);
+      await Context.errorManager.record(
+        instance,
+        ErrorType.InvalidValue,
+        `Set negative value ${value}`
+      );
       return;
     }
 
     if (value > Number.MAX_SAFE_INTEGER) {
-      console.warn(`Attempted to set a big value ${value}. Capped at ${Number.MAX_SAFE_INTEGER}.`);
       value = Number.MAX_SAFE_INTEGER;
     }