[PM-38460] experience impact metrics by audreyality · Pull Request #514 · bitwarden/browser-interactions-testing

audreyality · 2026-07-23T19:15:42Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-38460

📔 Objective

Introduce benchmark measuring frame rate drops; includes initial pass at "experience impact" measurements.

📖 Documentation

github-actions · 2026-07-23T19:16:28Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new experience-impact benchmarking harness: the in-page agent, the CDP capture channel (default/cpu/snapshot modes), the impact reporter and result writer, the run-benchmark CLI wrapper, the reworked benchmark config with per-mode projects, and the restructured performance docs. This is test-only tooling with no production or extension-runtime impact; blast radius is confined to the benchmarks/ and instrumentation/ harness.

Code Review Details

No new findings at or above the confidence threshold.

The harness is careful where it matters: file reads in both reporters use path-containment checks plus O_NOFOLLOW and skip non-regular entries; the CDP capture brackets each window, detaches sessions in a finally/safeDetach path, bounds the tracingComplete wait with a timeout, and poisons rather than reports untrustworthy captures; snapshot filenames use a monotonic sequence to avoid collisions across repeatEach. Known duplication is already flagged with in-code FIXME markers.

The one unresolved existing thread (benchmarks/fixtures.benchmark.ts:209-211, in-page agent install comment) is now stale: the context fixture no longer installs the agent, so the flagged contradiction was addressed by commit c9304a2 ("fix instrumentation double-install"). Not re-raised.

blackwood

The overall structure is solid, the stress level definitions are purposeful, and the modes provide an important level of detail. Attention to both feature support and failure modes are thorough. Approved, with a handful of questions/thoughts that can be addressed at author discretion.

blackwood · 2026-07-28T16:20:17Z

+          }
+        }
+      });
+      longTaskObserver.observe({ type: "longtask", buffered: true });


💭 I'm not sure if .observe will throw here... the spec is somewhat ambiguous, it says it will "abort", only specifying that the user agent may log something. Would this compromise the ability to suss out an actual unsupported feature?

I think we want PerformanceObserver.supportedEntryTypes?

Yuuuuuup.... 😬

blackwood · 2026-07-28T16:23:29Z

