Skip to content

Gate fetch priority collection behind a --priority flag - #345

Draft
sergeychernyshev wants to merge 3 commits into
mainfrom
gate-fetch-priority-behind-flag
Draft

Gate fetch priority collection behind a --priority flag#345
sergeychernyshev wants to merge 3 commits into
mainfrom
gate-fetch-priority-behind-flag

Conversation

@sergeychernyshev

Copy link
Copy Markdown
Member

Summary

Collecting fetch priorities requires tagging every request with an x-telescope-id header, so Chromium's CDP request IDs can be matched to HAR entries. Injecting that header needs a catch-all page.route() handler — and registering one disables the browser HTTP cache. That changes what is being measured: resources advertised in an Early Hints (103) response get re-requested instead of reused.

This makes the whole feature opt-in via --priority, defaulting to off.

Changes

Gating

  • testRunner.ts — the catch-all route and the requestfinished collector move into setupPriorityCorrelation(), which returns early unless the flag is set.
  • chromeRunner.ts — the three CDP Network.* priority listeners move into collectPriorities(), only called when the flag is set. Network.enable is no longer sent unconditionally. --cpuThrottle still works either way (it uses the Emulation domain).
  • Flag plumbed through LaunchOptions, DefaultOptions, CLIOptions, DEFAULT_OPTIONS, the Commander options, and normalizeCLIConfig.

mergeEntries() needed no guard — with the flag off, this.requests is empty, so it is already a no-op.

README

  • Removed the global x-telescope-id note from the top of the document.
  • Added a ### Fetch priority section: what the flag does, the _initialPriority / _priority HAR fields, and Chromium-only support.
  • Added a #### The x-telescope-id header subsection explaining why the header exists, that it is sent to the server, that it disables the HTTP cache, and that it changes the recorded header shape.
  • Added --priority to the --help block.

Behaviour change worth reviewing

Two things follow from the header no longer being injected by default.

1. HAR request headers are now the wire representation. Requests no longer pass back through a route handler, so the HAR records what actually went out rather than the set Telescope replayed:

entries[0].request.headers
--priority Accept, Upgrade-Insecure-Requests, User-Agent, x-telescope-id, …
default :authority, :method, :path, :scheme, accept, …, user-agent

The default is arguably more accurate. cli.test.ts was matching 'User-Agent' case-sensitively and relying on the interception artifact, so it now matches case-insensitively per RFC 9110 §5.1. Documented in the README for anyone consuming the HAR.

2. Per-entry timing enrichment is also gated. The same correlation drives _dns_start/_connect_*/_request_*/_response_*/_resourceType/_is_lcp, because the header is what disambiguates several requests sharing one URL. duplicateRequests.test.ts — whose point is that duplicate-URL entries each get their own timings — now opts in with priority: true. Worth a look if any downstream consumer expects those fields by default.

Tests

