feat: add otel tracing and perf budgets#619
Merged
shayancoin merged 3 commits intomainfrom Nov 1, 2025
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/perf-budgets.yml(1 hunks)frontend/next.config.mjs(1 hunks)frontend/package.json(1 hunks)frontend/playwright.config.ts(1 hunks)frontend/src/api/client.ts(2 hunks)frontend/src/app/components/Viewer3D.tsx(4 hunks)frontend/src/hooks/usePricePreview.ts(4 hunks)frontend/src/instrumentation.ts(1 hunks)frontend/src/lib/telemetry.ts(1 hunks)frontend/tests/e2e/perf/configurator.perf.spec.ts(1 hunks)frontend/tests/e2e/perf/perf-reporter.ts(1 hunks)tools/scripts/validate-budgets.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/tests/e2e/perf/perf-reporter.ts (1)
tools/scripts/validate-budgets.js (2)
path(3-3)fs(2-2)
frontend/src/app/components/Viewer3D.tsx (1)
frontend/src/lib/telemetry.ts (1)
computeOptionsHash(45-45)
frontend/src/hooks/usePricePreview.ts (3)
frontend/src/lib/api/price.ts (2)
PricePreviewRequest(28-33)PricePreviewResponse(75-85)frontend/src/lib/telemetry.ts (1)
computeOptionsHash(45-45)frontend/src/api/client.ts (1)
ApiError(5-19)
frontend/src/instrumentation.ts (1)
frontend/src/api/client.ts (1)
request(97-120)
frontend/tests/e2e/perf/configurator.perf.spec.ts (2)
frontend/tools/perf/run-perf-budget.js (9)
page(148-148)context(147-147)browser(390-390)index(282-282)values(406-408)sorted(275-275)lower(283-283)upper(284-284)weight(288-288)tools/scripts/validate-budgets.js (2)
metrics(49-49)metrics(58-58)
🪛 GitHub Check: CodeQL
.github/workflows/perf-budgets.yml
[warning] 12-79: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/perf-budgets.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 80-80: too many blank lines (1 > 0)
(empty-lines)
Comment on lines
+109
to
+128
| try { | ||
| const response = await previewWithSpan(payload); | ||
| span.setAttribute('http.status_code', 200); | ||
| span.setAttribute('price.total', response.total); | ||
| span.setStatus({ code: SpanStatusCode.OK }); | ||
| return response; | ||
| } catch (error) { | ||
| if (error instanceof ApiError && typeof error.status === 'number') { | ||
| span.setAttribute('http.status_code', error.status); | ||
| } | ||
| if (error instanceof Error) { | ||
| span.recordException(error); | ||
| span.setStatus({ code: SpanStatusCode.ERROR, message: error.message }); | ||
| } else { | ||
| span.setStatus({ code: SpanStatusCode.ERROR, message: 'Unknown error' }); | ||
| } | ||
| throw error; | ||
| } finally { | ||
| span.end(); | ||
| } |
Contributor
There was a problem hiding this comment.
Stop the infinite recursion in previewWithSpan.
The span helper is calling itself again, so every invocation recurses until the stack blows up and the pricing request never hits the API. We need to invoke the real previewPrice call inside the span.
Apply this diff:
- const response = await previewWithSpan(payload);
+ const response = await previewPrice(payload);🤖 Prompt for AI Agents
In frontend/src/hooks/usePricePreview.ts around lines 109 to 128, the span
helper currently calls previewWithSpan recursively causing infinite recursion;
replace the recursive call with the actual previewPrice API call (pass the same
payload), so the span wraps the real network request instead of invoking itself.
Ensure the try block awaits previewPrice(payload), sets the span
attributes/status from that response, and preserves the existing error handling
and span.end() in finally.
Comment on lines
+230
to
+233
| export async function register(): Promise<void> { | ||
| await registerNodeInstrumentation(); | ||
| await registerBrowserInstrumentation(); | ||
| } |
Contributor
There was a problem hiding this comment.
Browser tracer never initializes
register() only executes on Node.js/Edge runtimes, so window is always undefined when this file runs. Consequently registerBrowserInstrumentation() returns immediately, no browser tracer provider ever registers, and every client span (Viewer3D, price preview, etc.) remains a no-op. Next.js explicitly notes that register runs once per server instance and never in the browser.(nextjs.org) Please move the browser initialization into a client-side entry point (for example, export registerBrowserInstrumentation and call it from a 'use client' wrapper in app/layout.tsx) or otherwise ensure it executes in the browser before spans are created.
🤖 Prompt for AI Agents
In frontend/src/instrumentation.ts around lines 230-233, register() is only
invoked on the server so window is undefined and
registerBrowserInstrumentation() never runs; export
registerBrowserInstrumentation (if not already) and remove browser
initialization from server-only register(), then call
registerBrowserInstrumentation() from a client-side entry (e.g., create a small
'use client' wrapper in app/layout.tsx or a dedicated client bootstrap file and
invoke it there before any spans are created) so the browser tracer provider is
initialized in the browser runtime.
Comment on lines
+278
to
+325
| function aggregateMetrics(coldRuns: RunMetrics[], warmRuns: RunMetrics[]): AggregatedMetrics { | ||
| const coldTti = coldRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | ||
| const warmTti = warmRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | ||
| const quoteDurations = [...coldRuns, ...warmRuns] | ||
| .flatMap((run) => run.priceQuoteDurations) | ||
| .filter((value) => Number.isFinite(value)); | ||
| const compositorDurations = [...coldRuns, ...warmRuns] | ||
| .flatMap((run) => run.compositorDurations) | ||
| .filter((value) => Number.isFinite(value)); | ||
| const assetDurations = coldRuns.flatMap((run) => run.assetFetchDurations).filter((value) => Number.isFinite(value)); | ||
|
|
||
| const ttiColdCv = computeCv(coldTti); | ||
| const ttiWarmCv = computeCv(warmTti); | ||
| const quoteCv = computeCv(quoteDurations); | ||
| const compositorCv = computeCv(compositorDurations); | ||
| const assetCv = computeCv(assetDurations); | ||
|
|
||
| const flakeGuardTriggered = [ttiColdCv, ttiWarmCv, quoteCv, compositorCv, assetCv].some((value) => value > 0.2); | ||
|
|
||
| return { | ||
| tti_cold: { | ||
| runs: coldTti, | ||
| median: median(coldTti), | ||
| cv: ttiColdCv, | ||
| }, | ||
| tti_warm: { | ||
| runs: warmTti, | ||
| median: median(warmTti), | ||
| cv: ttiWarmCv, | ||
| }, | ||
| price_quote: { | ||
| runs: quoteDurations, | ||
| p95: percentile(quoteDurations, 0.95), | ||
| cv: quoteCv, | ||
| }, | ||
| compositor_compose: { | ||
| runs: compositorDurations, | ||
| p95: percentile(compositorDurations, 0.95), | ||
| cv: compositorCv, | ||
| }, | ||
| asset_fetch_cold: { | ||
| runs: assetDurations, | ||
| p95: percentile(assetDurations, 0.95), | ||
| cv: assetCv, | ||
| }, | ||
| flakeGuardTriggered, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when we capture zero usable samples
aggregateMetrics filters out non-finite values and happily proceeds even if every run produced NaN/empty data, so a broken instrumentation path yields empty arrays → medians/p95s of 0 and flakeGuardTriggered === false. The perf budget check would then pass with no signal, exactly when we need it to fail. Please assert that we retain the expected sample counts (cold & warm TTIs) and that span-based metrics aren’t empty before returning. Throwing here will surface regressions instead of silently masking them.
Apply this diff:
function aggregateMetrics(coldRuns: RunMetrics[], warmRuns: RunMetrics[]): AggregatedMetrics {
const coldTti = coldRuns.map((run) => run.tti).filter((value) => Number.isFinite(value));
const warmTti = warmRuns.map((run) => run.tti).filter((value) => Number.isFinite(value));
const quoteDurations = [...coldRuns, ...warmRuns]
.flatMap((run) => run.priceQuoteDurations)
.filter((value) => Number.isFinite(value));
const compositorDurations = [...coldRuns, ...warmRuns]
.flatMap((run) => run.compositorDurations)
.filter((value) => Number.isFinite(value));
const assetDurations = coldRuns.flatMap((run) => run.assetFetchDurations).filter((value) => Number.isFinite(value));
+
+ if (coldTti.length !== coldRuns.length) {
+ throw new Error(`Expected ${coldRuns.length} cold TTI samples, got ${coldTti.length}.`);
+ }
+ if (warmTti.length !== warmRuns.length) {
+ throw new Error(`Expected ${warmRuns.length} warm TTI samples, got ${warmTti.length}.`);
+ }
+ if (quoteDurations.length === 0) {
+ throw new Error('No price.quote spans captured for any run.');
+ }
+ if (compositorDurations.length === 0) {
+ throw new Error('No compositor.compose spans captured for any run.');
+ }
+ if (assetDurations.length === 0) {
+ throw new Error('No asset.fetch spans captured during cold runs.');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function aggregateMetrics(coldRuns: RunMetrics[], warmRuns: RunMetrics[]): AggregatedMetrics { | |
| const coldTti = coldRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | |
| const warmTti = warmRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | |
| const quoteDurations = [...coldRuns, ...warmRuns] | |
| .flatMap((run) => run.priceQuoteDurations) | |
| .filter((value) => Number.isFinite(value)); | |
| const compositorDurations = [...coldRuns, ...warmRuns] | |
| .flatMap((run) => run.compositorDurations) | |
| .filter((value) => Number.isFinite(value)); | |
| const assetDurations = coldRuns.flatMap((run) => run.assetFetchDurations).filter((value) => Number.isFinite(value)); | |
| const ttiColdCv = computeCv(coldTti); | |
| const ttiWarmCv = computeCv(warmTti); | |
| const quoteCv = computeCv(quoteDurations); | |
| const compositorCv = computeCv(compositorDurations); | |
| const assetCv = computeCv(assetDurations); | |
| const flakeGuardTriggered = [ttiColdCv, ttiWarmCv, quoteCv, compositorCv, assetCv].some((value) => value > 0.2); | |
| return { | |
| tti_cold: { | |
| runs: coldTti, | |
| median: median(coldTti), | |
| cv: ttiColdCv, | |
| }, | |
| tti_warm: { | |
| runs: warmTti, | |
| median: median(warmTti), | |
| cv: ttiWarmCv, | |
| }, | |
| price_quote: { | |
| runs: quoteDurations, | |
| p95: percentile(quoteDurations, 0.95), | |
| cv: quoteCv, | |
| }, | |
| compositor_compose: { | |
| runs: compositorDurations, | |
| p95: percentile(compositorDurations, 0.95), | |
| cv: compositorCv, | |
| }, | |
| asset_fetch_cold: { | |
| runs: assetDurations, | |
| p95: percentile(assetDurations, 0.95), | |
| cv: assetCv, | |
| }, | |
| flakeGuardTriggered, | |
| }; | |
| } | |
| function aggregateMetrics(coldRuns: RunMetrics[], warmRuns: RunMetrics[]): AggregatedMetrics { | |
| const coldTti = coldRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | |
| const warmTti = warmRuns.map((run) => run.tti).filter((value) => Number.isFinite(value)); | |
| const quoteDurations = [...coldRuns, ...warmRuns] | |
| .flatMap((run) => run.priceQuoteDurations) | |
| .filter((value) => Number.isFinite(value)); | |
| const compositorDurations = [...coldRuns, ...warmRuns] | |
| .flatMap((run) => run.compositorDurations) | |
| .filter((value) => Number.isFinite(value)); | |
| const assetDurations = coldRuns.flatMap((run) => run.assetFetchDurations).filter((value) => Number.isFinite(value)); | |
| if (coldTti.length !== coldRuns.length) { | |
| throw new Error(`Expected ${coldRuns.length} cold TTI samples, got ${coldTti.length}.`); | |
| } | |
| if (warmTti.length !== warmRuns.length) { | |
| throw new Error(`Expected ${warmRuns.length} warm TTI samples, got ${warmTti.length}.`); | |
| } | |
| if (quoteDurations.length === 0) { | |
| throw new Error('No price.quote spans captured for any run.'); | |
| } | |
| if (compositorDurations.length === 0) { | |
| throw new Error('No compositor.compose spans captured for any run.'); | |
| } | |
| if (assetDurations.length === 0) { | |
| throw new Error('No asset.fetch spans captured during cold runs.'); | |
| } | |
| const ttiColdCv = computeCv(coldTti); | |
| const ttiWarmCv = computeCv(warmTti); | |
| const quoteCv = computeCv(quoteDurations); | |
| const compositorCv = computeCv(compositorDurations); | |
| const assetCv = computeCv(assetDurations); | |
| const flakeGuardTriggered = [ttiColdCv, ttiWarmCv, quoteCv, compositorCv, assetCv].some((value) => value > 0.2); | |
| return { | |
| tti_cold: { | |
| runs: coldTti, | |
| median: median(coldTti), | |
| cv: ttiColdCv, | |
| }, | |
| tti_warm: { | |
| runs: warmTti, | |
| median: median(warmTti), | |
| cv: ttiWarmCv, | |
| }, | |
| price_quote: { | |
| runs: quoteDurations, | |
| p95: percentile(quoteDurations, 0.95), | |
| cv: quoteCv, | |
| }, | |
| compositor_compose: { | |
| runs: compositorDurations, | |
| p95: percentile(compositorDurations, 0.95), | |
| cv: compositorCv, | |
| }, | |
| asset_fetch_cold: { | |
| runs: assetDurations, | |
| p95: percentile(assetDurations, 0.95), | |
| cv: assetCv, | |
| }, | |
| flakeGuardTriggered, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/configurator.perf.spec.ts around lines 278-325, the
function aggregateMetrics currently filters non-finite values and silently
returns metrics even when all samples were invalid; add explicit validation
after computing coldTti, warmTti, quoteDurations, compositorDurations, and
assetDurations: assert that coldTti.length === coldRuns.length and
warmTti.length === warmRuns.length (or at least > 0 if some runs may
legitimately be missing), and assert that quoteDurations.length,
compositorDurations.length, and assetDurations.length are > 0; if any check
fails, throw an Error with a clear message naming the missing/empty metric so
the test fails fast and surfaces instrumentation regressions.
Comment on lines
+25
to
+39
| onTestEnd(_test: TestCase, result: TestResult): void { | ||
| const attachment = result.attachments.find((item) => item.name === 'perf-metrics.json'); | ||
| if (!attachment || attachment.body === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const buffer = Buffer.isBuffer(attachment.body) | ||
| ? attachment.body | ||
| : Buffer.from(attachment.body); | ||
| const parsed = JSON.parse(buffer.toString('utf8')) as JsonValue; | ||
| this.metrics = parsed; | ||
| } catch (error) { | ||
| console.warn('Failed to parse perf-metrics attachment', error); | ||
| } |
Contributor
There was a problem hiding this comment.
Support attachments saved to disk in PerfReporter.
Playwright frequently persists attachments as files (filling attachment.path) instead of embedding the body. With the current guard we drop those cases, so the metrics file never gets written and the budgets workflow has nothing to validate.
Apply this diff so we read file-backed attachments too:
- const attachment = result.attachments.find((item) => item.name === 'perf-metrics.json');
- if (!attachment || attachment.body === undefined) {
+ const attachment = result.attachments.find((item) => item.name === 'perf-metrics.json');
+ if (!attachment) {
return;
}
try {
- const buffer = Buffer.isBuffer(attachment.body)
- ? attachment.body
- : Buffer.from(attachment.body);
+ const body = attachment.body;
+ const buffer =
+ body !== undefined
+ ? (Buffer.isBuffer(body) ? body : Buffer.from(body))
+ : attachment.path
+ ? fs.readFileSync(attachment.path)
+ : null;
+ if (!buffer) {
+ return;
+ }
const parsed = JSON.parse(buffer.toString('utf8')) as JsonValue;
this.metrics = parsed;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/tests/e2e/perf/perf-reporter.ts around lines 25 to 39, the reporter
currently ignores attachments persisted to disk because it only handles
attachment.body; update the logic to also handle attachment.path when body is
undefined by reading the file from disk (e.g., using fs.readFileSync or
fs.promises.readFile), then convert the resulting Buffer (or string) to UTF-8
and JSON.parse it into this.metrics; keep the existing try/catch and warning on
parse failure and ensure you only proceed when either a body or a path yields
valid content.
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
2 participants
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.
PR Type
[Feature | Fix | Documentation | Other() ]
Short Description
...
Tests Added
...