+  if (cdp) {
+    if (cdp.poisoned) {
+      bucket.cdpPoisoned++;
+      return;


❓ Will any failure on any of the operations set poisoned on the whole capture?

Poisoning is a predictability signal that the extension uses to communicate that its measures are unreliable. BIT does not perform poisoning, it only looks for it.

https://github.com/bitwarden/clients/blob/main/apps/browser/src/autofill/content/performance.md#poisoning

Poisoned measures exclude a run from experience impact analysis. Their raw data is still included in the detailed output.

https://github.com/bitwarden/browser-interactions-testing/blob/autofill/pm-38460/benchmark-experience-metrics/docs/performance-output.md#predictability

blackwood · 2026-07-28T16:30:29Z

+    function frame(now: number) {
+      const delta = now - lastFrame;
+      lastFrame = now;
+      if (delta < BACKGROUND_GAP_MS) {


❓ Any benefit to recording backgrounding of tabs in the in-page result?

blackwood · 2026-07-28T16:41:17Z

+
+      const snapshotLabel = `${testInfo.titlePath.join("_")}__run${testInfo.repeatEachIndex}`;
+
+      const measure = async (


💭 What should measure() do if the page navigates mid-window and addInitScript rebuilds the agent underneath it?

So, if I follow where you're going with this, the agent's producing inPage data. When we navigate, the PerformanceObserver is lost and, with it, the data it collected.

This is already accounted for in the design: https://github.com/bitwarden/browser-interactions-testing/blob/autofill/pm-38460/benchmark-experience-metrics/docs/performance.design.md#capturing-before-navigation

But I totally missed collecting the inPage data during navigation:

browser-interactions-testing/benchmarks/fixtures.benchmark.ts

Lines 177 to 181 in 6156d50

captures.push({

url,

timestamp: new Date().toISOString(),

results,

});

blackwood · 2026-07-28T16:43:16Z

+};
+
+/** Which CDP capture mode produced a result. */
+export type CaptureMode = "default" | "cpu" | "snapshot";


❓ Is the mode v. tier distinction important?

Tier is deciding how much stress we're applying to the test. Mode is deciding what data we're collecting.

This matters because only the default mode is suitable for comparing benchmarks. The other two pause the runtime. CPU mode does this to collect the call stack. Snapshot mode traverses the heap. The modes are only useful for diagnosing why a benchmark reported a performance regression.

design notes: https://github.com/bitwarden/browser-interactions-testing/blob/autofill/pm-38460/benchmark-experience-metrics/docs/performance.design.md#separating-the-capture-modes
usage notes: https://github.com/bitwarden/browser-interactions-testing/blob/autofill/pm-38460/benchmark-experience-metrics/docs/performance.md#cdp-channel-benchmarks-only

Also worth noting: the "cpu" and "snapshot" modes generate a ton of data. Running them for a full benchmark suite (4 tiers, 10 runs each) generated 2.2 GB of data. 🫨

blackwood · 2026-07-28T16:49:57Z

+
+// --- Frame timeline ---------------------------------------------------------
+
+function summarizeFrames(events: TraceEvent[]): CdpImpactResult["frames"] {


💭 Would named FrameStats etc.: GcStats, AllocationStats read any better than indexing a union type?

blackwood · 2026-07-28T16:55:37Z

+
+    if (!fs.existsSync(dir)) {
+      return;
+    }


💭 perf-summary-reporter.ts resolves the directory differently, not sure if this is an issue.

const dir = path.isAbsolute(this.inputFolder) ? path.resolve(this.inputFolder) : path.join(__dirname, this.inputFolder); const outputPath = path.isAbsolute(this.outputFile) ? this.outputFile : path.join(__dirname, this.outputFile);

I don't think it is, but perf-summary-reporter.ts is written that way because Akami pegged it as incorrect. It shouldn't harm anything to reuse the pattern.

blackwood · 2026-07-28T17:07:05Z

+          // workload rather than the teardown, matching the trace window.
+          inPage = await readInPageImpact(page);
+          cdp = await capture.stop();
+        }


❓ Won't a failed workload that throws never allow captures.push resolve? Do we want to capture partial captures in that case?

Also, should line 237 be wrapped in its own try/catch block?

😭 This whole section is kind of hard to read. Between the nesting induced by playwright and the abomination of prettier formatting, this code is nearly as bad as perl code. I wasn't happy about it, but I also didn't see any way that indirection would make it easier to read.

📝 Workloads that throw run the finally block, test the collected data to annotate the test, and then record the capture. That's all happening within the measure function.

📝 As I understand it, readInPageImpact(page) doesn't throw.

audreyality added 2 commits July 21, 2026 17:04

introduce experience impact benchmarking

90a57fa

rename "sweep" mode to "custom"

7b38600

audreyality requested a review from a team as a code owner July 23, 2026 19:15

audreyality requested a review from blackwood July 23, 2026 19:15

audreyality mentioned this pull request Jul 23, 2026

[PM-38460] add out-of-page status beacon bitwarden/clients#22016

Open

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread benchmarks/fixtures.benchmark.ts

audreyality added the ai-review-vnext Request a Claude code review using the vNext workflow label Jul 23, 2026

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread docs/performance.design.md Outdated

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread docs/performance.md Outdated

audreyality added 4 commits July 23, 2026 15:55

path validation

e7c4a03

fix typo

1f50abe

fix instrumentation double-install

c9304a2

fix a different typo

6156d50

blackwood approved these changes Jul 28, 2026

View reviewed changes


		const snapshotLabel = `${testInfo.titlePath.join("_")}__run${testInfo.repeatEachIndex}`;

		const measure = async (

	captures.push({
	url,
	timestamp: new Date().toISOString(),
	results,
	});


		// --- Frame timeline ---------------------------------------------------------

		function summarizeFrames(events: TraceEvent[]): CdpImpactResult["frames"] {

Uh oh!

Conversation

audreyality commented Jul 23, 2026

🎟️ Tracking

📔 Objective

📖 Documentation

Uh oh!

github-actions Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audreyality Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jul 23, 2026 •

edited

Loading

audreyality Jul 28, 2026 •

edited

Loading