Skip to content

Commit a95d85f

Browse files
Dexterp37Dexterp37
committed
Remove circular dependency from labeled.ts
1 parent a635b7d commit a95d85f

File tree

4 files changed

+115
-104
lines changed

4 files changed

+115
-104
lines changed

‎glean/src/core/metrics/database.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ class MetricsDatabase {
131131
}
132132

133133
const store = this._chooseStore(metric.lifetime);
134-
const storageKey = await metric.identifier();
134+
const storageKey = await metric.identifier(this);
135135
for (const ping of metric.sendInPings) {
136136
const finalTransformFn = (v?: JSONValue): JSONValue => transformFn(v).get();
137137
await store.update([ping, metric.type, storageKey], finalTransformFn);
@@ -203,7 +203,7 @@ class MetricsDatabase {
203203
metric: MetricType
204204
): Promise<T | undefined> {
205205
const store = this._chooseStore(metric.lifetime);
206-
const storageKey = await metric.identifier();
206+
const storageKey = await metric.identifier(this);
207207
const value = await store.get([ping, metric.type, storageKey]);
208208
if (!isUndefined(value) && !validateMetricInternalRepresentation<T>(metric.type, value)) {
209209
console.error(`Unexpected value found for metric ${storageKey}: ${JSON.stringify(value)}. Clearing.`);

‎glean/src/core/metrics/index.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44

55
import type { JSONValue } from "../utils.js";
66
import { isUndefined } from "../utils.js";
7-
import LabeledMetricType from "./types/labeled.js";
87
import { Metric } from "./metric.js";
98
import type { Lifetime } from "./lifetime.js";
9+
import type MetricsDatabase from "./database.js";
10+
import { getValidDynamicLabel } from "./types/labeled_utils.js";
1011

1112
/**
1213
* The common set of data shared across all different metric types.
@@ -73,16 +74,18 @@ export abstract class MetricType implements CommonMetricData {
7374
/**
7475
* The metric's unique identifier, including the category, name and label.
7576
*
77+
* @param metricsDatabase The metrics database.
78+
*
7679
* @returns The generated identifier. If `category` is empty, it's ommitted. Otherwise,
7780
* it's the combination of the metric's `category`, `name` and `label`.
7881
*/
79-
async identifier(): Promise<string> {
82+
async identifier(metricsDatabase: MetricsDatabase): Promise<string> {
8083
const baseIdentifier = this.baseIdentifier();
8184

8285
// We need to use `isUndefined` and cannot use `(this.dynamicLabel)` because we want
8386
// empty strings to propagate as dynamic labels, so that erros are potentially recorded.
8487
if (!isUndefined(this.dynamicLabel)) {
85-
return await LabeledMetricType.getValidDynamicLabel(this);
88+
return await getValidDynamicLabel(metricsDatabase, this);
8689
} else {
8790
return baseIdentifier;
8891
}

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

Lines changed: 3 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -2,40 +2,11 @@
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 type { CommonMetricData, MetricType } from "../index.js";
6-
import Glean from "../../glean.js";
5+
import type { CommonMetricData } from "../index.js";
76
import type CounterMetricType from "./counter.js";
87
import type BooleanMetricType from "./boolean.js";
98
import type StringMetricType from "./string.js";
10-
11-
const MAX_LABELS = 16;
12-
const OTHER_LABEL = "__other__";
13-
const MAX_LABEL_LENGTH = 61;
14-
15-
// ** IMPORTANT **
16-
// When changing this documentation or the regex, be sure to change the same code
17-
// in the Glean SDK repository as well.
18-
//
19-
// This regex is used for matching against labels and should allow for dots,
20-
// underscores, and/or hyphens. Labels are also limited to starting with either
21-
// a letter or an underscore character.
22-
//
23-
// Some examples of good and bad labels:
24-
//
25-
// Good:
26-
// * `this.is.fine`
27-
// * `this_is_fine_too`
28-
// * `this.is_still_fine`
29-
// * `thisisfine`
30-
// * `_.is_fine`
31-
// * `this.is-fine`
32-
// * `this-is-fine`
33-
// Bad:
34-
// * `this.is.not_fine_due_tu_the_length_being_too_long_i_thing.i.guess`
35-
// * `1.not_fine`
36-
// * `this.$isnotfine`
37-
// * `-.not_fine`
38-
const LABEL_REGEX = /^[a-z_][a-z0-9_-]{0,29}(\.[a-z_][a-z0-9_-]{0,29})*$/;
9+
import { combineIdentifierAndLabel, OTHER_LABEL } from "./labeled_utils.js";
3910

4011
type SupportedLabeledTypes = CounterMetricType | BooleanMetricType | StringMetricType;
4112

@@ -62,21 +33,6 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
6233
});
6334
}
6435

65-
/**
66-
* Combines a metric's base identifier and label.
67-
*
68-
* @param metricName the metric base identifier
69-
* @param label the label
70-
*
71-
* @returns a string representing the complete metric id including the label.
72-
*/
73-
private static combineIdentifierAndLabel(
74-
metricName: string,
75-
label: string
76-
): string {
77-
return `${metricName}/${label}`;
78-
}
79-
8036
/**
8137
* Create an instance of the submetric type for the provided static label.
8238
*
@@ -99,7 +55,7 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
9955
const adjustedLabel = allowedLabels.includes(label) ? label : OTHER_LABEL;
10056
const newMeta: CommonMetricData = {
10157
...meta,
102-
name: LabeledMetricType.combineIdentifierAndLabel(meta.name, adjustedLabel)
58+
name: combineIdentifierAndLabel(meta.name, adjustedLabel)
10359
};
10460
return new submetricClass(newMeta);
10561
}
@@ -125,58 +81,6 @@ class LabeledMetricType<T extends SupportedLabeledTypes> {
12581
};
12682
return new submetricClass(newMeta);
12783
}
128-
129-
/**
130-
* Checks if the dynamic label stored in the metric data is
131-
* valid. If not, record an error and store data in the "__other__"
132-
* label.
133-
*
134-
* @param metric the metric metadata.
135-
*
136-
* @returns a valid label that can be used to store data.
137-
*/
138-
static async getValidDynamicLabel(metric: MetricType): Promise<string> {
139-
// Note that we assume `metric.dynamicLabel` to always be available within this function.
140-
// This is a safe assumptions because we should only call `getValidDynamicLabel` if we have
141-
// a dynamic label.
142-
if (metric.dynamicLabel === undefined) {
143-
throw new Error("This point should never be reached.");
144-
}
145-
146-
const key = LabeledMetricType.combineIdentifierAndLabel(metric.baseIdentifier(), metric.dynamicLabel);
147-
148-
for (const ping of metric.sendInPings) {
149-
if (await Glean.metricsDatabase.hasMetric(metric.lifetime, ping, metric.type, key)) {
150-
return key;
151-
}
152-
}
153-
154-
let numUsedKeys = 0;
155-
for (const ping of metric.sendInPings) {
156-
numUsedKeys += await Glean.metricsDatabase.countByBaseIdentifier(
157-
metric.lifetime,
158-
ping,
159-
metric.type,
160-
metric.baseIdentifier());
161-
}
162-
163-
let hitError = false;
164-
if (numUsedKeys >= MAX_LABELS) {
165-
hitError = true;
166-
} else if (metric.dynamicLabel.length > MAX_LABEL_LENGTH) {
167-
console.error(`label length ${metric.dynamicLabel.length} exceeds maximum of ${MAX_LABEL_LENGTH}`);
168-
hitError = true;
169-
// TODO: record error in bug 1682574
170-
} else if (!LABEL_REGEX.test(metric.dynamicLabel)) {
171-
console.error(`label must be snake_case, got '${metric.dynamicLabel}'`);
172-
hitError = true;
173-
// TODO: record error in bug 1682574
174-
}
175-
176-
return (hitError)
177-
? LabeledMetricType.combineIdentifierAndLabel(metric.baseIdentifier(), OTHER_LABEL)
178-
: key;
179-
}
18084
}
18185

18286
export default LabeledMetricType;

‎glean/src/core/metrics/types/labeled_utils.ts‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
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 type { MetricType } from "..";
6+
import type MetricsDatabase from "../database";
7+
8+
const MAX_LABELS = 16;
9+
const MAX_LABEL_LENGTH = 61;
10+
export const OTHER_LABEL = "__other__";
11+
12+
// ** IMPORTANT **
13+
// When changing this documentation or the regex, be sure to change the same code
14+
// in the Glean SDK repository as well.
15+
//
16+
// This regex is used for matching against labels and should allow for dots,
17+
// underscores, and/or hyphens. Labels are also limited to starting with either
18+
// a letter or an underscore character.
19+
//
20+
// Some examples of good and bad labels:
21+
//
22+
// Good:
23+
// * `this.is.fine`
24+
// * `this_is_fine_too`
25+
// * `this.is_still_fine`
26+
// * `thisisfine`
27+
// * `_.is_fine`
28+
// * `this.is-fine`
29+
// * `this-is-fine`
30+
// Bad:
31+
// * `this.is.not_fine_due_tu_the_length_being_too_long_i_thing.i.guess`
32+
// * `1.not_fine`
33+
// * `this.$isnotfine`
34+
// * `-.not_fine`
35+
const LABEL_REGEX = /^[a-z_][a-z0-9_-]{0,29}(\.[a-z_][a-z0-9_-]{0,29})*$/;
36+
37+
/**
38+
* Combines a metric's base identifier and label.
39+
*
40+
* @param metricName the metric base identifier
41+
* @param label the label
42+
*
43+
* @returns a string representing the complete metric id including the label.
44+
*/
45+
export function combineIdentifierAndLabel(
46+
metricName: string,
47+
label: string
48+
): string {
49+
return `${metricName}/${label}`;
50+
}
51+
52+
/**
53+
* Checks if the dynamic label stored in the metric data is
54+
* valid. If not, record an error and store data in the "__other__"
55+
* label.
56+
*
57+
* @param metricsDatabase the metrics database.
58+
* @param metric the metric metadata.
59+
*
60+
* @returns a valid label that can be used to store data.
61+
*/
62+
export async function getValidDynamicLabel(metricsDatabase: MetricsDatabase, metric: MetricType): Promise<string> {
63+
// Note that we assume `metric.dynamicLabel` to always be available within this function.
64+
// This is a safe assumptions because we should only call `getValidDynamicLabel` if we have
65+
// a dynamic label.
66+
if (metric.dynamicLabel === undefined) {
67+
throw new Error("This point should never be reached.");
68+
}
69+
70+
const key = combineIdentifierAndLabel(metric.baseIdentifier(), metric.dynamicLabel);
71+
72+
for (const ping of metric.sendInPings) {
73+
if (await metricsDatabase.hasMetric(metric.lifetime, ping, metric.type, key)) {
74+
return key;
75+
}
76+
}
77+
78+
let numUsedKeys = 0;
79+
for (const ping of metric.sendInPings) {
80+
numUsedKeys += await metricsDatabase.countByBaseIdentifier(
81+
metric.lifetime,
82+
ping,
83+
metric.type,
84+
metric.baseIdentifier());
85+
}
86+
87+
let hitError = false;
88+
if (numUsedKeys >= MAX_LABELS) {
89+
hitError = true;
90+
} else if (metric.dynamicLabel.length > MAX_LABEL_LENGTH) {
91+
console.error(`label length ${metric.dynamicLabel.length} exceeds maximum of ${MAX_LABEL_LENGTH}`);
92+
hitError = true;
93+
// TODO: record error in bug 1682574
94+
} else if (!LABEL_REGEX.test(metric.dynamicLabel)) {
95+
console.error(`label must be snake_case, got '${metric.dynamicLabel}'`);
96+
hitError = true;
97+
// TODO: record error in bug 1682574
98+
}
99+
100+
return (hitError)
101+
? combineIdentifierAndLabel(metric.baseIdentifier(), OTHER_LABEL)
102+
: key;
103+
}
104+

0 commit comments

Comments
 (0)