Allow numbers and booleans values as event extras

Author: brizental

glean/src/core/metrics/events_database/index.ts

@@ -4,6 +4,7 @@
 
 import type Store from "../../storage/index.js";
 import type { JSONArray, JSONObject, JSONValue } from "../../utils.js";
+import { isString } from "../../utils.js";
 import type { StorageBuilder } from "../../../platform/index.js";
 import { isUndefined } from "../../utils.js";
 import EventMetricType from "../types/event.js";
@@ -12,6 +13,7 @@ import CounterMetricType from "../types/counter.js";
 import { Lifetime } from "../lifetime.js";
 import { Context } from "../../context.js";
 import { generateReservedMetricIdentifiers } from "../database.js";
+import type { ExtraValues} from "./recorded_event.js";
 import { RecordedEvent } from "./recorded_event.js";
 import {
   GLEAN_EXECUTION_COUNTER_EXTRA_KEY,
@@ -29,7 +31,11 @@ const LOG_TAG = "core.Metric.EventsDatabase";
  * @param str The string to generate a date from.
  * @returns The Date object created.
  */
-function createDateObject(str = ""): Date {
+function createDateObject(str?: ExtraValues): Date {
+  if (!isString(str)) {
+    str = "";
+  }
+
   const date = new Date(str);
   // Date object will not throw errors when we attempt to create an invalid date.
   if (isNaN(date.getTime())) {
@@ -158,7 +164,7 @@ class EventsDatabase {
     for (const ping of metric.sendInPings) {
       const executionCounter = getExecutionCounterMetric([ping]);
 
-      let currentExecutionCount = await Context.metricsDatabase.getMetric(ping, executionCounter);
+      let currentExecutionCount = await Context.metricsDatabase.getMetric<number>(ping, executionCounter);
       // There might not be an execution counter stored in case:
       //
       // 1. The ping was already sent during this session and the events storage was cleared;
@@ -171,8 +177,7 @@ class EventsDatabase {
         // this must **always** be the first event of any events list.
         await recordGleanRestartedEvent([ping], new Date());
       }
-      // TODO (bug 1693487): Remove the stringification once the new events API is implemented.
-      value.addExtra(GLEAN_EXECUTION_COUNTER_EXTRA_KEY, currentExecutionCount.toString());
+      value.addExtra(GLEAN_EXECUTION_COUNTER_EXTRA_KEY, currentExecutionCount);
 
       const transformFn = (v?: JSONValue): JSONArray => {
         const existing: JSONArray = (v as JSONArray) ?? [];
@@ -285,7 +290,6 @@ class EventsDatabase {
   ): Promise<JSONArray> {
     // Sort events by execution counter and by timestamp.
     const sortedEvents = pingData.sort((a, b) => {
-      // TODO (bug 1693487): Remove the number casting once the new events API is implemented.
       const executionCounterA = Number(a.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY]);
       const executionCounterB = Number(b.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY]);
       // Sort by execution counter, in case they are different.

glean/src/core/metrics/events_database/recorded_event.ts

@@ -5,8 +5,9 @@
 import { GLEAN_RESERVED_EXTRA_KEYS } from "../../constants.js";
 import type { JSONObject } from "../../utils.js";
 
-// An helper type for the 'extra' map.
-export type ExtraMap = Record<string, string>;
+export type ExtraValues = string | boolean | number;
+// A helper type for the 'extra' map.
+export type ExtraMap = Record<string, ExtraValues>;
 
 // Represents the recorded data for a single event.
 export class RecordedEvent {
@@ -53,7 +54,7 @@ export class RecordedEvent {
    * @param key The key to add.
    * @param value The value of the key.
    */
-  addExtra(key: string, value: string): void {
+  addExtra(key: string, value: ExtraValues): void {
     if (!this.extra) {
       this.extra = {};
     }

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

@@ -6,7 +6,7 @@ import type { CommonMetricData } from "../index.js";
 import type { ExtraMap } from "../events_database/recorded_event.js";
 import { MetricType } from "../index.js";
 import { RecordedEvent } from "../events_database/recorded_event.js";
-import { getMonotonicNow, truncateStringAtBoundaryWithError } from "../../utils.js";
+import { getMonotonicNow, isString, truncateStringAtBoundaryWithError } from "../../utils.js";
 import { Context } from "../../context.js";
 import { ErrorType } from "../../error/error_type.js";
 
@@ -15,7 +15,7 @@ const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
 /**
  * An event metric.
  */
-class EventMetricType extends MetricType {
+class EventMetricType<SpecificExtraMap extends ExtraMap = ExtraMap> extends MetricType {
   private allowedExtraKeys?: string[];
 
   constructor(meta: CommonMetricData, allowedExtraKeys?: string[]) {
@@ -33,10 +33,8 @@ class EventMetricType extends MetricType {
    * that the recording of the metric will happen in order with other Glean API calls.
    *
    * @param instance The metric instance to record to.
-   * @param extra optional. This is a map, both keys and values need to be
-   *        strings, keys are identifiers. This is used for events where
-   *        additional richer context is needed.
-   *        The maximum length for values is 100 bytes.
+   * @param extra optional. Used for events where additional richer context is needed.
+   *        The maximum length for string values is 100 bytes.
    * @param timestamp The event timestamp, defaults to now.
    * @returns A promise that resolves once the event is recorded.
    */
@@ -55,7 +53,11 @@ class EventMetricType extends MetricType {
       truncatedExtra = {};
       for (const [name, value] of Object.entries(extra)) {
         if (instance.allowedExtraKeys.includes(name)) {
-          truncatedExtra[name] = await truncateStringAtBoundaryWithError(instance, value, MAX_LENGTH_EXTRA_KEY_VALUE);
+          if (isString(value)) {
+            truncatedExtra[name] = await truncateStringAtBoundaryWithError(instance, value, MAX_LENGTH_EXTRA_KEY_VALUE);
+          } else {
+            truncatedExtra[name] = value;
+          }
         } else {
           await Context.errorManager.record(instance, ErrorType.InvalidValue, `Invalid key index: ${name}`);
           continue;
@@ -77,12 +79,10 @@ class EventMetricType extends MetricType {
   /**
    * Record an event.
    *
-   * @param extra optional. This is a map, both keys and values need to be
-   *        strings, keys are identifiers. This is used for events where
-   *        additional richer context is needed.
-   *        The maximum length for values is 100 bytes.
+   * @param extra optional. Used for events where additional richer context is needed.
+   *        The maximum length for string values is 100 bytes.
    */
-  record(extra?: ExtraMap): void {
+  record(extra?: SpecificExtraMap): void {
     const timestamp = getMonotonicNow();
 
     Context.dispatcher.launch(async () => {

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

@@ -234,4 +234,35 @@ describe("EventMetric", function() {
       event.extra["truncatedExtra"]
     );
   });
+
+  it("all types of event keys are recorded correctly", async function () {
+    const event = new EventMetricType<{
+      "string": string,
+      "boolean": boolean,
+      "number": number
+    }>({
+      category: "test",
+      name: "event",
+      sendInPings: ["store1"],
+      lifetime: Lifetime.Ping,
+      disabled: false
+    }, ["string", "boolean", "number"]);
+
+    event.record({
+      "string": "aString",
+      "boolean": false,
+      "number": 42
+    });
+
+    const snapshot = await event.testGetValue();
+    assert.ok(snapshot);
+    assert.strictEqual(snapshot.length, 1);
+
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    assert.strictEqual(snapshot[0].extra!["string"], "aString");
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    assert.strictEqual(snapshot[0].extra!["boolean"], false);
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    assert.strictEqual(snapshot[0].extra!["number"], 42);
+  });
 });

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

@@ -199,7 +199,7 @@ describe("EventsDatabase", function() {
       // We need to use `getAndValidatePingData` here,
       // because the public function will strip reserved extra keys.
       const rawRecordedEvent = (await db["getAndValidatePingData"](ping))[0];
-      assert.strictEqual(rawRecordedEvent.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
+      assert.strictEqual(rawRecordedEvent.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
     }
   });
 
@@ -271,8 +271,8 @@ describe("EventsDatabase", function() {
     // We expect only two events here, restarted and the above. Execution counter 1.
     const rawRecordedEvents1 = (await db["getAndValidatePingData"]("aPing"));
     assert.strictEqual(rawRecordedEvents1.length, 2);
-    assert.strictEqual(rawRecordedEvents1[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
-    assert.strictEqual(rawRecordedEvents1[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
+    assert.strictEqual(rawRecordedEvents1[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
+    assert.strictEqual(rawRecordedEvents1[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
 
     // Fake restart Glean and record a new event.
     const restartedDb = new EventsDatabase(Glean.platform.Storage);
@@ -288,15 +288,15 @@ describe("EventsDatabase", function() {
     // the next two events are this run's restart event + the event we just recorded and both are execution counter 2.
     const rawRecordedEvents2 = (await db["getAndValidatePingData"]("aPing"))
       .sort((a, b) => {
-        const executionCounterA = parseInt(a.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY] || "0");
-        const executionCounterB = parseInt(b.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY] || "0");
+        const executionCounterA = a.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY] as number;
+        const executionCounterB = b.extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY] as number;
         return executionCounterA - executionCounterB;
       });
     assert.strictEqual(rawRecordedEvents2.length, 4);
-    assert.strictEqual(rawRecordedEvents2[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
-    assert.strictEqual(rawRecordedEvents2[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
-    assert.strictEqual(rawRecordedEvents2[2].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "2");
-    assert.strictEqual(rawRecordedEvents2[3].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "2");
+    assert.strictEqual(rawRecordedEvents2[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
+    assert.strictEqual(rawRecordedEvents2[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
+    assert.strictEqual(rawRecordedEvents2[2].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 2);
+    assert.strictEqual(rawRecordedEvents2[3].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 2);
 
     ping.submit();
     // Sanity check that the execution counter was cleared.
@@ -311,8 +311,8 @@ describe("EventsDatabase", function() {
     // We expect only two events again, the other have been cleared, execution counter 1.
     const rawRecordedEvents3 = (await db["getAndValidatePingData"]("aPing"));
     assert.strictEqual(rawRecordedEvents3.length, 2);
-    assert.strictEqual(rawRecordedEvents3[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
-    assert.strictEqual(rawRecordedEvents3[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], "1");
+    assert.strictEqual(rawRecordedEvents3[0].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
+    assert.strictEqual(rawRecordedEvents3[1].extra?.[GLEAN_EXECUTION_COUNTER_EXTRA_KEY], 1);
   });
 
   it("reserved extra properties are removed from the recorded events", async function () {