mozilla · Dexterp37 · Dec 22, 2023 · Dec 12, 2023 · Dec 20, 2023 · Dec 20, 2023
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,28 +2,27 @@
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.2...main)
 
-[#1848](https://github.com/mozilla/glean.js/pull/1848): Support for automatically collecting element click events (first version)
+* [#1848](https://github.com/mozilla/glean.js/pull/1848): Support for automatically collecting element click events (first version)
+* [#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.
 
 # v4.0.0-pre.2 (2023-12-06)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.1...v4.0.0-pre.2)
 
-[#1835](https://github.com/mozilla/glean.js/pull/1835): Added support for automatic page load instrumentation.
-
+* [#1835](https://github.com/mozilla/glean.js/pull/1835): Added support for automatic page load instrumentation.
 * [#1846](https://github.com/mozilla/glean.js/pull/1846): Add logging messages when using the debugging APIs from the browser console.
 
 # v4.0.0-pre.1 (2023-12-01)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v4.0.0-pre.0...v4.0.0-pre.1)
 
-[#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.
+* [#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.
 
 # v4.0.0-pre.0 (2023-11-27)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v3.0.0...v4.0.0-pre.0)
 
-[#1808](https://github.com/mozilla/glean.js/pull/1808): **BREAKING CHANGE**: Make glean.js fully synchronous.
-
+* [#1808](https://github.com/mozilla/glean.js/pull/1808): **BREAKING CHANGE**: Make glean.js fully synchronous.
 * [#1835](https://github.com/mozilla/glean.js/pull/1835): Automatic instrumentation of page load events for simple web properties.
 
 # v3.0.0 (2023-11-16)

diff --git a/glean/src/core/metrics/types/event.ts b/glean/src/core/metrics/types/event.ts
@@ -11,14 +11,14 @@ import {
   getMonotonicNow,
   isString,
   testOnlyCheck,
-  truncateStringAtBoundaryWithError
+  truncateStringAtBytesBoundaryWithError
 } from "../../utils.js";
 import { Context } from "../../context.js";
 import { ErrorType } from "../../error/error_type.js";
 import { MetricValidationError } from "../metric.js";
 
 const LOG_TAG = "core.metrics.EventMetricType";
-const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
+const MAX_BYTES_EXTRA_KEY_VALUE = 500;
 
 /**
  * Base implementation of the event metric type,
@@ -65,10 +65,10 @@ export class InternalEventMetricType<
         for (const [name, value] of Object.entries(extra)) {
           if (this.allowedExtraKeys.includes(name)) {
             if (isString(value)) {
-              truncatedExtra[name] = truncateStringAtBoundaryWithError(
+              truncatedExtra[name] = truncateStringAtBytesBoundaryWithError(
                 this,
                 value,
-                MAX_LENGTH_EXTRA_KEY_VALUE
+                MAX_BYTES_EXTRA_KEY_VALUE
               );
             } else {
               truncatedExtra[name] = value;

diff --git a/glean/src/core/metrics/types/string.ts b/glean/src/core/metrics/types/string.ts
@@ -9,10 +9,10 @@ import { Context } from "../../context.js";
 import { MetricType } from "../index.js";
 import { Metric, MetricValidationError } from "../metric.js";
 import { validateString } from "../utils.js";
-import { testOnlyCheck, truncateStringAtBoundaryWithError, } from "../../utils.js";
+import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
 
 const LOG_TAG = "core.metrics.StringMetricType";
-export const MAX_LENGTH_VALUE = 100;
+export const MAX_STRING_LENGTH_IN_BYTES = 100;
 
 export class StringMetric extends Metric<string, string> {
   constructor(v: unknown) {
@@ -46,7 +46,12 @@ export class InternalStringMetricType extends MetricType {
     }
 
     try {
-      const truncatedValue = truncateStringAtBoundaryWithError(this, value, MAX_LENGTH_VALUE);
+      const truncatedValue = truncateStringAtBytesBoundaryWithError(
+        this,
+        value,
+        MAX_STRING_LENGTH_IN_BYTES
+      );
+
       const metric = new StringMetric(truncatedValue);
       Context.metricsDatabase.record(this, metric);
     } catch (e) {

diff --git a/glean/src/core/metrics/types/string_list.ts b/glean/src/core/metrics/types/string_list.ts
@@ -12,11 +12,11 @@ import { ErrorType } from "../../error/error_type.js";
 import { MetricType } from "../index.js";
 import { Metric, MetricValidation, MetricValidationError } from "../metric.js";
 import { validateString } from "../utils.js";
-import { testOnlyCheck, truncateStringAtBoundaryWithError } from "../../utils.js";
+import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
 
 const LOG_TAG = "core.metrics.StringListMetricType";
 export const MAX_LIST_LENGTH = 20;
-export const MAX_STRING_LENGTH = 50;
+export const MAX_STRING_LENGTH_IN_BYTES = 100;
 
 export class StringListMetric extends Metric<string[], string[]> {
   constructor(v: unknown) {
@@ -89,10 +89,10 @@ class InternalStringListMetricType extends MetricType {
 
       const truncatedList: string[] = [];
       for (let i = 0; i < Math.min(value.length, MAX_LIST_LENGTH); ++i) {
-        const truncatedString = truncateStringAtBoundaryWithError(
+        const truncatedString = truncateStringAtBytesBoundaryWithError(
           this,
           value[i],
-          MAX_STRING_LENGTH
+          MAX_STRING_LENGTH_IN_BYTES
         );
         truncatedList.push(truncatedString);
       }
@@ -112,7 +112,7 @@ class InternalStringListMetricType extends MetricType {
     }
 
     try {
-      const truncatedValue = truncateStringAtBoundaryWithError(this, value, MAX_STRING_LENGTH);
+      const truncatedValue = truncateStringAtBytesBoundaryWithError(this, value, MAX_STRING_LENGTH_IN_BYTES);
 
       Context.metricsDatabase.transform(
         this,

diff --git a/glean/src/core/metrics/types/text.ts b/glean/src/core/metrics/types/text.ts
@@ -10,11 +10,11 @@ import { Metric } from "../metric.js";
 import { MetricType } from "../index.js";
 import { MetricValidationError } from "../metric.js";
 import { validateString } from "../utils.js";
-import { testOnlyCheck, truncateStringAtBoundaryWithError, } from "../../utils.js";
+import { testOnlyCheck, truncateStringAtBytesBoundaryWithError } from "../../utils.js";
 
 const LOG_TAG = "core.metrics.TextMetricType";
-// The maximum number of characters for text.
-export const TEXT_MAX_LENGTH = 200 * 1024;
+
+export const MAX_TEXT_LENGTH_IN_BYTES = 200 * 1024;
 
 export class TextMetric extends Metric<string, string> {
   constructor(v: unknown) {
@@ -54,7 +54,12 @@ class InternalTextMetricType extends MetricType {
     }
 
     try {
-      const truncatedValue = truncateStringAtBoundaryWithError(this, text, TEXT_MAX_LENGTH);
+      const truncatedValue = truncateStringAtBytesBoundaryWithError(
+        this,
+        text,
+        MAX_TEXT_LENGTH_IN_BYTES
+      );
+
       const metric = new TextMetric(truncatedValue);
       Context.metricsDatabase.record(this, metric);
     } catch (e) {

diff --git a/glean/src/core/utils.ts b/glean/src/core/utils.ts
@@ -241,6 +241,39 @@ export function truncateStringAtBoundaryWithError(metric: MetricType, value: unk
   return truncated;
 }
 
+/**
+ * Truncates a string to a given max number of bytes.
+ *
+ * If the string required truncation, records an error through the error
+ * reporting mechanism.
+ *
+ * @param metric The metric to record an error to, if necessary,
+ * @param value The string to truncate.
+ * @param maxBytes The max number of bytes to truncate to.
+ * @returns A string with at most `maxLength` bytes.
+ * @throws In case `value` is not a string.
+ */
+export function truncateStringAtBytesBoundaryWithError(metric: MetricType, value: unknown, maxBytes: number): string {
+  if (!isString(value)) {
+    throw new MetricValidationError(`Expected string, got ${JSON.stringify(value)}`);
+  }
+
+  const encoder = new TextEncoder();
+  const decoder = new TextDecoder("utf-8");
+
+  const uint8 = encoder.encode(value);
+  const section = uint8.slice(0, maxBytes);
+  const truncated = decoder.decode(section);
+  if (truncated !== value) {
+    Context.errorManager.record(
+      metric,
+      ErrorType.InvalidOverflow,
+      `Value length ${new Blob([value]).size} exceeds maximum of ${value.length} bytes.`
+    );
+  }
+  return truncated;
+}
+
 /**
  * Decorator factory that will only allow a function to be called when Glean is in testing mode.
  *

diff --git a/glean/tests/unit/core/metrics/event.spec.ts b/glean/tests/unit/core/metrics/event.spec.ts
@@ -126,7 +126,7 @@ describe("EventMetric", function() {
       disabled: false
     }, ["label"]);
 
-    metric.record({ label: "01234567890".repeat(20) });
+    metric.record({ label: "01234567890".repeat(100) });
     assert.strictEqual(metric.testGetNumRecordedErrors(ErrorType.InvalidOverflow), 1);
   });
 
@@ -210,7 +210,7 @@ describe("EventMetric", function() {
     const testValue = "LeanGleanByFrank";
     const extra = {
       "extra1": testValue,
-      "truncatedExtra": testValue.repeat(10),
+      "truncatedExtra": testValue.repeat(50),
     };
 
     testEvent.record(extra);
@@ -225,7 +225,7 @@ describe("EventMetric", function() {
     assert.strictEqual(2, Object.keys(event.extra).length);
     assert.strictEqual(testValue, event.extra["extra1"]);
     assert.strictEqual(
-      testValue.repeat(10).substr(0, 100),
+      testValue.repeat(50).substr(0, 500),
       event.extra["truncatedExtra"]
     );
   });

diff --git a/glean/tests/unit/core/metrics/string.spec.ts b/glean/tests/unit/core/metrics/string.spec.ts
@@ -5,7 +5,7 @@
 import assert from "assert";
 
 import Glean from "../../../../src/core/glean";
-import StringMetricType, { MAX_LENGTH_VALUE } from "../../../../src/core/metrics/types/string";
+import StringMetricType, { MAX_STRING_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/string";
 import { Lifetime } from "../../../../src/core/metrics/lifetime";
 
 import { Context } from "../../../../src/core/context";
@@ -95,7 +95,7 @@ describe("StringMetric", function() {
 
     assert.strictEqual(
       metric.testGetValue("aPing"),
-      testString.substring(0, MAX_LENGTH_VALUE)
+      testString.substring(0, MAX_STRING_LENGTH_IN_BYTES)
     );
 
     assert.strictEqual(

diff --git a/glean/tests/unit/core/metrics/string_list.spec.ts b/glean/tests/unit/core/metrics/string_list.spec.ts
@@ -8,7 +8,7 @@ import { ErrorType } from "../../../../src/core/error/error_type";
 
 import Glean from "../../../../src/core/glean";
 import { Lifetime } from "../../../../src/core/metrics/lifetime";
-import StringListMetricType, { MAX_LIST_LENGTH, MAX_STRING_LENGTH } from "../../../../src/core/metrics/types/string_list";
+import StringListMetricType, { MAX_LIST_LENGTH, MAX_STRING_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/string_list";
 import { testResetGlean } from "../../../../src/core/testing";
 
 describe("StringListMetric", function () {
@@ -121,7 +121,7 @@ describe("StringListMetric", function () {
     const testString = "01234567890".repeat(20);
     const testStringList = [testString];
     metric.set(testStringList);
-    const expectedList = [testString.substring(0, MAX_STRING_LENGTH)];
+    const expectedList = [testString.substring(0, MAX_STRING_LENGTH_IN_BYTES)];
     assert.deepStrictEqual(
       metric.testGetValue("aPing"),
       expectedList

diff --git a/glean/tests/unit/core/metrics/text.spec.ts b/glean/tests/unit/core/metrics/text.spec.ts
@@ -8,7 +8,7 @@ import { ErrorType } from "../../../../src/core/error/error_type";
 
 import Glean from "../../../../src/core/glean";
 import { Lifetime } from "../../../../src/core/metrics/lifetime";
-import TextMetricType, { TEXT_MAX_LENGTH } from "../../../../src/core/metrics/types/text";
+import TextMetricType, { MAX_TEXT_LENGTH_IN_BYTES } from "../../../../src/core/metrics/types/text";
 import { testResetGlean } from "../../../../src/core/testing";
 
 describe("TextMetric", function() {
@@ -97,9 +97,9 @@ describe("TextMetric", function() {
       disabled: false
     });
 
-    const testText = `some value ${"a".repeat(TEXT_MAX_LENGTH)}`;
+    const testText = `some value ${"a".repeat(MAX_TEXT_LENGTH_IN_BYTES)}`;
     metric.set(testText);
-    const truncated = testText.substr(0, TEXT_MAX_LENGTH);
+    const truncated = testText.substr(0, MAX_TEXT_LENGTH_IN_BYTES);
 
     assert.strictEqual(metric.testGetValue("aPing"), truncated);
     assert.strictEqual(