feat(metrics_server): drop the userId (#1528)

* feat(metrics_server): add support for tunnel time

* Add tests.

* Update `nodejs` runtime to 18.

* Update integration test.

* Use seconds instead of ms.

* Inline the minimun tunnel time and remove the unnecessary comment.

* Add field types to test case names.

* Make tunnel time optional.

* Add TODO to drop `userId`.

* Undo use of gdate.

* feat(metrics_server): drop the `userId`

* Update README.

* Fix bad merge.

* Fix bad merge on test.

* Revert change of country.

* Add types for the valid reports.

* Remove unrealistic test case with empty country list.

src/metrics_server/README.md

@@ -20,7 +20,6 @@ The metrics server supports two URL paths:
     startUtcMs: number,
     endUtcMs: number,
     userReports: [{
-        userId: string,
         countries: string[],
         bytesTransferred: number,
         tunnelTimeSec: number,

src/metrics_server/connection_metrics.spec.ts

@@ -20,18 +20,37 @@ import {
 import {InsertableTable} from './infrastructure/table';
 import {HourlyConnectionMetricsReport, HourlyUserConnectionMetricsReport} from './model';
 
-const VALID_USER_REPORT = {
-  userId: 'uid0',
+const VALID_USER_REPORT: HourlyUserConnectionMetricsReport = {
   countries: ['US'],
   bytesTransferred: 123,
   tunnelTimeSec: 789,
 };
 
+const VALID_USER_REPORT2: HourlyUserConnectionMetricsReport = {
+  countries: ['UK'],
+  bytesTransferred: 456,
+};
+
+/*
+ * A user report to test legacy fields to ensure backwards compatibility with
+ * older servers that may still send reports like this.
+ */
+const LEGACY_USER_REPORT = {
+  userId: 'foo',
+  countries: ['US', 'UK'],
+  bytesTransferred: 123,
+  tunnelTimeSec: 789,
+};
+
 const VALID_REPORT: HourlyConnectionMetricsReport = {
   serverId: 'id',
   startUtcMs: 1,
   endUtcMs: 2,
-  userReports: [structuredClone(VALID_USER_REPORT)],
+  userReports: [
+    structuredClone(VALID_USER_REPORT),
+    structuredClone(VALID_USER_REPORT2),
+    structuredClone(LEGACY_USER_REPORT),
+  ],
 };
 
 class FakeConnectionsTable implements InsertableTable<ConnectionRow> {
@@ -47,27 +66,19 @@ describe('postConnectionMetrics', () => {
     const table = new FakeConnectionsTable();
     const userReports = [
       {
-        userId: 'uid0',
         countries: ['UK'],
         bytesTransferred: 123,
         tunnelTimeSec: 987,
       },
       {
-        userId: 'uid1',
         countries: ['EC'],
         bytesTransferred: 456,
         tunnelTimeSec: 654,
       },
       {
-        userId: '',
         countries: ['BR'],
         bytesTransferred: 789,
       },
-      {
-        userId: 'uid1',
-        countries: [],
-        bytesTransferred: 555,
-      },
     ];
     const report = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports};
     await postConnectionMetrics(table, report);
@@ -76,7 +87,6 @@ describe('postConnectionMetrics', () => {
         serverId: report.serverId,
         startTimestamp: new Date(report.startUtcMs).toISOString(),
         endTimestamp: new Date(report.endUtcMs).toISOString(),
-        userId: userReports[0].userId,
         bytesTransferred: userReports[0].bytesTransferred,
         tunnelTimeSec: userReports[0].tunnelTimeSec,
         countries: userReports[0].countries,
@@ -85,7 +95,6 @@ describe('postConnectionMetrics', () => {
         serverId: report.serverId,
         startTimestamp: new Date(report.startUtcMs).toISOString(),
         endTimestamp: new Date(report.endUtcMs).toISOString(),
-        userId: userReports[1].userId,
         bytesTransferred: userReports[1].bytesTransferred,
         tunnelTimeSec: userReports[1].tunnelTimeSec,
         countries: userReports[1].countries,
@@ -94,20 +103,10 @@ describe('postConnectionMetrics', () => {
         serverId: report.serverId,
         startTimestamp: new Date(report.startUtcMs).toISOString(),
         endTimestamp: new Date(report.endUtcMs).toISOString(),
-        userId: undefined,
         bytesTransferred: userReports[2].bytesTransferred,
         tunnelTimeSec: undefined,
         countries: userReports[2].countries,
       },
-      {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        userId: userReports[3].userId,
-        bytesTransferred: userReports[3].bytesTransferred,
-        tunnelTimeSec: undefined,
-        countries: userReports[3].countries,
-      },
     ];
     expect(table.rows).toEqual(rows);
   });
@@ -115,14 +114,7 @@ describe('postConnectionMetrics', () => {
 
 describe('isValidConnectionMetricsReport', () => {
   it('returns true for valid report', () => {
-    const userReports = [
-      {userId: 'uid0', countries: ['AA'], bytesTransferred: 111, tunnelTimeSec: 1},
-      {userId: 'uid1', bytesTransferred: 222, tunnelTimeSec: 2},
-      {userId: 'uid2', countries: [], bytesTransferred: 333},
-      {countries: ['BB'], bytesTransferred: 444},
-      {userId: '', countries: ['CC'], bytesTransferred: 555},
-    ];
-    const report = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports};
+    const report = structuredClone(VALID_REPORT);
     expect(isValidConnectionMetricsReport(report)).toBeTrue();
   });
   it('returns false for missing report', () => {
@@ -174,12 +166,11 @@ describe('isValidConnectionMetricsReport', () => {
     delete report['endUtcMs'];
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for user report missing both `userId` and `countries`', () => {
+  it('returns false for missing user report field `countries`', () => {
+    const report = structuredClone(VALID_REPORT);
     const userReport: Partial<HourlyUserConnectionMetricsReport> =
       structuredClone(VALID_USER_REPORT);
-    delete userReport['userId'];
     delete userReport['countries'];
-    const report = structuredClone(VALID_REPORT);
     report.userReports[0] = userReport as HourlyUserConnectionMetricsReport;
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
@@ -211,11 +202,6 @@ describe('isValidConnectionMetricsReport', () => {
     report.endUtcMs = '100' as unknown as number;
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for `userId` field type that is not a string', () => {
-    const report = structuredClone(VALID_REPORT);
-    report.userReports[0].userId = 1234 as unknown as string;
-    expect(isValidConnectionMetricsReport(report)).toBeFalse();
-  });
   it('returns false for `countries` field type that is not a string', () => {
     const report = structuredClone(VALID_REPORT);
     report.userReports[0].countries = [1, 2, 3] as unknown as string[];

src/metrics_server/connection_metrics.ts

@@ -20,7 +20,6 @@ export interface ConnectionRow {
   serverId: string;
   startTimestamp: string; // ISO formatted string.
   endTimestamp: string; // ISO formatted string.
-  userId?: string;
   bytesTransferred: number;
   tunnelTimeSec?: number;
   countries?: string[];
@@ -50,7 +49,6 @@ function getConnectionRowsFromReport(report: HourlyConnectionMetricsReport): Con
       serverId: report.serverId,
       startTimestamp: startTimestampStr,
       endTimestamp: endTimestampStr,
-      userId: userReport.userId || undefined,
       bytesTransferred: userReport.bytesTransferred,
       tunnelTimeSec: userReport.tunnelTimeSec || undefined,
       countries: userReport.countries || [],
@@ -90,24 +88,14 @@ export function isValidConnectionMetricsReport(
     return false;
   }
 
-  // Check that userReports is an array of 1 or more item.
-  if (!(testObject.userReports.length >= 1)) {
+  // Check that at least 1 user report has been provided.
+  if (testObject.userReports.length === 0) {
     return false;
   }
 
   const MIN_BYTES_TRANSFERRED = 0;
   const MAX_BYTES_TRANSFERRED = 1 * Math.pow(2, 40); // 1 TB.
   for (const userReport of testObject.userReports) {
-    // TODO(sbruens): Drop unused `userId`.
-    // We require at least the userId or the country to be set.
-    if (!userReport.userId && (userReport.countries?.length ?? 0) === 0) {
-      return false;
-    }
-    // Check that `userId` is a string.
-    if (userReport.userId && typeof userReport.userId !== 'string') {
-      return false;
-    }
-
     // Check that `bytesTransferred` is a number between min and max transfer limits
     if (
       typeof userReport.bytesTransferred !== 'number' ||
@@ -124,12 +112,15 @@ export function isValidConnectionMetricsReport(
       return false;
     }
 
-    // Check that `countries` are strings.
-    if (userReport.countries) {
-      for (const country of userReport.countries) {
-        if (typeof country !== 'string') {
-          return false;
-        }
+    // We require at least 1 country to be set
+    const countries = userReport.countries ?? [];
+    if (countries.length === 0) {
+      return false;
+    }
+    // Check that all `countries` are strings.
+    for (const country of countries) {
+      if (typeof country !== 'string') {
+        return false;
       }
     }
   }

src/metrics_server/model.ts

@@ -22,7 +22,6 @@ export interface HourlyConnectionMetricsReport {
 }
 
 export interface HourlyUserConnectionMetricsReport {
-  userId: string;
   countries: string[];
   bytesTransferred: number;
   tunnelTimeSec?: number;

src/metrics_server/test_integration.action.sh

@@ -35,9 +35,7 @@ readonly FEATURES_EXPECTED_RESPONSE="${TMPDIR}/features_expected_res.json"
 TIMESTAMP="$(date +%s%3N)"
 SERVER_ID="$(uuidgen)"
 SERVER_VERSION="$(uuidgen)"
-USER_ID1="$(uuidgen)"
-USER_ID2="$(uuidgen)"
-readonly TIMESTAMP SERVER_ID SERVER_VERSION USER_ID1 USER_ID2
+readonly TIMESTAMP SERVER_ID SERVER_VERSION
 # BYTES_TRANSFERRED2 < BYTES_TRANSFERRED1 so we can order the records before comparing them.
 BYTES_TRANSFERRED1=$((2 + RANDOM % 100))
 BYTES_TRANSFERRED2=$((BYTES_TRANSFERRED1 - 1))
@@ -54,12 +52,10 @@ cat << EOF > "${CONNECTIONS_REQUEST}"
   "startUtcMs": ${TIMESTAMP},
   "endUtcMs": $((TIMESTAMP+1)),
   "userReports": [{
-    "userId": "${USER_ID1}",
     "bytesTransferred": ${BYTES_TRANSFERRED1},
     "tunnelTimeSec": ${TUNNEL_TIME},
     "countries": ["US", "NL"]
   }, {
-    "userId": "${USER_ID2}",
     "bytesTransferred": ${BYTES_TRANSFERRED2},
     "countries": ["UK"]
   }]
@@ -88,8 +84,7 @@ cat << EOF > "${CONNECTIONS_EXPECTED_RESPONSE}"
       "NL"
     ],
     "serverId": "${SERVER_ID}",
-    "tunnelTimeSec": "${TUNNEL_TIME}",
-    "userId": "${USER_ID1}"
+    "tunnelTimeSec": "${TUNNEL_TIME}"
   },
   {
     "bytesTransferred": "${BYTES_TRANSFERRED2}",
@@ -97,8 +92,7 @@ cat << EOF > "${CONNECTIONS_EXPECTED_RESPONSE}"
       "UK"
     ],
     "serverId": "${SERVER_ID}",
-    "tunnelTimeSec": null,
-    "userId": "${USER_ID2}"
+    "tunnelTimeSec": null
   }
 ]
 EOF
@@ -119,7 +113,7 @@ echo "Connections request:"
 cat "${CONNECTIONS_REQUEST}"
 curl -X POST -H "Content-Type: application/json" -d "@${CONNECTIONS_REQUEST}" "${METRICS_URL}/connections" && echo
 sleep 5
-bq --project_id "${BIGQUERY_PROJECT}" --format json query --nouse_legacy_sql "SELECT serverId, userId, bytesTransferred, tunnelTimeSec, countries FROM \`${BIGQUERY_DATASET}.${CONNECTIONS_TABLE}\` WHERE serverId = \"${SERVER_ID}\" ORDER BY bytesTransferred DESC LIMIT 2" | jq > "${CONNECTIONS_RESPONSE}"
+bq --project_id "${BIGQUERY_PROJECT}" --format json query --nouse_legacy_sql "SELECT serverId, bytesTransferred, tunnelTimeSec, countries FROM \`${BIGQUERY_DATASET}.${CONNECTIONS_TABLE}\` WHERE serverId = \"${SERVER_ID}\" ORDER BY bytesTransferred DESC LIMIT 2" | jq > "${CONNECTIONS_RESPONSE}"
 diff "${CONNECTIONS_RESPONSE}" "${CONNECTIONS_EXPECTED_RESPONSE}"
 
 echo "Features request:"