diff --git a/src/metrics_server/README.md b/src/metrics_server/README.md index c5b2c33b..e1c6ed77 100644 --- a/src/metrics_server/README.md +++ b/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, diff --git a/src/metrics_server/connection_metrics.spec.ts b/src/metrics_server/connection_metrics.spec.ts index f37de4a4..2e89cb57 100644 --- a/src/metrics_server/connection_metrics.spec.ts +++ b/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 { @@ -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 = 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[]; diff --git a/src/metrics_server/connection_metrics.ts b/src/metrics_server/connection_metrics.ts index 0296d75f..eee6cdaa 100644 --- a/src/metrics_server/connection_metrics.ts +++ b/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; } } } diff --git a/src/metrics_server/model.ts b/src/metrics_server/model.ts index afbb845c..8f127fe6 100644 --- a/src/metrics_server/model.ts +++ b/src/metrics_server/model.ts @@ -22,7 +22,6 @@ export interface HourlyConnectionMetricsReport { } export interface HourlyUserConnectionMetricsReport { - userId: string; countries: string[]; bytesTransferred: number; tunnelTimeSec?: number; diff --git a/src/metrics_server/test_integration.action.sh b/src/metrics_server/test_integration.action.sh index 541c9203..81b4073f 100755 --- a/src/metrics_server/test_integration.action.sh +++ b/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:"