fix: OpenTelemetry v2 breaking changes and polyfill completeness - #3917
Conversation
Address post-merge review comments from PR #3911: 1. **Fix OTel v2 breaking change (parentSpanId → parentSpanContext)**: - ReadableSpan.parentSpanId (string) was renamed to parentSpanContext (SpanContext) in OTel v2 - Updated src/utils/opentelemetry.js to use span.parentSpanContext?.spanId - Updated src/ssr/server/mrt-console-span-exporter.js to use span.parentSpanContext?.spanId - Updated all test mocks to use parentSpanContext: {spanId: 'xxx'} - Restores parent span links in exported spans which were previously undefined 2. **Complete CBOR polyfills**: - Added TextEncoder alongside existing TextDecoder in Jest setup files - AWS SDK 3.x's @smithy/core CBOR encoding needs both encode and decode - Updated both internal-lib-build and pwa-kit-dev setup-jest.js 3. **Update Node.js engine requirements**: - Updated pwa-kit-runtime engines.node to ^18.19.0 || ^20.6.0 || ^22.0.0 || ^24.0.0 - Reflects actual requirements from OTel v2 and mrt-utilities 0.2.2 (AWS SDK 3.x) - Previous ^18.0.0 was inaccurate after dependency upgrades All 329 pwa-kit-react-sdk tests pass.
|
Git2Gus App is installed but the |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
adamraya
left a comment
There was a problem hiding this comment.
LGTM, I left small nit comments
Fuzzy-matched routes embed the full site/locale enumeration inline (e.g. /:site(us|RefArch)/:locale(en-US|en-CA|...)/category/:categoryId), which makes the http.route span attribute noisy in LogCenter. Strip the constraint groups so the reported template is a clean, low-cardinality /:site/:locale/category/:categoryId.
Back out the pwa-kit-runtime engines.node bump and its CHANGELOG entry. engines is advisory (no engine-strict), the OTel v2 floor vs mrt-utilities >=22.16.0 floor produce an internally inconsistent range, and honoring mrt's real floor would drop Node 18/20 — a breaking change out of scope for this fix. Tracked separately.
Address review nits on #3917: - Assert logged parentId resolves to the parent span id in opentelemetry.test.js and opentelemetry-server.test.js so the mock shape actually guards against a regression to span.parentSpanId. - Fix dead #XXXX changelog links to #3917 and drop the inaccurate **Security** prefix (correctness fix + test infra, no vulnerability).
Address Adam's follow-up on #3917: - Remove the tracePerformance parentSpanContext test: logSpanData is mocked in this file, so it asserted against the mock fixture and couldn't fail on a regression. opentelemetry.test.js guards the fix with the real logSpanData. - Add a comment noting the http.route constraint-stripping regex assumes flat parens, not nested.
vcua-mobify
left a comment
There was a problem hiding this comment.
Reviewed the OTel v2 fix and polyfill changes. The core fixes look correct — parentSpanContext?.spanId is the right OTel v2 replacement for the removed ReadableSpan.parentSpanId, it's applied consistently across both code paths, and the optional chaining correctly yields undefined for root spans. The added logSpanData parentId resolution test is a good regression guard. Two items are holding back a clean approval, plus a couple of questions. Leaving this as a comment (not approving).
1. "Change #3 — Update Node.js Engine Requirements" isn't in this PR. The description says packages/pwa-kit-runtime/package.json engines.node was updated to ^18.19.0 || ^20.6.0 || ^22.0.0 || ^24.0.0, but no pwa-kit-runtime file (and no package.json) appears in the diff — 11 files changed, none under pwa-kit-runtime. develop still has ^18.0.0 || ^20.0.0 || ^22.0.0 || ^24.0.0. Question: was that change meant to be committed here, or should the section be removed from the description?
2. The http.route regex-stripping change in react-rendering.js is undocumented. It's a real change to exported telemetry, but it isn't in the description's Changes list or in either CHANGELOG (see inline note). Question: is it from @adamraya's #3911 review feedback? Please add a CHANGELOG entry and describe it — and since it's unrelated to the OTel-v2/polyfill scope, consider splitting it into its own PR to keep both easy to review.
Remaining notes are minor and inline.
…l guards - react-rendering: type-guard route.path before .replace() so array/RegExp paths (valid matchPath inputs) don't throw TypeError - setup-jest (dev + internal-lib-build): gate TextDecoder/TextEncoder independently so an env with only one still gets the other - CHANGELOG: document the http.route server-span attribute
Change #3 was reverted back as it was suggestive and is a breaking change. I removed it from the PR description as well. |
vcua-mobify
left a comment
There was a problem hiding this comment.
Re-reviewed the two new commits (1ada0a0b, 5283aed3) plus the earlier revert/CHANGELOG commits. All four points from my previous review are resolved:
- ✅ Node.js engines change — dropped from this PR (commit
7fbceff0) and the corresponding section removed from the description. Scope is now consistent with the diff. - ✅
http.routechange documented — added apwa-kit-react-sdkCHANGELOG entry describing the route-template attribute + constraint stripping. - ✅
route.pathtype safety — now guarded withtypeof route.path === 'string' ? … : route.path, with a comment notingmatchPathalso accepts array/RegExp paths. No moreTypeErrorrisk. - ✅ Independent polyfill guards —
TextDecoderandTextEncoderare now guarded separately in bothsetup-jest.jsfiles, so an environment providing only one still gets the other.
The core OTel v2 fix remains correct, no new issues in the delta, and CI is green (lint + all pwa-kit Node matrices pass). The require('util') moving to module scope is fine — it's a core module, always available under the supported Node range.
Only a trivial, non-blocking residual: the description's "Changes" section still lists just #1 and #2 and doesn't mention the http.route work, but the CHANGELOG now covers it, so this is purely cosmetic.
Not approving (leaving to the required reviewers), but from my side this looks good to go.
vcua-mobify
left a comment
There was a problem hiding this comment.
Approving. All feedback from my earlier reviews is resolved — engines change dropped to match scope, http.route work documented in the CHANGELOG, route.path guarded for non-string paths, and independent TextDecoder/TextEncoder polyfill guards. The core OTel v2 fix (parentSpanContext?.spanId) is correct and consistently applied, no new issues in the delta, and CI is green. Thanks for the quick turnaround, @shethj.
Summary
Addresses post-merge review comments from @adamraya on PR #3911.
Changes
1. Fix OTel v2 Breaking Change (parentSpanId → parentSpanContext)
Issue: OpenTelemetry v2 renamed
ReadableSpan.parentSpanId(string) toparentSpanContext(SpanContext). After the #3911 upgrade, every exported span gotparentId: undefinedand lost its parent link.Fix:
src/utils/opentelemetry.js:49to usespan.parentSpanContext?.spanIdsrc/ssr/server/mrt-console-span-exporter.js:28to usespan.parentSpanContext?.spanIdparentSpanContext: {spanId: 'xxx'}Impact: Restores parent span links in exported spans for both server_timing (
logSpanData) and distributed tracing (MrtConsoleSpanExporter) code paths.2. Complete CBOR Polyfills
Issue: AWS SDK 3.x's @smithy/core CBOR encoding needs both
TextDecoderANDTextEncoder. We only polyfilled TextDecoder. If any test hits the encode path, it fails withTextEncoder is not defined.Fix:
TextEncoderalongside existingTextDecoderin:packages/internal-lib-build/configs/jest/setup-jest.jspackages/pwa-kit-dev/src/configs/jest/setup-jest.jsTesting
Related