Add smoke test for the rate metric

Author: ChinYing-Li

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

@@ -7,9 +7,8 @@ import { MetricType } from "../index.js";
 import { Context } from "../../context.js";
 import { Metric } from "../metric.js";
 import { testOnly } from "../../utils.js";
-import type { JSONValue } from "../../utils.js";
+import { isNumber, isObject, JSONValue } from "../../utils.js";
 import { ErrorType } from "../../error/error_type.js";
-import { isMap } from "util/types";
 
 const LOG_TAG = "core.metrics.RateMetricType";
 
@@ -18,26 +17,36 @@ export type RateInternalRepresentation = {
   numerator: number,
   // The denominator
   denominator: number
-}
+};
 
 export type RatePayloadRepresentation = {
   // numerator
   numerator: number,
   // The denominator
   denominator: number
-}
+};
 
 export class RateMetric extends Metric<RateInternalRepresentation, RatePayloadRepresentation> {
   constructor(v: unknown) {
     super(v);
   }
 
+  get numerator(): number {
+    return this._inner.numerator;
+  }
+
+  get denominator(): number {
+    return this._inner.denominator;
+  }
+
   validate(v: unknown): v is RateInternalRepresentation {
-    if (!isMap(v)) {
+    if (!isObject(v) || Object.keys(v).length !== 2) {
       return false;
     }
 
-    return ("numerator" in v && "denominator" in v && Object.keys(v).length == 2);
+    const numeratorVerification = "numerator" in v && isNumber(v.numerator) && v.numerator >= 0;
+    const denominatorVerification = "denominator" in v && isNumber(v.denominator) && v.denominator >= 0;
+    return numeratorVerification && denominatorVerification;
   }
 
   payload(): RatePayloadRepresentation {
@@ -92,11 +101,11 @@ class RateMetricType extends MetricType {
           let result: number;
           try {
             metric = new RateMetric(v);
-            result = metric.get().numerator + amount;
+            result = metric.numerator + amount;
           } catch {
             metric = new RateMetric({
               numerator: amount,
-              denominator: 1
+              denominator: 0
             });
             result = amount;
           }
@@ -107,7 +116,7 @@ class RateMetricType extends MetricType {
 
           metric.set({
             numerator: result,
-            denominator: metric.get().denominator
+            denominator: metric.denominator
           });
           return metric;
         };
@@ -146,10 +155,10 @@ class RateMetricType extends MetricType {
           let result: number;
           try {
             metric = new RateMetric(v);
-            result = metric.get().denominator + amount;
+            result = metric.denominator + amount;
           } catch {
             metric = new RateMetric({
-              numerator: 1,
+              numerator: 0,
               denominator: amount
             });
             result = amount;
@@ -160,7 +169,7 @@ class RateMetricType extends MetricType {
           }
 
           metric.set({
-            numerator: metric.get().numerator,
+            numerator: metric.numerator,
             denominator: result
           });
           return metric;

glean/src/core/metrics/utils.ts

@@ -11,6 +11,7 @@ import { BooleanMetric } from "./types/boolean.js";
 import { CounterMetric } from "./types/counter.js";
 import { DatetimeMetric } from "./types/datetime.js";
 import { QuantityMetric } from "./types/quantity.js";
+import { RateMetric } from "./types/rate.js";
 import { StringMetric } from "./types/string.js";
 import { StringListMetric } from "./types/string_list.js";
 import { TextMetric } from "./types/text.js";
@@ -31,6 +32,7 @@ const METRIC_MAP: {
   "labeled_counter": LabeledMetric,
   "labeled_string": LabeledMetric,
   "quantity": QuantityMetric,
+  "rate": RateMetric,
   "string": StringMetric,
   "string_list": StringListMetric,
   "text": TextMetric,

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

@@ -3,12 +3,11 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import assert from "assert";
-import { Context } from "../../../../src/core/context";
 import { ErrorType } from "../../../../src/core/error/error_type";
 
 import Glean from "../../../../src/core/glean";
 import { Lifetime } from "../../../../src/core/metrics/lifetime";
-import QuantityMetricType from "../../../../src/core/metrics/types/quantity";
+import RateMetricType from "../../../../src/core/metrics/types/rate";
 
 describe("RateMetric", function() {
   const testAppId = `gleanjs.test.${this.title}`;
@@ -17,93 +16,32 @@ describe("RateMetric", function() {
     await Glean.testResetGlean(testAppId);
   });
 
-  it("attempting to get the value of a metric that hasn't been recorded doesn't error", async function() {
-    const metric = new QuantityMetricType({
-      category: "aCategory",
-      name: "aQuantityMetric",
-      sendInPings: ["aPing", "twoPing", "threePing"],
-      lifetime: Lifetime.Ping,
-      disabled: false
-    });
-
-    assert.strictEqual(await metric.testGetValue("aPing"), undefined);
-  });
-
-  it("attempting to set when glean upload is disabled is a no-op", async function() {
-    Glean.setUploadEnabled(false);
-
-    const metric = new QuantityMetricType({
-      category: "aCategory",
-      name: "aQuantityMetric",
-      sendInPings: ["aPing", "twoPing", "threePing"],
-      lifetime: Lifetime.Ping,
-      disabled: false
-    });
-
-    metric.set(10);
-    assert.strictEqual(await metric.testGetValue("aPing"), undefined);
-  });
+  it("smoke test for rate metric", async function() {
+    Glean.setUploadEnabled(true);
 
-  it("ping payload is correct", async function() {
-    const metric = new QuantityMetricType({
+    const metric = new RateMetricType({
       category: "aCategory",
-      name: "aQuantityMetric",
+      name: "aRate",
       sendInPings: ["aPing"],
       lifetime: Lifetime.Ping,
       disabled: false
     });
 
-    metric.set(10);
-    assert.strictEqual(await metric.testGetValue("aPing"), 10);
+    metric.add_to_numerator(0);
+    metric.add_to_denominator(0);
 
-    const snapshot = await Context.metricsDatabase.getPingMetrics("aPing", true);
-    assert.deepStrictEqual(snapshot, {
-      "quantity": {
-        "aCategory.aQuantityMetric": 10
-      }
-    });
-  });
+    assert.strictEqual(await metric.testGetNumRecordedErrors(ErrorType.InvalidValue), 0);
 
-  it("set properly sets the value in all pings", async function() {
-    const metric = new QuantityMetricType({
-      category: "aCategory",
-      name: "aQuantityMetric",
-      sendInPings: ["aPing", "twoPing", "threePing"],
-      lifetime: Lifetime.Ping,
-      disabled: false
-    });
+    metric.add_to_numerator(-1);
+    metric.add_to_denominator(-1);
 
-    metric.set(0);
-    assert.strictEqual(await metric.testGetValue("aPing"), 0);
-    assert.strictEqual(await metric.testGetValue("twoPing"), 0);
-    assert.strictEqual(await metric.testGetValue("threePing"), 0);
-  });
+    assert.strictEqual(await metric.testGetNumRecordedErrors(ErrorType.InvalidValue), 2);
 
-  it("must not set when passed negative", async function() {
-    const metric = new QuantityMetricType({
-      category: "aCategory",
-      name: "aQuantityMetric",
-      sendInPings: ["aPing"],
-      lifetime: Lifetime.Ping,
-      disabled: false
-    });
+    assert.deepStrictEqual(await metric.testGetValue("aPing"), {numerator: 0, denominator: 0});
 
-    metric.set(-1);
-    assert.strictEqual(await metric.testGetValue("aPing"), undefined);
+    metric.add_to_numerator(22);
+    metric.add_to_denominator(7);
 
-    assert.strictEqual(await metric.testGetNumRecordedErrors(ErrorType.InvalidValue, "aPing"), 1);
-  });
-
-  it("saturates at boundary", async function() {
-    const metric = new QuantityMetricType({
-      category: "aCategory",
-      name: "aQuantityMetric",
-      sendInPings: ["aPing"],
-      lifetime: Lifetime.Ping,
-      disabled: false
-    });
-
-    metric.set(Number.MAX_SAFE_INTEGER+1);
-    assert.strictEqual(await metric.testGetValue("aPing"), Number.MAX_SAFE_INTEGER);
-  });
-});
+    assert.deepStrictEqual(await metric.testGetValue("aPing"), {numerator: 22, denominator: 7});
+ });
+});