Performance: add dry-run mode and sanity-range assertions to CodeVitals posting#49999
Draft
LiamSarsfield wants to merge 10 commits into
Draft
Performance: add dry-run mode and sanity-range assertions to CodeVitals posting#49999LiamSarsfield wants to merge 10 commits into
LiamSarsfield wants to merge 10 commits into
Conversation
…ls posting CodeVitals is append-only with no self-service rollback, so a bad metric (wrong key, out-of-range value, scale error) permanently pollutes the trend graph. This adds the Phase 0 safety layer before we expand the metrics surface. - --dry-run flag: build and print the payload, skip the POST, no token required (usable as a CI smoke test). Exposed as `pnpm report:dry`. - Sanity-range assertions: each typed metric is checked against SANITY_RANGES in scenarios.js before posting. Out-of-range values are logged and skipped, and the script exits non-zero so CI surfaces them. Because run-performance-tests.js spawns this script, the gate guards both `pnpm report` and the integrated `pnpm test` path. - Documented the staging-key convention and the bad-data escalation path in the README. FORMS-713
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
Bot
added
the
[Status] Needs Author Reply
www.codevitals.run 301-redirects the API to the apex. On a 301 fetch retries a POST as a GET with no body, so a metric posted to the www default would never land. Default CODEVITALS_URL to https://codevitals.run.
The guard returned true for any metricType absent from SANITY_RANGES (a typo, a forgotten row, or the legacy untyped path), and coercion let null pass min-0 ranges and let numeric strings through as strings. Both post unchecked to an append-only store. checkSanityRange now rejects a typed metric with no range row and any non-finite value, and only a genuinely untyped legacy entry passes unchecked. Guard main() behind an import.meta.url check so the pure helpers can be imported, and add node:test coverage (pnpm test:unit) pinning the fail-closed contract: over-range skipped, non-finite/string/typo rejected, boundaries inclusive, untyped legacy passes.
The round-1 import guard compared import.meta.url against a raw
file://${argv[1]} string. Node percent-encodes and symlink-resolves
import.meta.url but argv[1] stays raw, so on a checkout whose path
contains a space or non-ASCII char (or via /tmp -> /private/tmp), the
match failed and main() silently never ran: the CLI exited 0 having
posted nothing. Replace it with isDirectInvocation(), which compares
realpath'd filesystem paths.
Also:
- Reject non-finite values for every entry, typed or untyped, by moving
the finite check above the untyped early return (never post null/NaN).
- Gate the unit tests on the integrated path: pnpm test now runs
node --test before the perf runner, so the guard is enforced wherever
this tool runs (tools/performance is outside the monorepo CI matrix).
- Add integration coverage for postToCodeVitals (in-range posts,
out-of-range skipped + validationFailed, missing file throws) and a CLI
test that runs the script from a path with a space (regression guard
for the bug above). Dry-run now returns the built payload so the
integration test can assert it.
- Point the README default and the perf runner's result link at the apex
host, matching the POST default.
Build the request URL with new URL() before attaching the token, so a malformed CODEVITALS_URL throws a generic error instead of a parse error that echoes the secret, and scrub token=... from any caught error before logging or rethrowing. Add live-POST tests (fetch stubbed) covering the payload, a non-OK response, and a malformed-URL redaction regression.
Redacting only the top-level error.message left the token in err.cause and util.inspect(err) when an upstream fetch error echoed the URL. Walk the caught error's whole cause chain and scrub it in place before logging or rethrowing, so the full error object is token-free. Also make the CLI test fixture explicitly ESM so it runs across the supported Node range, and document that CODEVITALS_URL must be origin-only.
extractScenarioMetrics now throws when a scenario sets metricKey but no metricType, instead of emitting an untyped entry that checkSanityRange would pass unchecked. That closed the one path the fail-closed guard is meant to protect: a future keyed metric posting any finite value to the append-only store. The current lcp scenario is unaffected. Also harden two tests: a dry run with a poisoned fetch proves it never posts, and the non-OK path now puts the token in the response body to prove the whole error (message, cause, util.inspect) is scrubbed.
…s build A sanity-check failure and a CodeVitals network outage both exited the poster with code 1, so --allow-codevitals-failure (meant to tolerate outages) also silently tolerated bad local data. Give validation failures a distinct exit code (2) that run-performance-tests.js never suppresses, and add a CLI test asserting an out-of-range dry run exits with it. Also extend sanitizeErrorChain to scrub the token from custom enumerable string error properties, not just message/stack/cause. Native fetch never populates these; this is belt-and-suspenders for a non-native HTTP client.
…or causes
Closes two gaps the round-6 hardening left open:
- A keyed scenario missing its metricType threw a plain Error, which main()
mapped to exit 1 — suppressible under --allow-codevitals-failure, despite
being local bad data exactly like an out-of-range metric. It now throws a
ValidationError that exitCodeForError maps to VALIDATION_FAILED_EXIT_CODE (2),
so the runner always fails the build on it.
- sanitizeErrorChain walked the cause chain but never redacted a primitive
string cause (new Error(m, { cause: someUrl })); cause is non-enumerable, so
the own-property pass missed it too, leaking the token into util.inspect. It
is now redacted in place before the walk advances.
Also makes run-performance-tests.js import-safe (guards main() with
isDirectInvocation, mirroring post-to-codevitals.js) and extracts the
build-fail decision into shouldFailBuildOnPostError, so the cross-file
validation/outage contract now has committed regression coverage.
Tests 27 -> 31, all green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
post-to-codevitals.jsbefore we expand the metrics surface area.--dry-runflag: builds and prints the full payload, skips the POST, and needs noCODEVITALS_TOKEN, so it works as a CI smoke test. Exposed aspnpm report:dry.SANITY_RANGESinscenarios.jsbefore posting. An out-of-range value is logged and skipped (never posted), and the script exits non-zero so CI surfaces it. Other valid metrics in the same run still post.SANITY_RANGESrow (a typo, or a forgotten row) and any non-finite value (null,NaN, a numeric string) are rejected rather than posted unchecked. The non-finite check runs for every entry, so even an untyped legacy entry can never postnull/NaN; only the range check is skipped for it. A scenario that sets an exactmetricKeybut omitsmetricTypenow fails closed as a validation error (it exits with the data-integrity code 2, surfaced by the dry-run smoke test, so the runner can never suppress it under--allow-codevitals-failure) instead of slipping through as an untyped entry and posting any finite value unchecked, which closes the exact path this guard protects. These typed cases are unreachable on today's singlelcpmetric but become reachable the moment a second metric type is wired, which is what this foundation protects.main()so the script runs only when invoked directly and the pure helpers stay importable by the tests. The check compares real filesystem paths (import.meta.filenamevs arealpath'dargv[1]), so a checkout whose path contains a space or non-ASCII char, or resolves through a symlink, still runsmain()instead of silently exiting 0 having done nothing.CODEVITALS_TOKENout of logs and errors. The request URL is now built withnew URL()before the token is attached, so a malformedCODEVITALS_URLthrows a generic error instead of a parse error that echoes the token. On any live-POST failure, the caught error and its fullcausechain are scrubbed of the token in place (message, stack, any custom enumerable string property, and a primitive stringcause— which is non-enumerable and so missed by the property pass) before the error is logged or rethrown, soerr.message,err.cause, andutil.inspect( err )are all token-free. Without this, a misconfigured host or an upstreamfetcherror could write the token straight into the build log.CODEVITALS_URLshould be origin-only (the API path is appended); the README notes this.run-performance-tests.jsalways fails the build on that code, even under--allow-codevitals-failure. That flag exists to tolerate CodeVitals network outages, and previously it also swallowed a local validation failure because both shared exit 1. The runner's build-fail decision is now a named, unit-tested predicate (shouldFailBuildOnPostError), and the runner is import-safe (itsmain()is guarded byisDirectInvocation, like the poster) so that cross-file contract has committed coverage.node:testunit tests for the guard (pnpm test:unit, no Docker/token/network), including a keyed-metric-without-metricTyperejection; integration tests forpostToCodeVitals(in-range posts, out-of-range skipped + non-zero exit, missing file throws, and a dry run that never callsfetcheven with a malformed URL and a token set); live-POST tests withfetchstubbed (payload sent as POST, non-OK throws, a malformed URL leaks the token into neither the error nor the logs, a token in the non-OK response body stays scrubbed acrosserr.message, thecausechain, andutil.inspect, a token-bearing upstream error stays redacted the same way, and a token-bearing primitive stringcauseis scrubbed too); a mapping test that a keyed-without-metricTypeconfig error is aValidationErrorthat resolves to the data-integrity exit code while a transport error resolves to 1; a CLI test that runs the script from a path containing a space (with an explicit{ "type": "module" }so it runs as ESM across the full supported Node range); and a cross-file test of the runner'sshouldFailBuildOnPostErrordecision (validation failures always fatal, transport failures suppressible only with--allow-codevitals-failure).pnpm testnow runsnode --testbefore the perf runner, so the guard is enforced on the integrated path (tools/performancesits outside the monorepo CI matrix).run-performance-tests.jsspawns this script and treats a non-zero exit as a failure, the gate guards bothpnpm reportand the integratedpnpm testpath.https://codevitals.run) by default. Thewww.host now 301-redirects the API, andfetchretries a redirected POST as a bodiless GET, so a metric sent to the oldwww.default never lands.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This adds safeguards to the existing CodeVitals posting tool. It changes nothing in shipped Jetpack code, and the existing LCP metric posts unchanged.
Testing instructions
tools/performance, runpnpm report:dry. It prints the CodeVitals payload and exits 0 without posting (no token needed).RESULTS_PATHat a results file whose median LCP is outside [100, 60000] (e.g. 70000) and runpnpm report:dry. The metric is logged as out-of-range, skipped, and the command exits non-zero.pnpm report:dryprintsCodeVitals URL: https://codevitals.run.tools/performance,pnpm test:unitruns thenode:testsuite (31 tests, no Docker/token/network) covering the fail-closed guard (including a keyed metric missing itsmetricType, and that this config error maps to the data-integrity exit code), the posting loop, the dry-run-never-posts short-circuit, the live-POST path withfetchstubbed (including the malformed-URL, response-body, custom-property, token-bearing-cause, and primitive-string-cause redaction regressions), a CLI invocation from a path containing a space, a CLI out-of-range dry run that exits with the data-integrity code (2), and the runner's build-fail decision (shouldFailBuildOnPostError: validation failures always fatal, transport failures suppressible only with the flag).