feat(metrics_server): do not invalidate legacy report types

src/metrics_server/connection_metrics.spec.ts

@@ -18,7 +18,11 @@ import {
   postConnectionMetrics,
 } from './connection_metrics';
 import {InsertableTable} from './infrastructure/table';
-import {HourlyConnectionMetricsReport, HourlyUserConnectionMetricsReport} from './model';
+import {
+  HourlyConnectionMetricsReport,
+  HourlyUserConnectionMetricsReport,
+  LegacyHourlyUserConnectionMetricsReport,
+} from './model';
 
 const VALID_USER_REPORT: HourlyUserConnectionMetricsReport = {
   countries: ['US'],
@@ -32,14 +36,21 @@ const VALID_USER_REPORT2: HourlyUserConnectionMetricsReport = {
 };
 
 /*
- * A user report to test legacy fields to ensure backwards compatibility with
- * older servers that may still send reports like this.
+ * Legacy access key (`userId`) user reports to ensure backwards compatibility with servers
+ * not synced past https://github.com/Jigsaw-Code/outline-server/pull/1529).
  */
-const LEGACY_USER_REPORT = {
+const LEGACY_USER_ID_USER_REPORT: LegacyHourlyUserConnectionMetricsReport = {
   userId: 'foo',
+  bytesTransferred: 123,
+};
+
+/*
+ * Legacy multiple countries user reports to ensure backwards compatibility with servers
+ * not synced past https://github.com/Jigsaw-Code/outline-server/pull/1242.
+ */
+const LEGACY_COUNTRIES_USER_REPORT: HourlyUserConnectionMetricsReport = {
   countries: ['US', 'UK'],
   bytesTransferred: 123,
-  tunnelTimeSec: 789,
 };
 
 const VALID_REPORT: HourlyConnectionMetricsReport = {
@@ -49,10 +60,17 @@ const VALID_REPORT: HourlyConnectionMetricsReport = {
   userReports: [
     structuredClone(VALID_USER_REPORT),
     structuredClone(VALID_USER_REPORT2),
-    structuredClone(LEGACY_USER_REPORT),
+    structuredClone(LEGACY_COUNTRIES_USER_REPORT),
   ],
 };
 
+const LEGACY_REPORT: HourlyConnectionMetricsReport = {
+  serverId: 'legacy-id',
+  startUtcMs: 3,
+  endUtcMs: 4,
+  userReports: [structuredClone(LEGACY_USER_ID_USER_REPORT)],
+};
+
 class FakeConnectionsTable implements InsertableTable<ConnectionRow> {
   public rows: ConnectionRow[] | undefined;
 
@@ -62,61 +80,57 @@ class FakeConnectionsTable implements InsertableTable<ConnectionRow> {
 }
 
 describe('postConnectionMetrics', () => {
-  it('correctly inserts feature metrics rows', async () => {
+  it('correctly inserts connection metrics rows', async () => {
     const table = new FakeConnectionsTable();
-    const userReports = [
-      {
-        countries: ['UK'],
-        bytesTransferred: 123,
-        tunnelTimeSec: 987,
-      },
-      {
-        countries: ['EC'],
-        bytesTransferred: 456,
-        tunnelTimeSec: 654,
-      },
-      {
-        countries: ['BR'],
-        bytesTransferred: 789,
-      },
-    ];
-    const report = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports};
-    await postConnectionMetrics(table, report);
+
+    await postConnectionMetrics(table, VALID_REPORT);
+
     const rows: ConnectionRow[] = [
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[0].bytesTransferred,
-        tunnelTimeSec: userReports[0].tunnelTimeSec,
-        countries: userReports[0].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: VALID_USER_REPORT.bytesTransferred,
+        tunnelTimeSec: VALID_USER_REPORT.tunnelTimeSec,
+        countries: VALID_USER_REPORT.countries,
       },
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[1].bytesTransferred,
-        tunnelTimeSec: userReports[1].tunnelTimeSec,
-        countries: userReports[1].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: VALID_USER_REPORT2.bytesTransferred,
+        tunnelTimeSec: VALID_USER_REPORT2.tunnelTimeSec,
+        countries: VALID_USER_REPORT2.countries,
       },
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[2].bytesTransferred,
-        tunnelTimeSec: undefined,
-        countries: userReports[2].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: LEGACY_COUNTRIES_USER_REPORT.bytesTransferred,
+        tunnelTimeSec: LEGACY_COUNTRIES_USER_REPORT.tunnelTimeSec,
+        countries: LEGACY_COUNTRIES_USER_REPORT.countries,
       },
     ];
     expect(table.rows).toEqual(rows);
   });
+  it('correctly drops legacy connection metrics', async () => {
+    const table = new FakeConnectionsTable();
+
+    await postConnectionMetrics(table, LEGACY_REPORT);
+
+    expect(table.rows).toEqual([]);
+  });
 });
 
 describe('isValidConnectionMetricsReport', () => {
   it('returns true for valid report', () => {
     const report = structuredClone(VALID_REPORT);
     expect(isValidConnectionMetricsReport(report)).toBeTrue();
   });
+  it('returns true for legacy report', () => {
+    const report = structuredClone(LEGACY_REPORT);
+    expect(isValidConnectionMetricsReport(report)).toBeTrue();
+  });
   it('returns false for missing report', () => {
     expect(isValidConnectionMetricsReport(undefined)).toBeFalse();
   });
@@ -166,14 +180,6 @@ describe('isValidConnectionMetricsReport', () => {
     delete report['endUtcMs'];
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for missing user report field `countries`', () => {
-    const report = structuredClone(VALID_REPORT);
-    const userReport: Partial<HourlyUserConnectionMetricsReport> =
-      structuredClone(VALID_USER_REPORT);
-    delete userReport['countries'];
-    report.userReports[0] = userReport as HourlyUserConnectionMetricsReport;
-    expect(isValidConnectionMetricsReport(report)).toBeFalse();
-  });
   it('returns false for missing user report field `bytesTransferred`', () => {
     const report = structuredClone(VALID_REPORT);
     const userReport: Partial<HourlyUserConnectionMetricsReport> =
@@ -202,7 +208,12 @@ describe('isValidConnectionMetricsReport', () => {
     report.endUtcMs = '100' as unknown as number;
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for `countries` field type that is not a string', () => {
+  it('returns false for `countries` field type that is not an array', () => {
+    const report = structuredClone(VALID_REPORT);
+    report.userReports[0].countries = 'US' as unknown as string[];
+    expect(isValidConnectionMetricsReport(report)).toBeFalse();
+  });
+  it('returns false for `countries` arry items that are not strings', () => {
     const report = structuredClone(VALID_REPORT);
     report.userReports[0].countries = [1, 2, 3] as unknown as string[];
     expect(isValidConnectionMetricsReport(report)).toBeFalse();

src/metrics_server/connection_metrics.ts

@@ -14,8 +14,13 @@
 
 import {Table} from '@google-cloud/bigquery';
 import {InsertableTable} from './infrastructure/table';
-import {HourlyConnectionMetricsReport} from './model';
+import {
+  HourlyConnectionMetricsReport,
+  HourlyUserConnectionMetricsReport,
+  LegacyHourlyUserConnectionMetricsReport,
+} from './model';
 
+const TERABYTE = Math.pow(2, 40);
 export interface ConnectionRow {
   serverId: string;
   startTimestamp: string; // ISO formatted string.
@@ -45,18 +50,28 @@ function getConnectionRowsFromReport(report: HourlyConnectionMetricsReport): Con
   const endTimestampStr = new Date(report.endUtcMs).toISOString();
   const rows = [];
   for (const userReport of report.userReports) {
+    if (!isCurrentUserReport(userReport)) {
+      continue;
+    }
     rows.push({
       serverId: report.serverId,
       startTimestamp: startTimestampStr,
       endTimestamp: endTimestampStr,
       bytesTransferred: userReport.bytesTransferred,
       tunnelTimeSec: userReport.tunnelTimeSec || undefined,
-      countries: userReport.countries || [],
+      countries: userReport.countries,
     });
   }
   return rows;
 }
 
+function isCurrentUserReport(
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  userReport: LegacyHourlyUserConnectionMetricsReport
+): userReport is HourlyUserConnectionMetricsReport {
+  return userReport.userId === undefined;
+}
+
 // Returns true iff testObject contains a valid HourlyConnectionMetricsReport.
 export function isValidConnectionMetricsReport(
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -93,38 +108,70 @@ export function isValidConnectionMetricsReport(
     return false;
   }
 
-  const MIN_BYTES_TRANSFERRED = 0;
-  const MAX_BYTES_TRANSFERRED = 1 * Math.pow(2, 40); // 1 TB.
-  for (const userReport of testObject.userReports) {
-    // Check that `bytesTransferred` is a number between min and max transfer limits
-    if (
-      typeof userReport.bytesTransferred !== 'number' ||
-      userReport.bytesTransferred < MIN_BYTES_TRANSFERRED ||
-      userReport.bytesTransferred > MAX_BYTES_TRANSFERRED
-    ) {
-      return false;
+  let i = testObject.userReports.length;
+  while (i--) {
+    const userReport = testObject.userReports[i];
+
+    // For backwards compatibility, we do not want to invalidate legacy report
+    // formats. Instead, we drop them silently.
+    if (!isCurrentUserReport(userReport)) {
+      testObject.userReports.splice(i, 1);
+      continue;
     }
 
-    if (
-      userReport.tunnelTimeSec &&
-      (typeof userReport.tunnelTimeSec !== 'number' || userReport.tunnelTimeSec < 0)
-    ) {
+    if (!isValidUserConnectionMetricsReport(userReport)) {
       return false;
     }
+  }
+
+  // Request is a valid HourlyConnectionMetricsReport.
+  return true;
+}
 
-    // We require at least 1 country to be set
-    const countries = userReport.countries ?? [];
-    if (countries.length === 0) {
+// Returns true iff testObject contains a valid HourlyUserConnectionMetricsReport.
+function isValidUserConnectionMetricsReport(
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  testObject: any
+): testObject is HourlyUserConnectionMetricsReport {
+  if (!testObject) {
+    return false;
+  }
+
+  // Check that all required fields are present.
+  const requiredConnectionMetricsFields = ['countries', 'bytesTransferred'];
+  for (const fieldName of requiredConnectionMetricsFields) {
+    if (!testObject[fieldName]) {
       return false;
     }
-    // Check that all `countries` are strings.
-    for (const country of countries) {
-      if (typeof country !== 'string') {
-        return false;
-      }
+  }
+
+  // Check that `bytesTransferred` is a number between min and max transfer limits
+  if (
+    typeof testObject.bytesTransferred !== 'number' ||
+    testObject.bytesTransferred < 0 ||
+    testObject.bytesTransferred > TERABYTE
+  ) {
+    return false;
+  }
+
+  if (
+    testObject.tunnelTimeSec &&
+    (typeof testObject.tunnelTimeSec !== 'number' || testObject.tunnelTimeSec < 0)
+  ) {
+    return false;
+  }
+
+  // We require at least 1 country to be set
+  if (!Array.isArray(testObject.countries) || testObject.countries.length === 0) {
+    return false;
+  }
+  // Check that all `countries` are strings.
+  for (const country of testObject.countries) {
+    if (typeof country !== 'string') {
+      return false;
     }
   }
 
-  // Request is a valid HourlyConnectionMetricsReport.
+  // Request is a valid HourlyUserConnectionMetricsReport.
   return true;
 }

src/metrics_server/model.ts

@@ -18,7 +18,7 @@ export interface HourlyConnectionMetricsReport {
   serverId: string;
   startUtcMs: number;
   endUtcMs: number;
-  userReports: HourlyUserConnectionMetricsReport[];
+  userReports: (HourlyUserConnectionMetricsReport | LegacyHourlyUserConnectionMetricsReport)[];
 }
 
 export interface HourlyUserConnectionMetricsReport {
@@ -27,6 +27,15 @@ export interface HourlyUserConnectionMetricsReport {
   tunnelTimeSec?: number;
 }
 
+/*
+ * Legacy reports used to test backwards compatibile reports that older servers
+ * may still send.
+ */
+export interface LegacyHourlyUserConnectionMetricsReport
+  extends Omit<HourlyUserConnectionMetricsReport, 'countries'> {
+  userId?: string;
+}
+
 export interface DailyFeatureMetricsReport {
   serverId: string;
   serverVersion: string;