Merge pull request #630 from brizental/1693487-event-extras

Bug 1693487 - Accept booleans and numbers as event extras

Author: Beatriz Rizental

CHANGELOG.md

@@ -6,6 +6,7 @@
 * [#580](https://github.com/mozilla/glean.js/pull/580): Limit size of pings database to 250 pings or 10MB.
 * [#580](https://github.com/mozilla/glean.js/pull/580): BUGFIX: Pending pings at startup up are uploaded from oldest to newest.
 * [#607](https://github.com/mozilla/glean.js/pull/607): Record an error when incoherent timestamps are calculated for events after a restart.
+* [#630](https://github.com/mozilla/glean.js/pull/630): Accept booleans and numbers as event extras.
 
 # v0.18.1 (2021-07-22)
 

glean/package-lock.json

@@ -19,7 +19,7 @@ const LOG_TAG = "CLI";
 const VIRTUAL_ENVIRONMENT_DIR = ".venv";
 
 // The version of glean_parser to install from PyPI.
-const GLEAN_PARSER_VERSION = "3.7.0";
+const GLEAN_PARSER_VERSION = "3.8.0";
 
 // This script runs a given Python module as a "main" module, like
 // `python -m module`. However, it first checks that the installed

glean/src/cli.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) ?? [];
@@ -273,7 +278,7 @@ class EventsDatabase {
    * 1. Sorts event by execution counter and timestamp;
    * 2. Applies offset to events timestamps;
    * 3. Removes the first event if it is a `glean.restarted` event;
-   * 4. Removes reserved extra keys.
+   * 4. Removes reserved extra keys and stringifies all extras values.
    *
    * @param pingName The name of the ping for which the payload is being prepared.
    * @param pingData An unsorted list of events.
@@ -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.
@@ -364,7 +368,7 @@ class EventsDatabase {
       );
     }
 
-    return sortedEvents.map((e) => RecordedEvent.toJSONObject(e.withoutReservedExtras()));
+    return sortedEvents.map((e) => RecordedEvent.toJSONObject(e.payload()));
   }
 
   /**

glean/src/core/metrics/events_database/index.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 {
@@ -47,13 +48,27 @@ export class RecordedEvent {
     );
   }
 
+  private static withTransformedExtras(
+    e: RecordedEvent,
+    transformFn: (extras: ExtraMap) => ExtraMap
+  ): RecordedEvent {
+    const extras = e.extra || {};
+    const transformedExtras = transformFn(extras);
+    return new RecordedEvent(
+      e.category,
+      e.name,
+      e.timestamp,
+      (transformedExtras && Object.keys(transformedExtras).length > 0) ? transformedExtras : undefined
+    );
+  }
+
   /**
    * Add another extra key to a RecordedEvent object.
    *
    * @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 = {};
     }
@@ -68,19 +83,38 @@ export class RecordedEvent {
    * @returns A new RecordedEvent object.
    */
   withoutReservedExtras(): RecordedEvent {
-    const extras = this.extra || {};
-    const filteredExtras = Object.keys(extras)
-      .filter(key => !GLEAN_RESERVED_EXTRA_KEYS.includes(key))
-      .reduce((obj: ExtraMap, key) => {
-        obj[key] = extras[key];
-        return obj;
-      }, {});
+    return RecordedEvent.withTransformedExtras(
+      this,
+      (extras: ExtraMap): ExtraMap => {
+        return Object.keys(extras)
+          .filter(key => !GLEAN_RESERVED_EXTRA_KEYS.includes(key))
+          .reduce((obj: ExtraMap, key) => {
+            obj[key] = extras[key];
+            return obj;
+          }, {});
+      }
+    );
+  }
 
-    return new RecordedEvent(
-      this.category,
-      this.name,
-      this.timestamp,
-      (filteredExtras && Object.keys(filteredExtras).length > 0) ? filteredExtras : undefined
+  /**
+   * Generate a new RecordedEvent object,
+   * in the format expected on ping payloads.
+   *
+   * Strips reserved extra keys
+   * and stringifies all event extras.
+   *
+   * @returns A new RecordedEvent object.
+   */
+  payload(): RecordedEvent {
+    return RecordedEvent.withTransformedExtras(
+      this.withoutReservedExtras(),
+      (extras: ExtraMap): ExtraMap => {
+        return Object.keys(extras)
+          .reduce((extra: Record<string, string>, key) => {
+            extra[key] = extras[key].toString();
+            return extra;
+          }, {});
+      }
     );
   }
 }

glean/src/core/metrics/events_database/recorded_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/src/core/metrics/types/event.ts

@@ -139,9 +139,15 @@ for_testing:
     send_in_pings:
       - testing
     extra_keys:
-      sample:
-        description: |
-          Sample extra key.
+      sample_string:
+        description: Sample string extra key.
+        type: string
+      sample_boolean:
+        description: Sample boolean extra key.
+        type: boolean
+      sample_quantity:
+        description: Sample quantity extra key.
+        type: quantity
   quantity:
     type: quantity
     description: |

glean/tests/integration/schema/metrics.yaml

@@ -60,7 +60,11 @@ describe("schema", function() {
     metrics.boolean.set(false);
     metrics.counter.add(10);
     metrics.datetime.set();
-    metrics.event.record({ sample: "hey" });
+    metrics.event.record({
+      sample_string: "hey",
+      sample_boolean: false,
+      sample_quantity: 42,
+    });
     metrics.labeledBoolean["a_label"].set(true);
     metrics.labeledCounter["a_label"].add();
     metrics.labeledString["a_label"].set("ho");

glean/tests/integration/schema/schema.spec.ts

@@ -234,4 +234,50 @@ 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
+    });
+    // Record again with incomplete extras
+    event.record({
+      "string": "twoString",
+      "boolean": true,
+    });
+    // Record again without extras
+    event.record();
+
+    const snapshot = await event.testGetValue();
+    assert.ok(snapshot);
+    assert.strictEqual(snapshot.length, 3);
+
+    const extras1 = snapshot[0].extra;
+    assert.ok(extras1);
+    assert.strictEqual(extras1["string"], "aString");
+    assert.strictEqual(extras1["boolean"], false);
+    assert.strictEqual(extras1["number"], 42);
+
+    const extras2 = snapshot[1].extra;
+    assert.ok(extras2);
+    assert.strictEqual(extras2["string"], "twoString");
+    assert.strictEqual(extras2["boolean"], true);
+    assert.strictEqual(extras2["number"], undefined);
+
+    const extras3 = snapshot[2].extra;
+    assert.ok(!extras3);
+  });
 });