Skip to content

Commit a3d4030

Browse files
Dexterp37Dexterp37
authored
Merge pull request #1849 from rosahbruno/1868748-trim-event-extras-to-500-bytes
Bug 1868748 - truncate event extra strings to 500 bytes
2 parents 27c2441 + 4cafdcd commit a3d4030

File tree

10 files changed

+74
-32
lines changed

10 files changed

+74
-32
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,28 +2,27 @@
22

33
[Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.2...main)
44

5-
[#1848](https://github.com/mozilla/glean.js/pull/1848): Support for automatically collecting element click events (first version)
5+
* [#1848](https://github.com/mozilla/glean.js/pull/1848): Support for automatically collecting element click events (first version)
6+
* [#1849](https://github.com/mozilla/glean.js/pull/1849): Truncate event extra strings to 500 bytes. This also updates other string-based metrics to truncate based on max bytes rather than a set number of characters.
67

78
# v4.0.0-pre.2 (2023-12-06)
89

910
[Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.1...v4.0.0-pre.2)
1011

11-
[#1835](https://github.com/mozilla/glean.js/pull/1835): Added support for automatic page load instrumentation.
12-
12+
* [#1835](https://github.com/mozilla/glean.js/pull/1835): Added support for automatic page load instrumentation.
1313
* [#1846](https://github.com/mozilla/glean.js/pull/1846): Add logging messages when using the debugging APIs from the browser console.
1414

1515
# v4.0.0-pre.1 (2023-12-01)
1616

1717
[Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.0...v4.0.0-pre.1)
1818

19-
[#1834](https://github.com/mozilla/glean.js/pull/1834): Added support for `navigator.sendBeacon`. This is not turned on by default and needs to be enabled manually.
19+
* [#1834](https://github.com/mozilla/glean.js/pull/1834): Added support for `navigator.sendBeacon`. This is not turned on by default and needs to be enabled manually.
2020

2121
# v4.0.0-pre.0 (2023-11-27)
2222

2323
[Full changelog](https://github.com/mozilla/glean.js/compare/v3.0.0...v4.0.0-pre.0)
2424

25-
[#1808](https://github.com/mozilla/glean.js/pull/1808): **BREAKING CHANGE**: Make glean.js fully synchronous.
26-
25+
* [#1808](https://github.com/mozilla/glean.js/pull/1808): **BREAKING CHANGE**: Make glean.js fully synchronous.
2726
* [#1835](https://github.com/mozilla/glean.js/pull/1835): Automatic instrumentation of page load events for simple web properties.
2827

2928
# v3.0.0 (2023-11-16)

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,14 @@ import {
1111
getMonotonicNow,
1212
isString,
1313
testOnlyCheck,
14-
truncateStringAtBoundaryWithError
14+
truncateStringAtBytesBoundaryWithError
1515
} from "../../utils.js";
1616
import { Context } from "../../context.js";
1717
import { ErrorType } from "../../error/error_type.js";
1818
import { MetricValidationError } from "../metric.js";
1919

2020
const LOG_TAG = "core.metrics.EventMetricType";
21-
const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
21+
const MAX_BYTES_EXTRA_KEY_VALUE = 500;
2222

2323
/**
2424
* Base implementation of the event metric type,
@@ -65,10 +65,10 @@ export class InternalEventMetricType<
6565
for (const [name, value] of Object.entries(extra)) {
6666
if (this.allowedExtraKeys.includes(name)) {
6767
if (isString(value)) {
68-
truncatedExtra[name] = truncateStringAtBoundaryWithError(
68+
truncatedExtra[name] = truncateStringAtBytesBoundaryWithError(
6969
this,
7070
value,
71-
MAX_LENGTH_EXTRA_KEY_VALUE
71+
MAX_BYTES_EXTRA_KEY_VALUE
7272
);
7373
} else {
7474
truncatedExtra[name] = value;

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ import { Context } from "../../context.js";
99
import { MetricType } from "../index.js";
1010
import { Metric, MetricValidationError } from "../metric.js";
1111
import { validateString } from "../utils.js";
12-
import { testOnlyCheck, truncateStringAtBoundaryWithError, } from "../../utils.js";
12+
import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
1313

1414
const LOG_TAG = "core.metrics.StringMetricType";
15-
export const MAX_LENGTH_VALUE = 100;
15+
export const MAX_STRING_LENGTH_IN_BYTES = 100;
1616

1717
export class StringMetric extends Metric<string, string> {
1818
constructor(v: unknown) {
@@ -46,7 +46,12 @@ export class InternalStringMetricType extends MetricType {
4646
}
4747

4848
try {
49-
const truncatedValue = truncateStringAtBoundaryWithError(this, value, MAX_LENGTH_VALUE);
49+
const truncatedValue = truncateStringAtBytesBoundaryWithError(
50+
this,
51+
value,
52+
MAX_STRING_LENGTH_IN_BYTES
53+
);
54+
5055
const metric = new StringMetric(truncatedValue);
5156
Context.metricsDatabase.record(this, metric);
5257
} catch (e) {

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ import { ErrorType } from "../../error/error_type.js";
1212
import { MetricType } from "../index.js";
1313
import { Metric, MetricValidation, MetricValidationError } from "../metric.js";
1414
import { validateString } from "../utils.js";
15-
import { testOnlyCheck, truncateStringAtBoundaryWithError } from "../../utils.js";
15+
import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
1616

1717
const LOG_TAG = "core.metrics.StringListMetricType";
1818
export const MAX_LIST_LENGTH = 20;
19-
export const MAX_STRING_LENGTH = 50;
19+
export const MAX_STRING_LENGTH_IN_BYTES = 100;
2020

2121
export class StringListMetric extends Metric<string[], string[]> {
2222
constructor(v: unknown) {
@@ -89,10 +89,10 @@ class InternalStringListMetricType extends MetricType {
8989

9090
const truncatedList: string[] = [];
9191
for (let i = 0; i < Math.min(value.length, MAX_LIST_LENGTH); ++i) {
92-
const truncatedString = truncateStringAtBoundaryWithError(
92+
const truncatedString = truncateStringAtBytesBoundaryWithError(
9393
this,
9494
value[i],
95-
MAX_STRING_LENGTH
95+
MAX_STRING_LENGTH_IN_BYTES
9696
);
9797
truncatedList.push(truncatedString);
9898
}
@@ -112,7 +112,7 @@ class InternalStringListMetricType extends MetricType {
112112
}
113113

114114
try {
115-
const truncatedValue = truncateStringAtBoundaryWithError(this, value, MAX_STRING_LENGTH);
115+
const truncatedValue = truncateStringAtBytesBoundaryWithError(this, value, MAX_STRING_LENGTH_IN_BYTES);
116116

117117
Context.metricsDatabase.transform(
118118
this,

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ import { Metric } from "../metric.js";
1010
import { MetricType } from "../index.js";
1111
import { MetricValidationError } from "../metric.js";
1212
import { validateString } from "../utils.js";
13-
import { testOnlyCheck, truncateStringAtBoundaryWithError, } from "../../utils.js";
13+
import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
1414

1515
const LOG_TAG = "core.metrics.TextMetricType";
16-
// The maximum number of characters for text.
17-
export const TEXT_MAX_LENGTH = 200 * 1024;
16+
17+
export const MAX_TEXT_LENGTH_IN_BYTES = 200 * 1024;
1818

1919
export class TextMetric extends Metric<string, string> {
2020
constructor(v: unknown) {
@@ -54,7 +54,12 @@ class InternalTextMetricType extends MetricType {
5454
}
5555

5656
try {
57-
const truncatedValue = truncateStringAtBoundaryWithError(this, text, TEXT_MAX_LENGTH);
57+
const truncatedValue = truncateStringAtBytesBoundaryWithError(
58+
this,
59+
text,
60+
MAX_TEXT_LENGTH_IN_BYTES
61+
);
62+
5863
const metric = new TextMetric(truncatedValue);
5964
Context.metricsDatabase.record(this, metric);
6065
} catch (e) {

‎glean/src/core/utils.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,39 @@ export function truncateStringAtBoundaryWithError(metric: MetricType, value: unk
241241
return truncated;
242242
}
243243

244+
/**
245+
* Truncates a string to a given max number of bytes.
246+
*
247+
* If the string required truncation, records an error through the error
248+
* reporting mechanism.
249+
*
250+
* @param metric The metric to record an error to, if necessary,
251+
* @param value The string to truncate.
252+
* @param maxBytes The max number of bytes to truncate to.
253+
* @returns A string with at most `maxLength` bytes.
254+
* @throws In case `value` is not a string.
255+
*/
256+
export function truncateStringAtBytesBoundaryWithError(metric: MetricType, value: unknown, maxBytes: number): string {
257+
if (!isString(value)) {
258+
throw new MetricValidationError(`Expected string, got ${JSON.stringify(value)}`);
259+
}
260+
261+
const encoder = new TextEncoder();
262+
const decoder = new TextDecoder("utf-8");
263+
264+
const uint8 = encoder.encode(value);
265+
const section = uint8.slice(0, maxBytes);
266+
const truncated = decoder.decode(section);
267+
if (truncated !== value) {
268+
Context.errorManager.record(
269+
metric,
270+
ErrorType.InvalidOverflow,
271+
`Value length ${new Blob([value]).size} exceeds maximum of ${value.length} bytes.`
272+
);
273+
}
274+
return truncated;
275+
}
276+
244277
/**
245278
* Decorator factory that will only allow a function to be called when Glean is in testing mode.
246279
*

‎glean/tests/unit/core/metrics/event.spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ describe("EventMetric", function() {
126126
disabled: false
127127
}, ["label"]);
128128

129-
metric.record({ label: "01234567890".repeat(20) });
129+
metric.record({ label: "01234567890".repeat(100) });
130130
assert.strictEqual(metric.testGetNumRecordedErrors(ErrorType.InvalidOverflow), 1);
131131
});
132132

@@ -210,7 +210,7 @@ describe("EventMetric", function() {
210210
const testValue = "LeanGleanByFrank";
211211
const extra = {
212212
"extra1": testValue,
213-
"truncatedExtra": testValue.repeat(10),
213+
"truncatedExtra": testValue.repeat(50),
214214
};
215215

216216
testEvent.record(extra);
@@ -225,7 +225,7 @@ describe("EventMetric", function() {
225225
assert.strictEqual(2, Object.keys(event.extra).length);
226226
assert.strictEqual(testValue, event.extra["extra1"]);
227227
assert.strictEqual(
228-
testValue.repeat(10).substr(0, 100),
228+
testValue.repeat(50).substr(0, 500),
229229
event.extra["truncatedExtra"]
230230
);
231231
});

‎glean/tests/unit/core/metrics/string.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import assert from "assert";
66

77
import Glean from "../../../../src/core/glean";
8-
import StringMetricType, { MAX_LENGTH_VALUE } from "../../../../src/core/metrics/types/string";
8+
import StringMetricType, { MAX_STRING_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/string";
99
import { Lifetime } from "../../../../src/core/metrics/lifetime";
1010

1111
import { Context } from "../../../../src/core/context";
@@ -95,7 +95,7 @@ describe("StringMetric", function() {
9595

9696
assert.strictEqual(
9797
metric.testGetValue("aPing"),
98-
testString.substring(0, MAX_LENGTH_VALUE)
98+
testString.substring(0, MAX_STRING_LENGTH_IN_BYTES)
9999
);
100100

101101
assert.strictEqual(

‎glean/tests/unit/core/metrics/string_list.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { ErrorType } from "../../../../src/core/error/error_type";
88

99
import Glean from "../../../../src/core/glean";
1010
import { Lifetime } from "../../../../src/core/metrics/lifetime";
11-
import StringListMetricType, { MAX_LIST_LENGTH, MAX_STRING_LENGTH } from "../../../../src/core/metrics/types/string_list";
11+
import StringListMetricType, { MAX_LIST_LENGTH, MAX_STRING_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/string_list";
1212
import { testResetGlean } from "../../../../src/core/testing";
1313

1414
describe("StringListMetric", function () {
@@ -121,7 +121,7 @@ describe("StringListMetric", function () {
121121
const testString = "01234567890".repeat(20);
122122
const testStringList = [testString];
123123
metric.set(testStringList);
124-
const expectedList = [testString.substring(0, MAX_STRING_LENGTH)];
124+
const expectedList = [testString.substring(0, MAX_STRING_LENGTH_IN_BYTES)];
125125
assert.deepStrictEqual(
126126
metric.testGetValue("aPing"),
127127
expectedList

‎glean/tests/unit/core/metrics/text.spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { ErrorType } from "../../../../src/core/error/error_type";
88

99
import Glean from "../../../../src/core/glean";
1010
import { Lifetime } from "../../../../src/core/metrics/lifetime";
11-
import TextMetricType, { TEXT_MAX_LENGTH } from "../../../../src/core/metrics/types/text";
11+
import TextMetricType, { MAX_TEXT_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/text";
1212
import { testResetGlean } from "../../../../src/core/testing";
1313

1414
describe("TextMetric", function() {
@@ -97,9 +97,9 @@ describe("TextMetric", function() {
9797
disabled: false
9898
});
9999

100-
const testText = `some value ${"a".repeat(TEXT_MAX_LENGTH)}`;
100+
const testText = `some value ${"a".repeat(MAX_TEXT_LENGTH_IN_BYTES)}`;
101101
metric.set(testText);
102-
const truncated = testText.substr(0, TEXT_MAX_LENGTH);
102+
const truncated = testText.substr(0, MAX_TEXT_LENGTH_IN_BYTES);
103103

104104
assert.strictEqual(metric.testGetValue("aPing"), truncated);
105105
assert.strictEqual(

0 commit comments

Comments
 (0)