New __tests__/priorityFlag.test.ts (11 tests):

  • setupPriorityCorrelation() touches neither page.route() nor page.on() when the flag is unset or false; registers the **/* route and the requestfinished listener when true.
  • Same assertions via preparePage(), so the call-site wiring is covered.
  • DEFAULT_OPTIONS.priority is false; --priority round-trips into config.json.
  • End-to-end against a local fixture server: no x-telescope-id header and no priority fields in the HAR by default; header present on every entry with the flag on.

priority.test.ts updated to pass priority: true.

Full suite green in headless CI mode (npm run test:ci): 16 files, 418 tests.

Refs

Collecting fetch priorities requires tagging every request with an
x-telescope-id header so CDP request IDs can be matched to HAR entries.
Injecting that header needs a catch-all page.route() handler, and
registering one disables the browser HTTP cache, which changes what is
being measured — Early Hints resources get re-requested instead of reused.

Make the whole feature opt-in via --priority (default false):

- testRunner: move the catch-all route and the requestfinished collector
  into setupPriorityCorrelation(), which returns early unless enabled
- chromeRunner: move the CDP Network priority listeners into
  collectPriorities(), only called when enabled. Network.enable is no
  longer sent unconditionally; --cpuThrottle keeps working either way
- add the flag to LaunchOptions, DefaultOptions, CLIOptions,
  DEFAULT_OPTIONS, the Commander options and normalizeCLIConfig

With the flag off no extra header is sent and the cache is left intact.
Note this also means the HAR records the headers as they went out on the
wire (lower-cased, with HTTP/2 pseudo-headers) rather than the canonically
cased set replayed through the route handler, so cli.test.ts now matches
User-Agent case-insensitively per RFC 9110 section 5.1.

README: drop the global x-telescope-id note from the top of the document
in favour of a Fetch priority section that documents the flag, the two
HAR extension fields, and a dedicated subsection on the header covering
why it exists and its cache and header-shape consequences.

Refs: #327
Copilot AI lite review requested due to automatic review settings July 31, 2026 20:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Makes fetch-priority collection opt-in via a new --priority flag to avoid the Playwright page.route() side effect of disabling the browser HTTP cache (notably impacting Early Hints / 103 behavior). This keeps default runs closer to real browser caching behavior while still allowing correlation-based enrichment when explicitly requested.

Changes:

  • Plumbs a new priority option through types/defaults/CLI and gates request correlation + CDP priority listeners behind it.
  • Updates documentation to describe --priority, the HAR extension fields, and the x-telescope-id implications.
  • Adds/updates tests to verify the flag wiring and the default/no-header behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/telescope/src/types.ts Adds priority to LaunchOptions/DefaultOptions/CLIOptions types.
packages/telescope/src/testRunner.ts Moves the catch-all route + requestfinished correlation into setupPriorityCorrelation() gated by options.priority.
packages/telescope/src/index.ts Adds --priority Commander option and updates API note about x-telescope-id being priority-gated.
packages/telescope/src/defaultOptions.ts Sets DEFAULT_OPTIONS.priority = false.
packages/telescope/src/config.ts Normalizes CLI config to include priority.
packages/telescope/src/chromeRunner.ts Gates CDP Network.* priority listeners behind options.priority (and avoids unconditional Network.enable).
packages/telescope/README.md Documents fetch priority collection and x-telescope-id tradeoffs; adds --priority to help text.
packages/telescope/tests/priorityFlag.test.ts New tests covering gating behavior, CLI wiring, and end-to-end header presence/absence.
packages/telescope/tests/priority.test.ts Opts into priority collection for the priority assertions.
packages/telescope/tests/duplicateRequests.test.ts Opts into priority/correlation since per-entry timing enrichment depends on it.
packages/telescope/tests/cli.test.ts Makes User-Agent header matching case-insensitive to align with “wire” header casing when not routing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/telescope/src/config.ts
Comment thread packages/telescope/README.md Outdated
- `_initialPriority` — the priority Chromium assigned when the request was created
- `_priority` — the final priority, after any mid-flight change. Equal to `_initialPriority` when the priority was never changed.

Priority data comes from the Chrome DevTools Protocol, so it is only available on Chromium engines (`chrome`, `chrome-beta`, `chromium`, `canary`, `edge`). The flag has no effect on Firefox or Safari.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the sentence was wrong. Fixed in 7768090.

You're right about the mechanism: setupPriorityCorrelation() is on TestRunner, and TestRunner.createPage() (the Firefox/Safari path) calls preparePage(), so the catch-all route and header injection happen on every engine. Confirmed empirically on Firefox with --priority:

header=true  _dns_start=true  _priority=false

So the header is sent and the cache is disabled, the timing correlation works, and only the priority fields are missing.

I considered restricting the correlation to Chromium so the flag really would be a no-op elsewhere, but that would break the per-entry timing enrichment (_dns_start, _connect_start, _resourceType, …) which is engine-independent and genuinely useful — duplicateRequests.test.ts depends on it and runs Firefox-only in CI. The header is the only thing that disambiguates several requests sharing a URL.

So I reworded rather than re-gated, and spelled out what each engine actually gets, including the timing fields the correlation adds everywhere.

@sergeychernyshev
sergeychernyshev marked this pull request as draft July 31, 2026 20:52
…ort wording

- config.ts: use ?? instead of || when defaulting priority, so an explicit
  false is not treated as unset. No behaviour change while the default is
  false, but it stops the default from becoming impossible to override if
  that ever changes. Locked in with normalizeCLIConfig tests.

- README: the claim that --priority has no effect on Firefox or Safari was
  wrong. setupPriorityCorrelation() lives on TestRunner, so the flag injects
  x-telescope-id and disables the cache on every browser; only the priority
  fields are Chromium-only. Reworded to say what each browser actually gets,
  including the per-entry timing fields the correlation adds everywhere.

Verified on Firefox with --priority: header injected, _dns_start present,
_priority absent.
Copilot AI review requested due to automatic review settings August 4, 2026 14:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/telescope/README.md:243

  • The text says “Two consequences…”, but the list that follows contains three bullet points. This can confuse readers about what to expect when enabling --priority.
Two consequences are worth knowing before you enable it:

The header exists solely to join CDP priority events to HAR entries, and
only Chromium reports those. Injecting it on Firefox or Safari disabled
the HTTP cache and sent an extra header to the origin for no benefit.

Gate the correlation on the browser engine as well as the flag:
supportsFetchPriority() checks for the chromium engine, and passing
--priority to another engine now warns and is otherwise a no-op.

Consequence: the per-entry timing enrichment that rides the same
correlation (_dns_start, _connect_start, _request_start, _response_end,
_resourceType, _is_lcp) is now Chromium-only too, since the header is what
tells apart several requests sharing a URL. The duplicate-request timing
test is scoped to Chromium engines accordingly, and skips when the matrix
has none (CI runs Firefox only).

Docs and help text updated to say the flag is ignored off Chromium.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants