From 99f7421fce1e6232e0ee7ee5bc57a17149f97cd9 Mon Sep 17 00:00:00 2001 From: Hugh Grigg Date: Sat, 29 Aug 2026 18:41:35 +0100 Subject: [PATCH] refactor: keep SQL out of the beacon's browser bundle `src/beacon-events.ts` held both halves of a beacon event. The browser half builds the query string an event travels in, and the SQL half reads the same parameters back off a delivered log record. Browser code imports the package root, so a site folding the beacon into its own bundle took the SQL with it. The SQL constants sat at module scope, so a bundler kept their initialisers along with `decodedParameter` and `quoted` to compute them. An entry importing `beaconQueryString` and sending one event, bundled and minified with rolldown, came to 464 bytes and 331 gzipped. Split, the same entry is 241 bytes and 215 gzipped, and it carries no `url_decode`, no `url_extract_parameter` and no `strpos`. `beacon-events.ts` keeps the envelope and now imports nothing at all. `beacon-rows.ts` holds the three column expressions and the two row conditions, and imports the parameter names from next door. Every export keeps its name and the package root exports the same set, so nothing outside this repository changes. `pack-check.sh` gains the guard that keeps it that way. The forbidden-import check it already runs cannot see this, because the package root legitimately reaches SQL and every one of those imports is a relative path it allows. The new check reads the built envelope module and fails on any import at all. --- scripts/sh/pack-check.sh | 41 ++++++++++++++++++++++++ src/beacon-events.test.ts | 32 ------------------- src/beacon-events.ts | 66 ++++---------------------------------- src/beacon-rollup.ts | 4 +-- src/beacon-rows.test.ts | 39 +++++++++++++++++++++++ src/beacon-rows.ts | 67 +++++++++++++++++++++++++++++++++++++++ src/cdk/log-table.test.ts | 5 ++- src/index.ts | 12 ++++--- src/rollup-questions.ts | 3 +- src/rollups.test.ts | 3 +- 10 files changed, 169 insertions(+), 103 deletions(-) create mode 100644 src/beacon-rows.test.ts create mode 100644 src/beacon-rows.ts diff --git a/scripts/sh/pack-check.sh b/scripts/sh/pack-check.sh index da4a0c2..00ebb5a 100755 --- a/scripts/sh/pack-check.sh +++ b/scripts/sh/pack-check.sh @@ -150,6 +150,47 @@ if [[ -n "$forbidden" ]]; then exit 1 fi +# The beacon envelope, which has to stand on its own. +# +# `beaconQueryString` is the one thing in this package that runs on every page +# of a measured site. A bundler reaches it through the package root, and the +# root re-exports everything, so what actually decides the page weight is +# which modules the envelope itself pulls in. Its module answers that with +# nothing, and this is what keeps the answer that way. +# +# KensioSoftware/rainlytics#110 is what this is guarding. The SQL reading the +# same parameters back used to sit in the same file, and a minified bundle of +# a beacon-shaped entry carried `url_decode`, `url_extract_parameter` and +# `strpos` that no browser runs. Splitting the file took that bundle from 464 +# bytes to 241. +# +# The forbidden-import check above cannot see this. The root legitimately +# reaches SQL, and every one of those imports is a relative path it allows. + +envelope="$tmp/package/dist/beacon-events.js" + +if [[ ! -f "$envelope" ]]; then + echo "The beacon envelope is not at dist/beacon-events.js." >&2 + echo "Point this check at wherever beaconQueryString moved to." >&2 + exit 1 +fi + +envelope_imports="$( + grep --only-matching --extended-regexp \ + "(from|import)[[:space:]]*\(?[[:space:]]*[\"'][^\"']+[\"']" \ + "$envelope" || true +)" + +if [[ -n "$envelope_imports" ]]; then + echo "The beacon envelope imports something:" >&2 + sed 's/^/ /' <<<"$envelope_imports" >&2 + echo >&2 + echo "Every page of a measured site downloads this module. Whatever it" >&2 + echo "imports is downloaded with it, however little of that a browser" >&2 + echo "runs. SQL belongs in dist/beacon-rows.js, which no browser reaches." >&2 + exit 1 +fi + # The CLI, run out of the tarball. # # Running it proves more than that the file was packed. The extracted tarball diff --git a/src/beacon-events.test.ts b/src/beacon-events.test.ts index 33631ef..84dfa04 100644 --- a/src/beacon-events.test.ts +++ b/src/beacon-events.test.ts @@ -13,13 +13,10 @@ import { deployStacks } from "#test/simulated-deployment.js"; import { runAthenaQuery } from "./athena/athena-query.js"; import { - aBeaconEvent, - beaconEventColumn, beaconParameters, beaconQueryString, beaconSchemaVersion, defaultBeaconPath, - outsideTheBeaconPath, } from "./beacon-events.js"; import { CloudFrontLogDelivery } from "./cdk/log-delivery.js"; import { LogBucket } from "./cdk/log-bucket.js"; @@ -75,35 +72,6 @@ describe("the beacon event envelope", () => { expect(sent.split("&")).toHaveLength(3); }); - it("reads a row's event back off the column CloudFront wrote it to", () => { - // Given the SQL a rollup selects the event with. - // Then it reads the query string, which is where the payload is. No - // column of the table holds it, because a table column is a CloudFront - // field and CloudFront has no field for somebody else's payload. - expect(beaconEventColumn).toContain("cs_uri_query"); - expect(beaconEventColumn).toContain(`'${beaconParameters.event}'`); - }); - - it("counts only requests carrying an envelope", () => { - // Given the conditions a rollup filters beacon rows with. - // Then a request to the beacon's path with no version parameter is left - // out. A crawler that found the URL in a page's source sends one of - // those, and counting it would report an event nobody caused. - expect(aBeaconEvent).toContain("cs_uri_query <> '-'"); - expect(aBeaconEvent.join(" ")).toContain(`'${beaconParameters.version}'`); - }); - - it("names the path where the envelope's own conditions leave it out", () => { - // Given the condition status-codes takes the beacon's requests back out - // with. - // Then it names the path, where `aBeaconEvent` leaves the path to the - // request's own `paths`. The two run in opposite directions. It reads - // the column as delivered, since the path carries nothing a browser or - // CloudFront escapes. - expect(outsideTheBeaconPath).toContain(`'${defaultBeaconPath}'`); - expect(outsideTheBeaconPath).not.toContain("url_decode"); - }); - it("sends to a path a site is unlikely to serve already", () => { // Given the default path. // Then it is one path, absolute, and marked as not a page. Pointing the diff --git a/src/beacon-events.ts b/src/beacon-events.ts index ff4c497..af574da 100644 --- a/src/beacon-events.ts +++ b/src/beacon-events.ts @@ -1,5 +1,4 @@ -// What a beacon event is, and how it survives the round trip through a -// CloudFront access log. +// What a beacon event is, and the query string it travels in. // // The beacon sends a GET to a path on the site's own domain and puts its // payload in the query string. CloudFront records `cs-uri-query` whatever the @@ -31,12 +30,11 @@ // is for the beacon's own issue, and `version` is what lets that arrive // without reinterpreting rows already written. // -// Both halves of the package read this. The beacon builds a query string from -// it in a browser, so nothing here may reach a Node built-in or `aws-cdk-lib`, -// and a rollup reads the same parameters back as SQL. - -import { decodedParameter } from "./log-encoding.js"; -import { quoted } from "./sql-text.js"; +// This module is the browser's half of that definition, and it imports +// nothing. Every page of a measured site downloads it, so the SQL reading +// these same parameters back off a row lives in `beacon-rows.ts` next door. +// KensioSoftware/rainlytics#110 has what keeping the two together cost a +// bundle. /** * The path a beacon reports to, where a site chooses none. @@ -106,7 +104,7 @@ export interface BeaconEvent { * * The browser's own encoding, which is the single pass a request carries. * CloudFront adds its own on the way into the record, and - * {@link beaconEventColumn} reads both back off. + * `beaconEventColumn` in `beacon-rows.ts` reads both back off. * * No leading `?`. The caller joins it to the path it is sending to. * @@ -125,53 +123,3 @@ export function beaconQueryString(event: BeaconEvent): string { ) .join("&"); } - -/** The envelope version a row was written under, as SQL. */ -export const beaconVersionColumn = decodedParameter(beaconParameters.version); - -/** What happened, as SQL. */ -export const beaconEventColumn = decodedParameter(beaconParameters.event); - -/** The page it happened on, as SQL. */ -export const beaconPageColumn = decodedParameter(beaconParameters.page); - -/** - * The rows a beacon event is, as conditions for `rowsFor`. - * - * The path is not among them. A rollup narrows to the beacon's path through - * the request's own `paths`, the way any other question narrows to a section - * of a site, and a site that moved its beacon then says so in one place. - * - * These leave out anything else reaching the same path. A crawler following - * a beacon URL out of a page's source carries no version parameter, and the - * bot filter `rowsFor` applies has already taken most of them. - * - * ```typescript - * rowsFor({ ...request, paths: [defaultBeaconPath] }, aBeaconEvent); - * ``` - */ -export const aBeaconEvent: readonly string[] = [ - "cs_method = 'GET'", - "cs_uri_query <> '-'", - `${beaconVersionColumn} <> ''`, -]; - -/** - * The rows outside the beacon's path, as a condition for `rowsFor`. - * - * The other direction from {@link aBeaconEvent}, and it names the path that - * one leaves out. A question about beacon events narrows to the beacon - * through the request's own `paths`. A question about what the site answered - * has to take the beacon's requests back out, and `status-codes` is the one - * that does. - * - * Matched against the column as CloudFront delivered it, where `--path` - * decodes twice first. The path here is a constant this package chose and it - * carries nothing a browser or CloudFront escapes, so a record holds it as it - * was sent. An address somebody typed can hold anything. - * - * A prefix, the way every path match in Rainlytics is one. - */ -export const outsideTheBeaconPath = `strpos(cs_uri_stem, ${quoted( - defaultBeaconPath, -)}) <> 1`; diff --git a/src/beacon-rollup.ts b/src/beacon-rollup.ts index 9b2aed8..9fa4acb 100644 --- a/src/beacon-rollup.ts +++ b/src/beacon-rollup.ts @@ -30,12 +30,12 @@ // exported for a site running the beacon to add to its own summaries, and a // deployment with no beacon computes nothing for it. +import { defaultBeaconPath } from "./beacon-events.js"; import { aBeaconEvent, beaconEventColumn, beaconPageColumn, - defaultBeaconPath, -} from "./beacon-events.js"; +} from "./beacon-rows.js"; import { qualifiedTableName } from "./dataset.js"; import type { Rollup, RollupTotals } from "./rollups.js"; import { rowsFor } from "./rollups.js"; diff --git a/src/beacon-rows.test.ts b/src/beacon-rows.test.ts new file mode 100644 index 0000000..1f1b554 --- /dev/null +++ b/src/beacon-rows.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; + +import { beaconParameters, defaultBeaconPath } from "./beacon-events.js"; +import { + aBeaconEvent, + beaconEventColumn, + outsideTheBeaconPath, +} from "./beacon-rows.js"; + +describe("reading a beacon event off a row", () => { + it("reads a row's event back off the column CloudFront wrote it to", () => { + // Given the SQL a rollup selects the event with. + // Then it reads the query string, which is where the payload is. No + // column of the table holds it, because a table column is a CloudFront + // field and CloudFront has no field for somebody else's payload. + expect(beaconEventColumn).toContain("cs_uri_query"); + expect(beaconEventColumn).toContain(`'${beaconParameters.event}'`); + }); + + it("counts only requests carrying an envelope", () => { + // Given the conditions a rollup filters beacon rows with. + // Then a request to the beacon's path with no version parameter is left + // out. A crawler that found the URL in a page's source sends one of + // those, and counting it would report an event nobody caused. + expect(aBeaconEvent).toContain("cs_uri_query <> '-'"); + expect(aBeaconEvent.join(" ")).toContain(`'${beaconParameters.version}'`); + }); + + it("names the path where the envelope's own conditions leave it out", () => { + // Given the condition status-codes takes the beacon's requests back out + // with. + // Then it names the path, where `aBeaconEvent` leaves the path to the + // request's own `paths`. The two run in opposite directions. It reads + // the column as delivered, since the path carries nothing a browser or + // CloudFront escapes. + expect(outsideTheBeaconPath).toContain(`'${defaultBeaconPath}'`); + expect(outsideTheBeaconPath).not.toContain("url_decode"); + }); +}); diff --git a/src/beacon-rows.ts b/src/beacon-rows.ts new file mode 100644 index 0000000..f600825 --- /dev/null +++ b/src/beacon-rows.ts @@ -0,0 +1,67 @@ +// Reading a beacon event back off the rows CloudFront wrote. +// +// Apart from `beacon-events.ts` because the two halves have different +// readers. That module builds a query string inside a browser and this one +// builds SQL for Athena, and a site folding the beacon into its own bundle +// was carrying both. KensioSoftware/rainlytics#110 measured what that cost +// and split them. +// +// The payload stays in `cs_uri_query` and is read at query time, the way +// `searches` already reads a search term out of the same column. #100 settled +// that, and `beacon-events.ts` carries the rest of the reasoning along with +// the parameter names these expressions read. + +import { decodedParameter } from "./log-encoding.js"; +import { quoted } from "./sql-text.js"; + +import { beaconParameters, defaultBeaconPath } from "./beacon-events.js"; + +/** The envelope version a row was written under, as SQL. */ +export const beaconVersionColumn = decodedParameter(beaconParameters.version); + +/** What happened, as SQL. */ +export const beaconEventColumn = decodedParameter(beaconParameters.event); + +/** The page it happened on, as SQL. */ +export const beaconPageColumn = decodedParameter(beaconParameters.page); + +/** + * The rows a beacon event is, as conditions for `rowsFor`. + * + * The path is not among them. A rollup narrows to the beacon's path through + * the request's own `paths`, the way any other question narrows to a section + * of a site, and a site that moved its beacon then says so in one place. + * + * These leave out anything else reaching the same path. A crawler following + * a beacon URL out of a page's source carries no version parameter, and the + * bot filter `rowsFor` applies has already taken most of them. + * + * ```typescript + * rowsFor({ ...request, paths: [defaultBeaconPath] }, aBeaconEvent); + * ``` + */ +export const aBeaconEvent: readonly string[] = [ + "cs_method = 'GET'", + "cs_uri_query <> '-'", + `${beaconVersionColumn} <> ''`, +]; + +/** + * The rows outside the beacon's path, as a condition for `rowsFor`. + * + * The other direction from {@link aBeaconEvent}, and it names the path that + * one leaves out. A question about beacon events narrows to the beacon + * through the request's own `paths`. A question about what the site answered + * has to take the beacon's requests back out, and `status-codes` is the one + * that does. + * + * Matched against the column as CloudFront delivered it, where `--path` + * decodes twice first. The path here is a constant this package chose and it + * carries nothing a browser or CloudFront escapes, so a record holds it as it + * was sent. An address somebody typed can hold anything. + * + * A prefix, the way every path match in Rainlytics is one. + */ +export const outsideTheBeaconPath = `strpos(cs_uri_stem, ${quoted( + defaultBeaconPath, +)}) <> 1`; diff --git a/src/cdk/log-table.test.ts b/src/cdk/log-table.test.ts index 7024d6e..3cce127 100644 --- a/src/cdk/log-table.test.ts +++ b/src/cdk/log-table.test.ts @@ -10,13 +10,12 @@ import { describe, expect, it } from "vitest"; import { deployStacks, simStartedAt } from "#test/simulated-deployment.js"; import { defaultLogDataset, qualifiedTableName } from "../dataset.js"; +import { beaconQueryString, defaultBeaconPath } from "../beacon-events.js"; import { beaconEventColumn, beaconPageColumn, - beaconQueryString, beaconVersionColumn, - defaultBeaconPath, -} from "../beacon-events.js"; +} from "../beacon-rows.js"; import { deliveredLogColumnNames, logFieldNamesWithoutAddress, diff --git a/src/index.ts b/src/index.ts index 2520edf..15eca4d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -74,17 +74,19 @@ export { windowRange, } from "./summary-runs.js"; export { - aBeaconEvent, type BeaconEvent, - beaconEventColumn, - beaconPageColumn, beaconParameters, beaconQueryString, beaconSchemaVersion, - beaconVersionColumn, defaultBeaconPath, - outsideTheBeaconPath, } from "./beacon-events.js"; +export { + aBeaconEvent, + beaconEventColumn, + beaconPageColumn, + beaconVersionColumn, + outsideTheBeaconPath, +} from "./beacon-rows.js"; export { decodedColumn, decodedParameter } from "./log-encoding.js"; export { lastRange, diff --git a/src/rollup-questions.ts b/src/rollup-questions.ts index 1e98511..c94254a 100644 --- a/src/rollup-questions.ts +++ b/src/rollup-questions.ts @@ -6,7 +6,8 @@ // rollup that filtered differently would answer a different question from // its neighbours without saying so. -import { defaultBeaconPath, outsideTheBeaconPath } from "./beacon-events.js"; +import { defaultBeaconPath } from "./beacon-events.js"; +import { outsideTheBeaconPath } from "./beacon-rows.js"; import { qualifiedTableName } from "./dataset.js"; import { decodedColumn, decodedParameter } from "./log-encoding.js"; import { diff --git a/src/rollups.test.ts b/src/rollups.test.ts index e2846b5..95e2884 100644 --- a/src/rollups.test.ts +++ b/src/rollups.test.ts @@ -3,7 +3,8 @@ import { describe, expect, it } from "vitest"; import { defaultLogDataset, qualifiedTableName } from "./dataset.js"; import { decodedParameter } from "./log-encoding.js"; -import { defaultBeaconPath, outsideTheBeaconPath } from "./beacon-events.js"; +import { defaultBeaconPath } from "./beacon-events.js"; +import { outsideTheBeaconPath } from "./beacon-rows.js"; import { rollups } from "./rollup-questions.js"; import type { Rollup, RollupRequest } from "./rollups.js"; import {