From 571be90bbfeb999c0ceb202922698974b4d65271 Mon Sep 17 00:00:00 2001 From: Sander Bruens Date: Thu, 4 Apr 2024 12:19:23 -0400 Subject: [PATCH] feat(metrics_server): add support for tunnel time (#1527) * 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. * Make the test reports more realistic with 1 country. --- src/metrics_server/README.md | 1 + src/metrics_server/app_dev.yaml | 2 +- src/metrics_server/app_prod.yaml | 2 +- src/metrics_server/connection_metrics.spec.ts | 283 ++++++++---------- src/metrics_server/connection_metrics.ts | 10 + src/metrics_server/model.ts | 1 + src/metrics_server/test_integration.action.sh | 41 ++- 7 files changed, 170 insertions(+), 170 deletions(-) diff --git a/src/metrics_server/README.md b/src/metrics_server/README.md index c4a0298f..c5b2c33b 100644 --- a/src/metrics_server/README.md +++ b/src/metrics_server/README.md @@ -23,6 +23,7 @@ The metrics server supports two URL paths: userId: string, countries: string[], bytesTransferred: number, + tunnelTimeSec: number, }] } ``` diff --git a/src/metrics_server/app_dev.yaml b/src/metrics_server/app_dev.yaml index 01d663ba..a0ced492 100644 --- a/src/metrics_server/app_dev.yaml +++ b/src/metrics_server/app_dev.yaml @@ -1,4 +1,4 @@ -runtime: nodejs16 +runtime: nodejs18 service: dev handlers: - url: /.* diff --git a/src/metrics_server/app_prod.yaml b/src/metrics_server/app_prod.yaml index 360be28b..e6b2e569 100644 --- a/src/metrics_server/app_prod.yaml +++ b/src/metrics_server/app_prod.yaml @@ -1,4 +1,4 @@ -runtime: nodejs16 +runtime: nodejs18 service: prod handlers: - url: /.* diff --git a/src/metrics_server/connection_metrics.spec.ts b/src/metrics_server/connection_metrics.spec.ts index b5e559a5..f37de4a4 100644 --- a/src/metrics_server/connection_metrics.spec.ts +++ b/src/metrics_server/connection_metrics.spec.ts @@ -18,6 +18,21 @@ import { postConnectionMetrics, } from './connection_metrics'; import {InsertableTable} from './infrastructure/table'; +import {HourlyConnectionMetricsReport, HourlyUserConnectionMetricsReport} from './model'; + +const VALID_USER_REPORT = { + userId: 'uid0', + countries: ['US'], + bytesTransferred: 123, + tunnelTimeSec: 789, +}; + +const VALID_REPORT: HourlyConnectionMetricsReport = { + serverId: 'id', + startUtcMs: 1, + endUtcMs: 2, + userReports: [structuredClone(VALID_USER_REPORT)], +}; class FakeConnectionsTable implements InsertableTable { public rows: ConnectionRow[] | undefined; @@ -33,13 +48,15 @@ describe('postConnectionMetrics', () => { const userReports = [ { userId: 'uid0', - countries: ['US', 'UK'], + countries: ['UK'], bytesTransferred: 123, + tunnelTimeSec: 987, }, { userId: 'uid1', countries: ['EC'], bytesTransferred: 456, + tunnelTimeSec: 654, }, { userId: '', @@ -61,6 +78,7 @@ describe('postConnectionMetrics', () => { endTimestamp: new Date(report.endUtcMs).toISOString(), userId: userReports[0].userId, bytesTransferred: userReports[0].bytesTransferred, + tunnelTimeSec: userReports[0].tunnelTimeSec, countries: userReports[0].countries, }, { @@ -69,6 +87,7 @@ describe('postConnectionMetrics', () => { endTimestamp: new Date(report.endUtcMs).toISOString(), userId: userReports[1].userId, bytesTransferred: userReports[1].bytesTransferred, + tunnelTimeSec: userReports[1].tunnelTimeSec, countries: userReports[1].countries, }, { @@ -77,6 +96,7 @@ describe('postConnectionMetrics', () => { endTimestamp: new Date(report.endUtcMs).toISOString(), userId: undefined, bytesTransferred: userReports[2].bytesTransferred, + tunnelTimeSec: undefined, countries: userReports[2].countries, }, { @@ -85,6 +105,7 @@ describe('postConnectionMetrics', () => { endTimestamp: new Date(report.endUtcMs).toISOString(), userId: userReports[3].userId, bytesTransferred: userReports[3].bytesTransferred, + tunnelTimeSec: undefined, countries: userReports[3].countries, }, ]; @@ -95,183 +116,119 @@ describe('postConnectionMetrics', () => { describe('isValidConnectionMetricsReport', () => { it('returns true for valid report', () => { const userReports = [ - {userId: 'uid0', countries: ['AA'], bytesTransferred: 111}, - {userId: 'uid1', bytesTransferred: 222}, + {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}; - expect(isValidConnectionMetricsReport(report)).toBeTruthy(); + expect(isValidConnectionMetricsReport(report)).toBeTrue(); }); it('returns false for missing report', () => { - expect(isValidConnectionMetricsReport(undefined)).toBeFalsy(); + expect(isValidConnectionMetricsReport(undefined)).toBeFalse(); }); it('returns false for inconsistent timestamp values', () => { - const userReports = [ - {userId: 'uid0', countries: ['US', 'UK'], bytesTransferred: 123}, - {userId: 'uid1', countries: ['EC'], bytesTransferred: 456}, - ]; - const invalidReport = { - serverId: 'id', - startUtcMs: 999, // startUtcMs > endUtcMs - endUtcMs: 1, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport)).toBeFalsy(); + const report = structuredClone(VALID_REPORT); + // startUtcMs > endUtcMs + report.startUtcMs = 999; + report.endUtcMs = 1; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); it('returns false for out-of-bounds transferred bytes', () => { - const userReports = [ - { - userId: 'uid0', - countries: ['US', 'UK'], - bytesTransferred: -123, // Should not be negative - }, - {userId: 'uid1', countries: ['EC'], bytesTransferred: 456}, - ]; - const invalidReport = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports}; - expect(isValidConnectionMetricsReport(invalidReport)).toBeFalsy(); + const report = structuredClone(VALID_REPORT); + report.userReports[0].bytesTransferred = -123; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); - const userReports2 = [ - {userId: 'uid0', countries: ['US', 'UK'], bytesTransferred: 123}, - { - userId: 'uid1', - countries: ['EC'], - bytesTransferred: 2 * Math.pow(2, 40), // 2TB is above the server capacity - }, - ]; - const invalidReport2 = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports: userReports2}; - expect(isValidConnectionMetricsReport(invalidReport2)).toBeFalsy(); + // 2TB is above the server capacity + report.userReports[0].bytesTransferred = 2 * Math.pow(2, 40); + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for missing report fields', () => { - const invalidReport = { - // Missing `userReports` - serverId: 'id', - startUtcMs: 1, - endUtcMs: 2, - }; - expect(isValidConnectionMetricsReport(invalidReport)).toBeFalsy(); - - const invalidReport2 = { - serverId: 'id', - startUtcMs: 1, - endUtcMs: 2, - userReports: [], // Should not be empty - }; - expect(isValidConnectionMetricsReport(invalidReport2)).toBeFalsy(); - - const userReports = [ - {userId: 'uid0', countries: ['US', 'UK'], bytesTransferred: 123}, - {userId: 'uid1', countries: ['EC'], bytesTransferred: 456}, - ]; - const invalidReport3 = { - // Missing `serverId` - startUtcMs: 1, - endUtcMs: 2, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport3)).toBeFalsy(); - - const invalidReport4 = { - // Missing `startUtcMs` - serverId: 'id', - endUtcMs: 2, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport4)).toBeFalsy(); - - const invalidReport5 = { - // Missing `endUtcMs` - serverId: 'id', - startUtcMs: 2, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport5)).toBeFalsy(); + it('returns false for out-of-bounds tunnel time', () => { + const report = structuredClone(VALID_REPORT); + report.userReports[0].tunnelTimeSec = -123; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for missing user report fields', () => { - const invalidReport1 = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports: [ - { - // Missing `userId` and `countries` - bytesTransferred: 123, - }, - ]}; - expect(isValidConnectionMetricsReport(invalidReport1)).toBeFalsy(); - - const invalidReport2 = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports: { - // Missing `bytesTransferred` - userId: 'uid0', - countries: ['US', 'UK'], - }}; - expect(isValidConnectionMetricsReport(invalidReport2)).toBeFalsy(); + it('returns false for missing user reports', () => { + const report: Partial = structuredClone(VALID_REPORT); + delete report['userReports']; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for incorrect report field types', () => { - const invalidReport = { - serverId: 'id', - startUtcMs: 1, - endUtcMs: 2, - userReports: [1, 2, 3], // Should be `HourlyUserConnectionMetricsReport[]` - }; - expect(isValidConnectionMetricsReport(invalidReport)).toBeFalsy(); - - const userReports = [ - {userId: 'uid0', countries: ['US', 'UK'], bytesTransferred: 123}, - {userId: 'uid1', countries: ['EC'], bytesTransferred: 456}, - ]; - const invalidReport2 = { - serverId: 987, // Should be a string - startUtcMs: 1, - endUtcMs: 2, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport2)).toBeFalsy(); - - const invalidReport3 = { - serverId: 'id', - startUtcMs: '100', // Should be a number - endUtcMs: 200, - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport3)).toBeFalsy(); - - const invalidReport4 = { - // Missing `startUtcMs` - serverId: 'id', - startUtcMs: 1, - endUtcMs: '200', // Should be a number - userReports, - }; - expect(isValidConnectionMetricsReport(invalidReport4)).toBeFalsy(); + it('returns false for empty user reports', () => { + const report = structuredClone(VALID_REPORT); + report.userReports = []; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for incorrect user report field types ', () => { - const userReports = [ - { - userId: 1234, // Should be a string - countries: ['US', 'UK'], - bytesTransferred: 123, - }, - {userId: 'uid1', countries: ['EC'], bytesTransferred: 456}, - ]; - const invalidReport = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports}; - expect(isValidConnectionMetricsReport(invalidReport)).toBeFalsy(); - - const userReports2 = [ - { - userId: 'uid0', - countries: [1, 2, 3], // Should be string[] - bytesTransferred: 123, - }, - ]; - const invalidReport2 = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports: userReports2}; - expect(isValidConnectionMetricsReport(invalidReport2)).toBeFalsy(); - - const userReports3 = [ - { - userId: 'uid0', - countries: ['US', 'UK'], - bytesTransferred: '1234', // Should be a number - }, - ]; - const invalidReport3 = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports: userReports3}; - expect(isValidConnectionMetricsReport(invalidReport3)).toBeFalsy(); + it('returns false for missing `serverId`', () => { + const report: Partial = structuredClone(VALID_REPORT); + delete report['serverId']; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for missing `startUtcMs`', () => { + const report: Partial = structuredClone(VALID_REPORT); + delete report['startUtcMs']; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for missing `endUtcMs`', () => { + const report: Partial = structuredClone(VALID_REPORT); + delete report['endUtcMs']; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for user report missing both `userId` and `countries`', () => { + 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(); + }); + it('returns false for missing user report field `bytesTransferred`', () => { + const report = structuredClone(VALID_REPORT); + const userReport: Partial = + structuredClone(VALID_USER_REPORT); + delete userReport['bytesTransferred']; + report.userReports[0] = userReport as HourlyUserConnectionMetricsReport; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for user report field types that is not `HourlyUserConnectionMetricsReport`', () => { + const report = structuredClone(VALID_REPORT); + report.userReports = [1, 2, 3] as unknown as HourlyUserConnectionMetricsReport[]; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `serverId` field type that is not a string', () => { + const report = structuredClone(VALID_REPORT); + report.serverId = 987 as unknown as string; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `startUtcMs` field type that is not a number', () => { + const report = structuredClone(VALID_REPORT); + report.startUtcMs = '100' as unknown as number; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `endUtcMs` field type that is not a number', () => { + const report = structuredClone(VALID_REPORT); + 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[]; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `bytesTransferred` field type that is not a number', () => { + const report = structuredClone(VALID_REPORT); + report.userReports[0].bytesTransferred = '1234' as unknown as number; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `tunnelTimeSec` field type that is not a number', () => { + const report = structuredClone(VALID_REPORT); + report.userReports[0].tunnelTimeSec = '789' as unknown as number; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); }); diff --git a/src/metrics_server/connection_metrics.ts b/src/metrics_server/connection_metrics.ts index 01dc8135..0296d75f 100644 --- a/src/metrics_server/connection_metrics.ts +++ b/src/metrics_server/connection_metrics.ts @@ -22,6 +22,7 @@ export interface ConnectionRow { endTimestamp: string; // ISO formatted string. userId?: string; bytesTransferred: number; + tunnelTimeSec?: number; countries?: string[]; } @@ -51,6 +52,7 @@ function getConnectionRowsFromReport(report: HourlyConnectionMetricsReport): Con endTimestamp: endTimestampStr, userId: userReport.userId || undefined, bytesTransferred: userReport.bytesTransferred, + tunnelTimeSec: userReport.tunnelTimeSec || undefined, countries: userReport.countries || [], }); } @@ -96,6 +98,7 @@ export function isValidConnectionMetricsReport( 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; @@ -114,6 +117,13 @@ export function isValidConnectionMetricsReport( return false; } + if ( + userReport.tunnelTimeSec && + (typeof userReport.tunnelTimeSec !== 'number' || userReport.tunnelTimeSec < 0) + ) { + return false; + } + // Check that `countries` are strings. if (userReport.countries) { for (const country of userReport.countries) { diff --git a/src/metrics_server/model.ts b/src/metrics_server/model.ts index e355e824..afbb845c 100644 --- a/src/metrics_server/model.ts +++ b/src/metrics_server/model.ts @@ -25,6 +25,7 @@ export interface HourlyUserConnectionMetricsReport { userId: string; countries: string[]; bytesTransferred: number; + tunnelTimeSec?: number; } export interface DailyFeatureMetricsReport { diff --git a/src/metrics_server/test_integration.action.sh b/src/metrics_server/test_integration.action.sh index b2b6ead5..541c9203 100755 --- a/src/metrics_server/test_integration.action.sh +++ b/src/metrics_server/test_integration.action.sh @@ -41,8 +41,9 @@ readonly TIMESTAMP SERVER_ID SERVER_VERSION USER_ID1 USER_ID2 # BYTES_TRANSFERRED2 < BYTES_TRANSFERRED1 so we can order the records before comparing them. BYTES_TRANSFERRED1=$((2 + RANDOM % 100)) BYTES_TRANSFERRED2=$((BYTES_TRANSFERRED1 - 1)) +TUNNEL_TIME=$((RANDOM)) PER_KEY_LIMIT_COUNT=$((RANDOM)) -declare -ir BYTES_TRANSFERRED1 BYTES_TRANSFERRED2 PER_KEY_LIMIT_COUNT +declare -ir BYTES_TRANSFERRED1 BYTES_TRANSFERRED2 TUNNEL_TIME PER_KEY_LIMIT_COUNT echo "Using tmp directory ${TMPDIR}" @@ -55,6 +56,7 @@ cat << EOF > "${CONNECTIONS_REQUEST}" "userReports": [{ "userId": "${USER_ID1}", "bytesTransferred": ${BYTES_TRANSFERRED1}, + "tunnelTimeSec": ${TUNNEL_TIME}, "countries": ["US", "NL"] }, { "userId": "${USER_ID2}", @@ -78,22 +80,51 @@ EOF # Write the expected responses to temporary files. # Ignore the ISO formatted timestamps to ease the comparison. cat << EOF > "${CONNECTIONS_EXPECTED_RESPONSE}" -[{"bytesTransferred":"${BYTES_TRANSFERRED1}","countries":["US","NL"],"serverId":"${SERVER_ID}","userId":"${USER_ID1}"},{"bytesTransferred":"${BYTES_TRANSFERRED2}","countries":["UK"],"serverId":"${SERVER_ID}","userId":"${USER_ID2}"}] +[ + { + "bytesTransferred": "${BYTES_TRANSFERRED1}", + "countries": [ + "US", + "NL" + ], + "serverId": "${SERVER_ID}", + "tunnelTimeSec": "${TUNNEL_TIME}", + "userId": "${USER_ID1}" + }, + { + "bytesTransferred": "${BYTES_TRANSFERRED2}", + "countries": [ + "UK" + ], + "serverId": "${SERVER_ID}", + "tunnelTimeSec": null, + "userId": "${USER_ID2}" + } +] EOF cat << EOF > "${FEATURES_EXPECTED_RESPONSE}" -[{"dataLimit":{"enabled":"false","perKeyLimitCount":"${PER_KEY_LIMIT_COUNT}"},"serverId":"${SERVER_ID}","serverVersion":"${SERVER_VERSION}"}] +[ + { + "dataLimit": { + "enabled": "false", + "perKeyLimitCount": "${PER_KEY_LIMIT_COUNT}" + }, + "serverId": "${SERVER_ID}", + "serverVersion": "${SERVER_VERSION}" + } +] EOF 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, countries FROM \`${BIGQUERY_DATASET}.${CONNECTIONS_TABLE}\` WHERE serverId = \"${SERVER_ID}\" ORDER BY bytesTransferred DESC LIMIT 2" > "${CONNECTIONS_RESPONSE}" +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}" diff "${CONNECTIONS_RESPONSE}" "${CONNECTIONS_EXPECTED_RESPONSE}" echo "Features request:" cat "${FEATURES_REQUEST}" curl -X POST -H "Content-Type: application/json" -d "@${FEATURES_REQUEST}" "${METRICS_URL}/features" && echo sleep 5 -bq --project_id "${BIGQUERY_PROJECT}" --format json query --nouse_legacy_sql "SELECT serverId, serverVersion, dataLimit FROM \`${BIGQUERY_DATASET}.${FEATURES_TABLE}\` WHERE serverId = \"${SERVER_ID}\" ORDER BY timestamp DESC LIMIT 1" > "${FEATURES_RESPONSE}" +bq --project_id "${BIGQUERY_PROJECT}" --format json query --nouse_legacy_sql "SELECT serverId, serverVersion, dataLimit FROM \`${BIGQUERY_DATASET}.${FEATURES_TABLE}\` WHERE serverId = \"${SERVER_ID}\" ORDER BY timestamp DESC LIMIT 1" | jq > "${FEATURES_RESPONSE}" diff "${FEATURES_RESPONSE}" "${FEATURES_EXPECTED_RESPONSE}"