Skip to content

Commit f17b4eb

Browse files
author
Beatriz Rizental
authored
Bug 1679380 - Implement the counter metric type (#14)
1 parent ee299ca commit f17b4eb

File tree

12 files changed

+286
-20
lines changed

12 files changed

+286
-20
lines changed

.eslintrc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@
2020
"no-debugger": ["error"],
2121
"jsdoc/no-types": "off",
2222
"jsdoc/require-param-type": "off",
23-
"jsdoc/require-returns-type": "off",
24-
"jsdoc/check-param-names": ["error"]
23+
"jsdoc/require-returns-type": "off"
2524
},
2625
"extends": [
2726
"plugin:jsdoc/recommended"

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
"scripts": {
77
"test": "npm run build:test-webext && ts-mocha \"tests/**/*.spec.ts\" --paths -p ./tsconfig.json --recursive --timeout 0",
88
"test:debug": "ts-mocha \"tests/**/*.spec.ts\" --paths -p ./tsconfig.json --recursive --inspect-brk",
9-
"lint": "eslint . --ext .ts,.js,.json",
9+
"lint": "eslint . --ext .ts,.js,.json --max-warnings=0",
1010
"fix": "eslint . --ext .ts,.js,.json --fix",
1111
"build:webext": "webpack --config webpack.config.webext.js --mode production",
1212
"dev:webext": "webpack --watch --config webpack.config.webext.js --mode development",

src/database.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ class Database {
110110
* @param metric The metric to record to.
111111
* @param transformFn The transformation function to apply to the currently persisted value.
112112
*/
113-
async transform(metric: Metric, transformFn: (v?: MetricPayload) => MetricPayload): Promise<void> {
113+
async transform<T extends MetricPayload>(metric: Metric, transformFn: (v?: T) => T): Promise<void> {
114114
if (metric.disabled) {
115115
return;
116116
}
@@ -119,7 +119,7 @@ class Database {
119119
const storageKey = metric.identifier;
120120
for (const ping of metric.sendInPings) {
121121
const finalTransformFn = (v: StorageValue): Exclude<StorageValue, undefined> => {
122-
if (!isUndefined(v) && !isMetricPayload(metric.type, v)) {
122+
if (!isUndefined(v) && !isMetricPayload<T>(metric.type, v)) {
123123
throw new Error(`Unexpected value found for metric ${metric.identifier}: ${JSON.stringify(v)}.`);
124124
}
125125
return transformFn(v);
@@ -146,21 +146,19 @@ class Database {
146146
* Currently Glean.js does not include mechanisms to reliably prevent that.
147147
*
148148
* @param ping The ping from which we want to retrieve the given metric.
149-
* @param validateFn A validation function to verify if persisted payload is in the correct format.
150149
* @param metric An object containing the information about the metric to retrieve.
151150
*
152151
* @returns The payload persisted for the given metric,
153152
* `undefined` in case the metric has not been recorded yet or the found valus in invalid.
154153
*/
155154
async getMetric<T>(
156155
ping: string,
157-
validateFn: (v: unknown) => v is T,
158156
metric: Metric
159157
): Promise<T | undefined> {
160158
const store = this._chooseStore(metric.lifetime);
161159
const storageKey = metric.identifier;
162160
const value = await store.get([ping, metric.type, storageKey]);
163-
if (!isUndefined(value) && !validateFn(value)) {
161+
if (!isUndefined(value) && !isMetricPayload<T>(metric.type, value)) {
164162
console.error(`Unexpected value found for metric ${metric.identifier}: ${JSON.stringify(value)}. Clearing.`);
165163
await store.delete([ping, metric.type, storageKey]);
166164
return;

src/metrics/boolean.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ export function isBooleanMetricPayload(v: unknown): v is BooleanMetricPayload {
2020
return isBoolean(v);
2121
}
2222

23+
/**
24+
* A boolean metric.
25+
*
26+
* Records a simple flag.
27+
*/
2328
class BooleanMetric extends Metric {
2429
constructor(meta: CommonMetricData) {
2530
super("boolean", meta);
@@ -52,7 +57,7 @@ class BooleanMetric extends Metric {
5257
* @returns The value found in storage or `undefined` if nothing was found.
5358
*/
5459
async testGetValue(ping: string): Promise<BooleanMetricPayload | undefined> {
55-
return Glean.db.getMetric(ping, isBooleanMetricPayload, this);
60+
return Glean.db.getMetric(ping, this);
5661
}
5762
}
5863

src/metrics/counter.ts

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
import Metric, { CommonMetricData } from "metrics";
6+
import Glean from "glean";
7+
import { isNumber, isUndefined } from "utils";
8+
9+
export const MAX_LENGTH_VALUE = 100;
10+
11+
export type CounterMetricPayload = number;
12+
13+
/**
14+
* Checks whether or not `v` is a valid counter metric payload.
15+
*
16+
* # Note
17+
*
18+
* Not only will this verify if `v` is a number,
19+
* it will also check if it is greater than 0.
20+
*
21+
* @param v The value to verify.
22+
*
23+
* @returns A special Typescript value (which compiles down to a boolean)
24+
* stating wether `v` is a valid string metric payload.
25+
*/
26+
export function isCounterMetricPayload(v: unknown): v is CounterMetricPayload {
27+
if (!isNumber(v)) {
28+
return false;
29+
}
30+
31+
if (v <= 0) {
32+
return false;
33+
}
34+
35+
return true;
36+
}
37+
38+
/**
39+
* A counter metric.
40+
*
41+
* Used to count things.
42+
* The value can only be incremented, not decremented.
43+
*/
44+
class CounterMetric extends Metric {
45+
constructor(meta: CommonMetricData) {
46+
super("counter", meta);
47+
}
48+
49+
/**
50+
* Increases the counter by `amount`.
51+
*
52+
* # Note
53+
*
54+
* - Logs an error if the `amount` is 0 or negative.
55+
* - If the addition yields a number larger than Number.MAX_SAFE_INTEGER,
56+
* Number.MAX_SAFE_INTEGER will be recorded.
57+
*
58+
* @param amount The amount to increase by. Should be positive.
59+
* If not provided will default to `1`.
60+
*/
61+
async add(amount?: number): Promise<void> {
62+
if (!this.shouldRecord()) {
63+
return;
64+
}
65+
66+
if (isUndefined(amount)) {
67+
amount = 1;
68+
}
69+
70+
if (amount <= 0) {
71+
// TODO: record error once Bug 1682574 is resolved.
72+
console.warn(`Attempted to add an invalid amount ${amount}. `);
73+
return;
74+
}
75+
76+
const transformFn = ((amount) => {
77+
return (v?: CounterMetricPayload): CounterMetricPayload => {
78+
let result = v ? v + amount : amount;
79+
if (result > Number.MAX_SAFE_INTEGER) {
80+
result = Number.MAX_SAFE_INTEGER;
81+
}
82+
return result;
83+
};
84+
})(amount);
85+
86+
await Glean.db.transform(this, transformFn);
87+
}
88+
89+
/**
90+
* **Test-only API.**
91+
*
92+
* Gets the currently stored value as a number.
93+
*
94+
* This doesn't clear the stored value.
95+
*
96+
* TODO: Only allow this function to be called on test mode (depends on Bug 1682771).
97+
*
98+
* @param ping the ping from which we want to retrieve this metrics value from.
99+
*
100+
* @returns The value found in storage or `undefined` if nothing was found.
101+
*/
102+
async testGetValue(ping: string): Promise<CounterMetricPayload | undefined> {
103+
return Glean.db.getMetric(ping, this);
104+
}
105+
}
106+
107+
export default CounterMetric;

src/metrics/payload.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
import { BooleanMetricPayload, isBooleanMetricPayload } from "metrics/boolean";
6+
import { CounterMetricPayload, isCounterMetricPayload } from "metrics/counter";
67
import { StringMetricPayload, isStringMetricPayload } from "metrics/string";
78

89
/**
@@ -13,10 +14,12 @@ import { StringMetricPayload, isStringMetricPayload } from "metrics/string";
1314
*
1415
* @returns Whether or not `v` is of the correct type.
1516
*/
16-
export function isMetricPayload(type: string, v: unknown): v is MetricPayload {
17+
export function isMetricPayload<T>(type: string, v: unknown): v is T {
1718
switch (type) {
1819
case "boolean":
1920
return isBooleanMetricPayload(v);
21+
case "counter":
22+
return isCounterMetricPayload(v);
2023
case "string":
2124
return isStringMetricPayload(v);
2225
default:
@@ -27,5 +30,6 @@ export function isMetricPayload(type: string, v: unknown): v is MetricPayload {
2730
// Leaving the `string` as a valid metric payload here so that tests keep working for now.
2831
export type MetricPayload =
2932
BooleanMetricPayload |
33+
CounterMetricPayload |
3034
StringMetricPayload;
3135

src/metrics/string.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ export function isStringMetricPayload(v: unknown): v is StringMetricPayload {
3535
return true;
3636
}
3737

38+
/**
39+
* A string metric.
40+
*
41+
* Record an Unicode string value with arbitrary content.
42+
* Strings are length-limited to `MAX_LENGTH_VALUE` bytes.
43+
*/
3844
class StringMetric extends Metric {
3945
constructor(meta: CommonMetricData) {
4046
super("string", meta);
@@ -77,7 +83,7 @@ class StringMetric extends Metric {
7783
* @returns The value found in storage or `undefined` if nothing was found.
7884
*/
7985
async testGetValue(ping: string): Promise<StringMetricPayload | undefined> {
80-
return Glean.db.getMetric(ping, isStringMetricPayload, this);
86+
return Glean.db.getMetric(ping, this);
8187
}
8288
}
8389

src/storage/index.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { isString, isUndefined, isObject, isBoolean } from "utils";
5+
import { isString, isUndefined, isObject, isBoolean, isNumber } from "utils";
66

77
/**
88
* The storage index in the ordered list of keys to navigate on the store
@@ -28,7 +28,7 @@ export type StorageIndex = string[];
2828
/**
2929
* The possible values to be retrievd from storage.
3030
*/
31-
export type StorageValue = undefined | string | boolean | StorageObject;
31+
export type StorageValue = undefined | string | boolean | number | StorageObject;
3232
export interface StorageObject {
3333
[key: string]: StorageValue;
3434
}
@@ -42,7 +42,7 @@ export interface StorageObject {
4242
* stating whether `v` is a valid StorageValue.
4343
*/
4444
export function isStorageValue(v: unknown): v is StorageValue {
45-
if (isUndefined(v) || isString(v) || isBoolean(v)) {
45+
if (isUndefined(v) || isString(v) || isBoolean(v) || isNumber(v)) {
4646
return true;
4747
}
4848

src/utils.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,15 @@ export function isString(v: unknown): v is string {
4949
export function isBoolean(v: unknown): v is string {
5050
return (typeof v === "boolean" || (typeof v === "object" && v !== null && v.constructor === Boolean));
5151
}
52+
53+
/**
54+
* Checks whether or not `v` is a number.
55+
*
56+
* @param v The value to verify.
57+
*
58+
* @returns A special Typescript value (which compiles down to a boolean)
59+
* stating whether `v` is a number.
60+
*/
61+
export function isNumber(v: unknown): v is number {
62+
return (typeof v === "number" || (typeof v === "object" && v !== null && v.constructor === Number));
63+
}

tests/database.spec.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import Database, { isValidPingPayload } from "database";
88
import { Lifetime } from "metrics";
99
import StringMetric from "metrics/string";
1010
import { MetricPayload } from "metrics/payload";
11-
import { isString } from "utils";
1211

1312
describe("Database", function() {
1413
describe("record", function() {
@@ -208,7 +207,7 @@ describe("Database", function() {
208207
});
209208

210209
await db.record(metric, "aValue");
211-
assert.strictEqual(await db.getMetric("aPing", isString, metric), "aValue");
210+
assert.strictEqual(await db.getMetric("aPing", metric), "aValue");
212211
});
213212

214213
it("doesn't error if trying to get a metric that hasn't been recorded yet", async function() {
@@ -220,7 +219,7 @@ describe("Database", function() {
220219
disabled: false
221220
});
222221

223-
assert.strictEqual(await db.getMetric("aPing", isString, metric), undefined);
222+
assert.strictEqual(await db.getMetric("aPing", metric), undefined);
224223
});
225224

226225
it("deletes entry in case an unexpected value in encountered", async function() {
@@ -237,7 +236,7 @@ describe("Database", function() {
237236
() => ({ "out": "of place" })
238237
);
239238

240-
assert.strictEqual(await db.getMetric("aPing", isString, metric), undefined);
239+
assert.strictEqual(await db.getMetric("aPing", metric), undefined);
241240
assert.strictEqual(await db["appStore"].get(["aPing", "string", "aMetric"]), undefined);
242241
});
243242
});

0 commit comments

Comments
 (0)