Address review feedback

Author: Dexterp37

src/metrics/events_database.ts

@@ -3,9 +3,9 @@
 //  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import { Store } from "storage";
-import WeakStore from "storage/weak";
-import { MetricType } from "metrics";
+import PersistentStore from "storage/persistent";
 import { isUndefined, JSONArray, JSONObject, JSONValue } from "utils";
+import EventMetricType from "./types/event";
 
 export interface Metrics {
   [aMetricType: string]: {
@@ -14,21 +14,28 @@ export interface Metrics {
 }
 
 // An helper type for the 'extra' map.
-export type ExtraMap = { [name: string]: string };
+export type ExtraMap = Record<string, string>;
 
 // Represents the recorded data for a single event.
 export class RecordedEvent {
   constructor(
-    category: string,
-    name: string,
-    timestamp: number,
-    extra?: ExtraMap,
-  ) {
-    this.category = category;
-    this.name = name;
-    this.timestamp = timestamp;
-    this.extra = extra;
-  }
+    // The event's category.
+    //
+    // This is defined by users in the metrics file.
+    readonly category: string,
+    // The event's name.
+    //
+    // This is defined by users in the metrics file.
+    readonly name: string,
+    // The timestamp of when the event was recorded.
+    //
+    // This allows to order events.
+    readonly timestamp: number,
+    // A map of all extra data values.
+    //
+    // The set of allowed extra keys is defined by users in the metrics file.
+    readonly extra?: ExtraMap,
+  ) {}
 
   static toJSONObject(e: RecordedEvent): JSONObject {
     return {
@@ -47,23 +54,6 @@ export class RecordedEvent {
       e["extra"] as ExtraMap | undefined
     );
   }
-
-  // The event's category.
-  //
-  // This is defined by users in the metrics file.
-  readonly category: string;
-  // The event's name.
-  //
-  // This is defined by users in the metrics file.
-  readonly name: string;
-  // The timestamp of when the event was recorded.
-  //
-  // This allows to order events.
-  readonly timestamp: number;
-  // A map of all extra data values.
-  //
-  // The set of allowed extra keys is defined by users in the metrics file.
-  readonly extra?: ExtraMap;
 }
 
 /**
@@ -91,7 +81,7 @@ class EventsDatabase {
   private eventsStore: Store;
 
   constructor() {
-    this.eventsStore = new WeakStore("unused");
+    this.eventsStore = new PersistentStore("unused");
   }
 
   /**
@@ -100,7 +90,7 @@ class EventsDatabase {
    * @param metric The metric to record to.
    * @param value The value we want to record to the given metric.
    */
-  async record(metric: MetricType, value: RecordedEvent): Promise<void> {
+  async record(metric: EventMetricType, value: RecordedEvent): Promise<void> {
     if (metric.disabled) {
       return;
     }
@@ -127,26 +117,19 @@ class EventsDatabase {
    * @returns an array of `RecordedEvent` containing the found events or `undefined`
    *          if no recorded event was found.
    */
-  async testGetValue(
+  async getEvents(
     ping: string,
-    metric: MetricType
+    metric: EventMetricType
   ): Promise<RecordedEvent[] | undefined> {
-    const value = await this.eventsStore.get([ping]);
-    if (!value) {
+    const events = await this.getAndValidatePingData(ping);
+    if (events.length === 0) {
       return undefined;
     }
 
-    const rawEvents = value as JSONArray;
-    return rawEvents
+    return events
       // Only report events for the requested metric.
       .filter((e) => {
-        const rawEventObj = e as JSONObject;
-        return (rawEventObj["category"] === metric.category)
-          && (rawEventObj["name"] === metric.name);
-      })
-      // Convert them to `RecordedEvent`s.
-      .map((e) => {
-        return RecordedEvent.fromJSONObject(e as JSONObject);
+        return (e.category === metric.category) && (e.name === metric.name);
       });
   }
 
@@ -164,7 +147,7 @@ class EventsDatabase {
    * @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): Promise<JSONArray> {
+  private async getAndValidatePingData(ping: string): Promise<RecordedEvent[]> {
     const data = await this.eventsStore.get([ping]);
     if (isUndefined(data)) {
       return [];
@@ -177,7 +160,7 @@ class EventsDatabase {
       return [];
     }
 
-    return data;
+    return data.map((e) => RecordedEvent.fromJSONObject(e as JSONObject));
   }
 
   /**
@@ -189,7 +172,7 @@ class EventsDatabase {
    * @returns An object containing all the metrics recorded to the given ping,
    *          `undefined` in case the ping doesn't contain any recorded metrics.
    */
-  async getPingMetrics(ping: string, clearPingLifetimeData: boolean): Promise<JSONArray | undefined> {
+  async getPingEvents(ping: string, clearPingLifetimeData: boolean): Promise<JSONArray | undefined> {
     const pingData = await this.getAndValidatePingData(ping);
 
     if (clearPingLifetimeData) {
@@ -202,20 +185,16 @@ class EventsDatabase {
 
     // Sort the events by their timestamp.
     const sortedData = pingData.sort((a, b) => {
-      const objA = a as unknown as RecordedEvent;
-      const objB = b as unknown as RecordedEvent;
-      return objA["timestamp"] - objB["timestamp"];
+      return a.timestamp - b.timestamp;
     });
 
     // Make all the events relative to the first one.
-    const firstTimestamp =
-      (sortedData[0] as unknown as RecordedEvent)["timestamp"];
+    const firstTimestamp = sortedData[0].timestamp;
 
     return sortedData.map((e) => {
-      const objE = e as JSONObject;
-      const timestamp = (objE["timestamp"] as number) ?? 0;
-      objE["timestamp"] = timestamp - firstTimestamp;
-      return objE; 
+      const adjusted = RecordedEvent.toJSONObject(e);
+      adjusted["timestamp"] = e.timestamp - firstTimestamp;
+      return adjusted;
     });
   }
 

src/metrics/index.ts

@@ -131,7 +131,7 @@ export abstract class MetricType implements CommonMetricData {
    * @returns The generated identifier.
    */
   get identifier(): string {
-    if (this.category && this.category.length > 0) {
+    if (this.category.length > 0) {
       return `${this.category}.${this.name}`;
     } else {
       return this.name;

src/metrics/types/event.ts

@@ -5,14 +5,12 @@
 import { MetricType, CommonMetricData } from "metrics";
 import Glean from "glean";
 import { ExtraMap, RecordedEvent } from "metrics/events_database";
+import { isUndefined } from "utils";
 
 const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
 
 /**
- * A datetime metric.
- *
- * Used to record an absolute date and time,
- * such as the time the user first ran the application.
+ * An event metric.
  */
 class EventMetricType extends MetricType {
   private allowedExtraKeys?: string[];
@@ -30,17 +28,21 @@ class EventMetricType extends MetricType {
    * @returns the number of milliseconds since the time origin.
    */
   protected getMonotonicNow(): number {
-    return Math.round(performance.now() / 1000);
+    // Sadly, `performance.now` is not available outside of browsers, which
+    // means we should get creative to find a proper clock. Fall back to `Date.now`
+    // for now, until bug 1690528 is fixed.
+    const now = isUndefined(performance) ? Date.now() : performance.now();
+    return Math.round(now / 1000);
   }
 
   /**
    * Record an event by using the information
    * provided by the instance of this class.
    *
    * @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.
+   *        strings, keys are identifiers. This is used for events where
+   *        additional richer context is needed.
+   *        The maximum length for values is 100 bytes.
    */
   async record(extra?: ExtraMap): Promise<void> {
     if (!this.shouldRecord()) {
@@ -88,7 +90,7 @@ class EventMetricType extends MetricType {
    */
   async testGetValue(ping?: string): Promise<RecordedEvent[] | undefined> {
     const pingToQuery = ping ?? this.sendInPings[0];
-    const events = await Glean.eventsDatabase.testGetValue(pingToQuery, this);
+    const events = await Glean.eventsDatabase.getEvents(pingToQuery, this);
     if (!events) {
       return undefined;
     }

src/metrics/utils.ts

@@ -8,7 +8,6 @@ import { JSONValue } from "utils";
 import { BooleanMetric } from "metrics/types/boolean";
 import { CounterMetric } from "metrics/types/counter";
 import { DatetimeMetric } from "./types/datetime";
-import { EventMetric } from "./types/event";
 import { StringMetric } from "metrics/types/string";
 import { UUIDMetric } from "metrics/types/uuid";
 
@@ -21,7 +20,6 @@ const METRIC_MAP: {
   "boolean": BooleanMetric,
   "counter": CounterMetric,
   "datetime": DatetimeMetric,
-  "event": EventMetric,
   "string": StringMetric,
   "uuid": UUIDMetric,
 });

src/pings/maker.ts

@@ -169,7 +169,7 @@ export async function getPingHeaders(): Promise<Record<string, string> | undefin
  */
 export async function collectPing(ping: PingType, reason?: string): Promise<PingPayload | undefined> {
   const metricsData = await Glean.metricsDatabase.getPingMetrics(ping.name, true);
-  const eventsData = await Glean.eventsDatabase.getPingMetrics(ping.name, true);
+  const eventsData = await Glean.eventsDatabase.getPingEvents(ping.name, true);
   if (!metricsData && !eventsData && !ping.sendIfEmtpy) {
     console.info(`Storage for ${ping.name} empty. Bailing out.`);
     return;

tests/metrics/database.spec.ts

@@ -57,6 +57,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing", "otherPing", "oneMorePing"],
         lifetime: Lifetime.Application,
         disabled: false
@@ -75,6 +76,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: false
@@ -90,6 +92,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: true
@@ -154,6 +157,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing", "otherPing", "oneMorePing"],
         lifetime: Lifetime.Application,
         disabled: false
@@ -174,6 +178,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: true
@@ -192,6 +197,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: false
@@ -205,6 +211,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: false
@@ -217,6 +224,7 @@ describe("MetricsDatabase", function() {
       const db = new Database();
       const metric = new StringMetricType({
         name: "aMetric",
+        category: "",
         sendInPings: ["aPing"],
         lifetime: Lifetime.Application,
         disabled: false

tests/metrics/events_database.spec.ts

@@ -67,7 +67,7 @@ describe("EventsDatabase", function() {
 
   it("getPingMetrics returns undefined if nothing is recorded", async function () {
     const db = new EventsDatabase();
-    const data = await db.getPingMetrics("test-unknown-ping", true);
+    const data = await db.getPingEvents("test-unknown-ping", true);
 
     assert.strictEqual(data, undefined);
   });
@@ -85,7 +85,7 @@ describe("EventsDatabase", function() {
 
     // We didn't record anything yet, so we don't expect anything to be
     // stored.
-    let snapshot = await db.getPingMetrics("store1", false);
+    let snapshot = await db.getPingEvents("store1", false);
     assert.strictEqual(snapshot, undefined);
 
     await db.record(metric, new RecordedEvent(
@@ -95,14 +95,14 @@ describe("EventsDatabase", function() {
     ));
 
     // Take a first snapshot and clear the recorded content.
-    snapshot = await db.getPingMetrics("store1", true);
+    snapshot = await db.getPingEvents("store1", true);
     assert.ok(snapshot != undefined);
 
     // If we snapshot a second time, the store must be empty.
-    const empty_snapshot = await db.getPingMetrics("store1", false);
+    const empty_snapshot = await db.getPingEvents("store1", false);
     assert.strictEqual(empty_snapshot, undefined);
 
-    const store2 = await db.getPingMetrics("store2", false);
+    const store2 = await db.getPingEvents("store2", false);
     for (const events of [snapshot, store2]) {
       assert.ok(events != undefined);
       assert.strictEqual(1, events.length);
@@ -142,7 +142,7 @@ describe("EventsDatabase", function() {
       10000,
     ));
 
-    const snapshot = await db.getPingMetrics("store1", true);
+    const snapshot = await db.getPingEvents("store1", true);
     assert.ok(snapshot);
     assert.strictEqual(3, snapshot.length);
     assert.strictEqual(0, (snapshot[0] as JSONObject)["timestamp"]);