fix(otel): inject experimental.clientTraceMetadata into SSR head#1644
Conversation
|
/bigbonk review |
commit: |
|
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(otel): inject experimental.clientTraceMetadata into SSR head
Clean implementation. The architecture is right — runtime logic lives in server/client-trace-metadata.ts, entries stay thin, and the config plumbing threads through all the correct paths. A few observations:
Good
- Architecture follows guidelines. The core logic is in
server/client-trace-metadata.ts(a proper typed module), not in generated entries. The entry files just pass the config value through. - XSS prevention is solid. Uses
escapeHtmlAttrfromserver/html.tsfor bothnameandcontentattributes. The test attests/client-trace-metadata.test.ts:72-78explicitly verifies injection is blocked. - Optional dependency pattern is correct. The
require()resolution viaglobalThis.requirewith try/catch is the right approach for an optional peer. Both the "not installed" and "propagator throws" paths return empty arrays. - Config parsing is defensive. The
clientTraceMetadataextraction atnext-config.ts:1251-1255properly filters to strings only, matching the pattern used forinstrumentationClientInjectand similar config fields. - All code paths covered. I traced
clientTraceMetadatathrough:next-config.ts→index.ts→app-rsc-entry.ts→app-page-dispatch.ts→app-page-render.ts→app-page-stream.ts→app-ssr-entry.ts(App Router), andnext-config.ts→index.ts→dev-server.ts(Pages Router dev), andnext-config.ts→pages-server-entry.ts→pages-page-response.ts(Pages Router prod). All paths are wired.
Issues
-
carrierSetter.settype guard is unnecessary but harmless (client-trace-metadata.ts:31). The OTelTextMapSettercontract guaranteeskeyandvalueare strings. The guard is defense-in-depth against a rogue propagator — fine to keep, but worth noting it's not matching Next.js behavior (their setter is a bare push). -
Lazy trace meta in
app-ssr-entry.tsis cached per-request, which is correct (app-ssr-entry.ts:422-428). Thelet traceMetaHTML: string | null = null+ lazy getter ensures it's computed once and only when the head injection path runs. This is well-done. -
app-ssr-entry.ts:425—?? undefinedcoercion. Theoptions?.clientTraceMetadata ?? undefinedconvertsnulltoundefinedto matchgetClientTraceMetadataHTML's parameter type (readonly string[] | undefined). This works but is a sign the type isn't fully harmonized —clientTraceMetadataflows asreadonly string[] | nullthrough most of the App Router pipeline butgetClientTraceMetadataHTMLacceptsundefined. Not a bug, just a minor inconsistency. -
No test for the config parsing path. The
tests/next-config.test.tschange (line 1022) only addsclientTraceMetadata: undefinedto the default resolved config fixture. There's no test verifying thatresolveNextConfig({ experimental: { clientTraceMetadata: ["key1", "key2"] } })producesclientTraceMetadata: ["key1", "key2"], or that non-string values in the array are filtered out. The unit tests intests/client-trace-metadata.test.tsare good for the runtime behavior, but a config-resolution test would close the loop. -
PR follow-up note on static vs dynamic. The PR description correctly flags that vinext emits trace meta tags whenever an active OTel span is present, regardless of static vs dynamic rendering. Next.js only injects them for dynamic renders in production (static pages are pre-rendered without an active request span). This is fine for now — in practice, prerendered pages won't have an active span at build time, so the behavior converges. But if ISR regeneration runs inside an OTel-instrumented context, the cached HTML would embed stale trace IDs. Worth keeping in mind for ISR parity.
Nits
- The comment at
dev-server.ts:1128-1131("Trace metadata is appended after Head shim output so it always lands in the final document head") could mention that this is intentionally aftergetSSRHeadHTML()so user-provided<Head>tags take precedence in document order.
Overall this is a well-structured change that follows the project's layering guidelines. The plumbing is thorough across all server entry points.
|
Review posted on PR #1644. Here's a summary of the findings: Overall: Clean, well-structured implementation that follows the project's architecture guidelines. The core logic lives in Key positives:
Notable items:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(otel): inject experimental.clientTraceMetadata into SSR head
Good change. Architecture is sound — runtime logic in server/client-trace-metadata.ts, entries stay thin, config threads correctly through all paths. I verified against the Next.js reference implementation. A few items to flag:
Correctness
Once-only injection in App Router is correct. The didInjectHeadHTML guard at app-ssr-entry.ts:429-433 ensures getTraceMetaHTML() is only called on the first getInsertedHTML() invocation (inside buildHeadInjectionHtml). Subsequent calls return insertedHTML + errorMetaHTML without trace meta. This mirrors Next.js's pattern of clearing traceMetaTags = [] after first flush in make-get-server-inserted-html.tsx:110.
Lazy computation is correctly scoped. The let traceMetaHTML: string | null = null + closure pattern at app-ssr-entry.ts:422-428 defers the OTel context read until the SSR shell renders, then caches it for the request lifetime. This is the right call — reading the OTel context too early (before the request span is active) would produce empty/wrong data.
XSS prevention is solid. escapeHtmlAttr handles ", <, >, & in both name and content attributes. The test at client-trace-metadata.test.ts:72-78 verifies this.
Parity gap: filterClientTraceMetadata with empty allow-list
Minor divergence from Next.js. In Next.js's getTracedMetadata:
if (!clientTraceMetadata) return undefined
return traceData.filter(...) // returns [] when config is []In vinext's filterClientTraceMetadata:
if (!allowList || allowList.length === 0) return undefined // returns undefined when config is []So clientTraceMetadata: [] in next.config produces undefined in vinext but [] in Next.js. Both render zero meta tags, so this is not a functional bug. But the config resolution test at next-config.test.ts:901-904 correctly asserts that clientTraceMetadata: [] resolves to [], so the divergence is localized to filterClientTraceMetadata. Might be worth aligning for consistency, but not blocking.
require() on every request — acceptable
getOpenTelemetryTraceData calls require("@opentelemetry/api") on every SSR render (when the allow-list is configured). This is fine — Node's require cache makes this a hash lookup. Next.js does the same thing at module load time (top-level require in tracer.ts:28-35) and then calls propagation.inject() per-request via getTracePropagationData(). The vinext approach is equivalent but delays the module resolution to first use, which is actually better for the no-OTel case since the require is never attempted.
The carrierSetter type guard
client-trace-metadata.ts:31 — the typeof key !== "string" || typeof value !== "string" guard is defense-in-depth that Next.js doesn't have (their setter at tracer.ts:193-199 is a bare push). The OTel TextMapSetter contract guarantees string types, so this guard only protects against a misbehaving propagator. Fine to keep — it's harmless and defensive.
Config test coverage is good
The previous review noted missing config-resolution tests. This has been addressed — tests/next-config.test.ts now has a dedicated resolveNextConfig clientTraceMetadata describe block covering: default undefined, string array, non-string filtering, non-array rejection, and empty array. This closes the loop the previous review flagged.
Dev/prod parity check
All four server entry points are wired:
- App Router dev:
index.ts:3363→dev-server.ts(but this is Pages Router dev — see note below) - App Router prod:
app-rsc-entry.ts:485,569→app-page-dispatch.ts→app-page-render.ts→app-page-stream.ts→app-ssr-entry.ts - Pages Router dev:
index.ts:3363→dev-server.ts:263,1135 - Pages Router prod:
pages-server-entry.ts:124,879→pages-page-response.ts:350
All paths thread clientTraceMetadata from config to the point of HTML injection. The plumbing is consistent.
One question on the dev-server wiring
index.ts:3363 passes clientTraceMetadata as the 10th positional argument to createSSRHandler. This function now has 10 positional parameters (dev-server.ts:248-263). This is getting unwieldy. Not blocking for this PR, but a future cleanup might want to move these into an options object. The positional approach is fragile — it's easy to mix up the boolean hasMiddleware with the string array clientTraceMetadata since TypeScript won't catch the mismatch if both are falsy.
ISR interaction (acknowledged follow-up)
The PR description correctly flags the ISR concern — cached HTML from a regeneration would embed the regeneration span's trace IDs. This is the right call to defer. Next.js has the same issue in production for dynamically rendered pages that get cached.
LGTM — solid implementation with thorough testing and correct architecture.
|
Review posted on PR #1644. Here's the summary: Overall: LGTM. The implementation is well-architected — runtime logic lives in Key findings:
|
james-elicx
force-pushed
the
fix/issue-1494-client-trace-metadata
branch
from
2dfab42 to
ef7eac8
Compare
|
Rebased on main. Conflicts in next-config.ts and pages-server-entry.ts were trivial (both branches added independent fields). |
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(otel): inject experimental.clientTraceMetadata into SSR head
LGTM. The implementation is architecturally sound, thoroughly tested, and follows the project's layering guidelines. I traced all code paths end-to-end and verified the plumbing is correct.
Architecture & Layering
Core runtime logic lives in server/client-trace-metadata.ts — a proper typed module with clean separation of concerns (OTel resolution, filtering, rendering). Generated entries stay thin: app-rsc-entry.ts just embeds __clientTraceMetadata as JSON, pages-server-entry.ts threads it through vinextConfig. This matches the AGENTS.md guidance on keeping entries as codegen glue.
All Code Paths Verified
I traced clientTraceMetadata through every server entry point:
- Config resolution:
next-config.ts:1266-1270— parsesexperimental.clientTraceMetadata, filters to strings only - App Router RSC entry:
index.ts:2231→app-rsc-entry.ts:485(embeds as__clientTraceMetadata) →app-rsc-entry.ts:569(passes to__dispatchAppPage) - App Router dispatch:
app-page-dispatch.ts:661→app-page-render.ts:521→app-page-stream.ts:120→app-ssr-entry.ts:425 - Pages Router dev:
index.ts:3363→dev-server.ts:264(10th positional arg) →dev-server.ts:1163-1167 - Pages Router prod:
pages-server-entry.ts:129(embedded invinextConfig) →pages-server-entry.ts:915→pages-page-response.ts:350-351
All paths are correctly wired. Dev and prod are in sync.
Correctness Details
-
Once-only injection in App Router: The
didInjectHeadHTMLguard atapp-ssr-entry.ts:429-433correctly ensures trace meta tags are only emitted in the firstgetInsertedHTML()call (insidebuildHeadInjectionHtml). This mirrors Next.js's pattern of clearingtraceMetaTags = []after first flush. -
Lazy OTel context read: The
let traceMetaHTML: string | null = null+ closure pattern atapp-ssr-entry.ts:422-428defers the OTel context read until the SSR shell renders, then caches for the request. This is correct — reading eagerly before the request span is active would produce wrong data. -
XSS prevention:
escapeHtmlAttrhandles",<,>,&in bothnameandcontentattributes. The test atclient-trace-metadata.test.ts:72-78explicitly verifies injection is blocked. -
Optional dependency:
require()viaglobalThis.requirewith try/catch is the right pattern for optional peers. Both "not installed" and "propagator throws" paths return empty arrays, verified in tests. -
Config test coverage: The
resolveNextConfig clientTraceMetadatadescribe block atnext-config.test.ts:873-923covers: default undefined, string array, non-string filtering, non-array rejection, and empty array. This closes the loop on config resolution.
Non-blocking Observations
-
filterClientTraceMetadatawith empty[]: Returnsundefinedwhile Next.js'sgetTracedMetadatawould return[]. Both produce zero meta tags — no functional impact. The config test at line 918-923 correctly asserts[]survives config resolution, so the divergence is localized to the filter function. Worth aligning for consistency in a follow-up, but not blocking. -
Positional parameter growth:
createSSRHandleratdev-server.ts:249-265now has 10 positional parameters. An options object would be less fragile for future additions. Non-blocking — the type system catches mismatches. -
ISR interaction: Correctly flagged in the PR description. Cached HTML from ISR regeneration would embed the regeneration span's trace IDs. This is the right call to defer — Next.js has the same open question for dynamically rendered pages that get cached.
Tests
140 lines of well-structured tests covering the filter, render, and end-to-end OTel mock paths. The test keys (my-test-key-1, my-test-key-2, my-parent-span-id) and the exclusion check for non-metadata-key-3 are ported from the Next.js test suite, which is the right approach per AGENTS.md.
|
Review posted on PR #1644 with approval. Here's a summary: Verdict: LGTM (approved) The implementation is architecturally sound and follows the project's layering guidelines. Key findings:
|
When `experimental.clientTraceMetadata` is configured in next.config, read the active OpenTelemetry context via the registered propagator and emit `<meta name="..." content="...">` tags in the SSR HTML head for each allow-listed key. Both App Router and Pages Router are covered (dev and prod servers). `@opentelemetry/api` is resolved as an optional peer at runtime, so apps without OTel get a zero-cost no-op. Mirrors Next.js: packages/next/src/server/lib/trace/utils.ts and packages/next/src/server/app-render/make-get-server-inserted-html.tsx.
knip flagged the helper as unused — it is only consumed internally by `getClientTraceMetadataHTML`. Keep it module-private.
Pick one convention for the optional allow-list across the App Router pipeline (was mixing `string[] | null` and `string[] | undefined`). Drops the redundant `?? null` / `?? undefined` coercions and aligns every server-side type with `ResolvedNextConfig.clientTraceMetadata`, which is `string[] | undefined`. Also adds a resolveNextConfig test asserting that an array passed via experimental.clientTraceMetadata is preserved verbatim, non-string entries are filtered, and unset / non-array values resolve to undefined.
james-elicx
force-pushed
the
fix/issue-1494-client-trace-metadata
branch
from
ef7eac8 to
bd8ea48
Compare
|
Rebased on main; conflict in |
Resolves a conflict in packages/vinext/src/server/app-ssr-entry.ts where cloudflare#1644 (clientTraceMetadata SSR head injection) and this PR both edited the post-`waitForAllReady` setup block. Kept both: the inline-css manifest read + font-merge wiring runs first, then the lazy `getTraceMetaHTML` closure.
1 participant
Summary
Fixes #1494.
experimental.clientTraceMetadatainnext.config: when configured, vinext reads the active OpenTelemetry propagation context and emits<meta name="..." content="...">tags in the SSR HTML head for each allow-listed key.app-ssr-entry.ts,dev-server.ts,pages-page-response.ts).@opentelemetry/apiis resolved as an optional peer via runtimerequire(...). Apps that don't install OTel get a zero-cost no-op; the path returns""and the head HTML is forwarded verbatim.packages/next/src/server/lib/trace/utils.ts(getTracedMetadata) andpackages/next/src/server/app-render/make-get-server-inserted-html.tsx(traceMetaTags).test/e2e/opentelemetry/client-trace-metadata/intotests/client-trace-metadata.test.ts(uses the same test keysmy-test-key-1,my-test-key-2,my-parent-span-id, and verifiesnon-metadata-key-3is excluded).Test plan
pnpm test tests/client-trace-metadata.test.ts— new unit tests cover filter, render, and end-to-end with a mocked OTel propagatorpnpm test tests/next-config.test.ts tests/app-page-dispatch.test.ts tests/app-page-render.test.ts tests/app-page-stream.test.ts tests/head.test.ts tests/pages-page-response.test.ts— 248 tests passpnpm run check— format, lint, types all greenclientTraceMetadatanot injected into SSR head #1494Follow-ups
<meta name="...">tags capture the regeneration span's trace IDs and are then served verbatim to subsequent requests until the next revalidation. This can mislead client-side tracing that joins ontrace-id/parent-span-id. Out of scope for this PR — needs a dedicated design pass (likely "skip injection for cacheable static renders" or "stamp + rewrite on each serve") and matching Next.js parity work. Tracked as future follow-up.