Address review comments

Author: Dexterp37

CHANGELOG.md

@@ -10,7 +10,7 @@
 * [#101](https://github.com/mozilla/glean.js/pull/101): BUGFIX: Only validate Debug View Tag and Source Tags when they are present.
 * [#101](https://github.com/mozilla/glean.js/pull/101): BUGFIX: Only validate Debug View Tag and Source Tags when they are present.
 * [#102](https://github.com/mozilla/glean.js/pull/102): BUGFIX: Include a Glean User-Agent header in all pings.
-* [#97](https://github.com/mozilla/glean.js/pull/97): Add support for labelled metric types (string, boolean and counter).
+* [#97](https://github.com/mozilla/glean.js/pull/97): Add support for labeled metric types (string, boolean and counter).
 
 # v0.4.0 (2021-03-10)
 

glean/src/core/metrics/database.ts

@@ -133,7 +133,7 @@ class MetricsDatabase {
     }
 
     const store = this._chooseStore(metric.lifetime);
-    const storageKey = await metric.getAsyncIdentifier();
+    const storageKey = await metric.identifier();
     for (const ping of metric.sendInPings) {
       const finalTransformFn = (v?: JSONValue): JSONValue => transformFn(v).get();
       await store.update([ping, metric.type, storageKey], finalTransformFn);
@@ -205,7 +205,7 @@ class MetricsDatabase {
     metric: MetricType
   ): Promise<T | undefined> {
     const store = this._chooseStore(metric.lifetime);
-    const storageKey = await metric.getAsyncIdentifier();
+    const storageKey = await metric.identifier();
     const value = await store.get([ping, metric.type, storageKey]);
     if (!isUndefined(value) && !validateMetricInternalRepresentation<T>(metric.type, value)) {
       console.error(`Unexpected value found for metric ${storageKey}: ${JSON.stringify(value)}. Clearing.`);

glean/src/core/metrics/index.ts

@@ -155,11 +155,11 @@ export abstract class MetricType implements CommonMetricData {
    * @returns The generated identifier. If `category` is empty, it's ommitted. Otherwise,
    *          it's the combination of the metric's `category`, `name` and `label`.
    */
-  async getAsyncIdentifier(): Promise<string> {
+  async identifier(): Promise<string> {
     const baseIdentifier = this.baseIdentifier();
 
     // We need to use `isUndefined` and cannot use `(this.dynamicLabel)` because we want
-    // empty strings to propagate as a dynamic labels, so that erros are potentially recorded.
+    // empty strings to propagate as dynamic labels, so that erros are potentially recorded.
     if (!isUndefined(this.dynamicLabel)) {
       return await LabeledMetricType.getValidDynamicLabel(this);
     } else {
@@ -178,7 +178,7 @@ export abstract class MetricType implements CommonMetricData {
 }
 
 /**
- * This is no-op internal metric representation.
+ * This is a no-op internal metric representation.
  *
  * This can be used to instruct the validators to simply report
  * whatever is stored internally, without performing any specific

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

@@ -52,13 +52,12 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
     labels?: string[],
   ) {
     return new Proxy(this, {
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      get: (_target: LabeledMetricType<T>, label: string): any => {
+      get: (_target: LabeledMetricType<T>, label: string): T => {
         if (labels) {
-          return LabeledMetricType.createFromStaticLabel<typeof submetric>(meta, submetric, labels, label);
+          return LabeledMetricType.createFromStaticLabel(meta, submetric, labels, label);
         }
 
-        return LabeledMetricType.createFromDynamicLabel<typeof submetric>(meta, submetric, label);
+        return LabeledMetricType.createFromDynamicLabel(meta, submetric, label);
       }
     });
   }
@@ -94,7 +93,7 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
     submetricClass: T,
     allowedLabels: string[],
     label: string
-  ): T {
+  ): InstanceType<T> {
     // If the label was provided in the registry file, then use it. Otherwise,
     // store data in the `OTHER_LABEL`.
     const adjustedLabel = allowedLabels.includes(label) ? label : OTHER_LABEL;
@@ -119,14 +118,23 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
     meta: CommonMetricData,
     submetricClass: T,
     label: string
-  ): T {
+  ): InstanceType<T> {
     const newMeta: CommonMetricData = {
       ...meta,
       dynamicLabel: label
     };
     return new submetricClass(newMeta);
   }
 
+  /**
+   * 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 metric the metric metadata.
+   *
+   * @returns a valid label that can be used to store data.
+   */
   static async getValidDynamicLabel(metric: MetricType): Promise<string> {
     // Note that we assume `metric.dynamicLabel` to always be available within this function.
     // This is a safe assumptions because we should only call `getValidDynamicLabel` if we have

glean/tests/core/metrics/labeled.spec.ts

@@ -17,8 +17,10 @@ import PingType from "../../../src/core/pings";
 const sandbox = sinon.createSandbox();
 
 describe("LabeledMetric", function() {
+  const testAppId = `gleanjs.test.${this.title}`;
+
   beforeEach(async function() {
-    await Glean.testResetGlean("gleanjs.unit.test");
+    await Glean.testResetGlean(testAppId);
     // Disable ping uploading for it not to interfere with this tests.
     sandbox.stub(Glean["pingUploader"], "triggerUpload").callsFake(() => Promise.resolve());
   });
@@ -34,7 +36,7 @@ describe("LabeledMetric", function() {
       sendIfEmpty: false,
     });
 
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -79,7 +81,7 @@ describe("LabeledMetric", function() {
       sendIfEmpty: false,
     });
 
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -126,7 +128,7 @@ describe("LabeledMetric", function() {
   });
 
   it("test __other__ label without predefined labels", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -151,7 +153,7 @@ describe("LabeledMetric", function() {
   });
 
   it("test __other__ label without predefined labels before Glean initialization", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -181,7 +183,7 @@ describe("LabeledMetric", function() {
   });
 
   it("Ensure invalid labels on labeled counter go to __other__", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -203,7 +205,7 @@ describe("LabeledMetric", function() {
   });
 
   it("Ensure invalid labels on labeled boolean go to __other__", async function() {
-    const labeledBooleanMetric = new LabeledMetricType<BooleanMetricType>(
+    const labeledBooleanMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_boolean_metric",
@@ -225,7 +227,7 @@ describe("LabeledMetric", function() {
   });
 
   it("Ensure invalid labels on labeled string go to __other__", async function() {
-    const labeledStringMetric = new LabeledMetricType<StringMetricType>(
+    const labeledStringMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_string_metric",
@@ -247,7 +249,7 @@ describe("LabeledMetric", function() {
   });
 
   it("test labeled string metric type", async function() {
-    const labeledStringMetric = new LabeledMetricType<StringMetricType>(
+    const labeledStringMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_string_metric",
@@ -269,7 +271,7 @@ describe("LabeledMetric", function() {
   });
 
   it("test labeled boolean metric type", async function() {
-    const metric = new LabeledMetricType<BooleanMetricType>(
+    const metric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_bool",
@@ -291,7 +293,7 @@ describe("LabeledMetric", function() {
   });
 
   it("dynamic labels regex mismatch", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -320,7 +322,7 @@ describe("LabeledMetric", function() {
   });
 
   it("dynamic labels regex allowed", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_counter_metric",
@@ -350,7 +352,7 @@ describe("LabeledMetric", function() {
   });
 
   it("seen labels get reloaded across initializations", async function() {
-    const labeledCounterMetric = new LabeledMetricType<CounterMetricType>(
+    const labeledCounterMetric = new LabeledMetricType(
       {
         category: "telemetry",
         name: "labeled_metric",