fix(instrumentation-fetch, instrumentation-xml-http-request)!: remove unreachable CORS Preflight span - #7025
Open
nabeelamjadsheikh wants to merge 1 commit into
Conversation
… unreachable CORS Preflight span
Both browser instrumentations contained an `_addChildSpan` helper that
emitted a `CORS Preflight` child span whenever `getResource()` reported a
`corsPreFlightRequest`.
Under Resource Timing Level 2 the preflight request is folded into the
main request's `PerformanceResourceTiming` entry instead of being
surfaced as a separate one, so `performance.getEntriesByType('resource')`
never yields the extra entry the split relies on. The branch is therefore
unreachable in any browser implementing the current spec, and no `CORS
Preflight` span has been produced in practice.
Removes the dead helper and its call site from both instrumentations,
along with the XHR tests that asserted on the span. `getResource()` in
`@opentelemetry/sdk-trace-web` still returns `corsPreFlightRequest`; that
field is part of the stable package's public API and is left untouched.
Refs: open-telemetry#5122
Signed-off-by: nabeelamjadsheikh <131901574+nabeelamjadsheikh@users.noreply.github.com>
Pull request dashboard statusWaiting on reviewers · refreshed 2026-09-02 16:40 UTC Review the latest changes. Status above doesn't look right?
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7025 +/- ##
==========================================
+ Coverage 94.98% 95.02% +0.03%
==========================================
Files 409 409
Lines 14335 14318 -17
Branches 3276 3274 -2
==========================================
- Hits 13616 13605 -11
+ Misses 719 713 -6
🚀 New features to boost your workflow:
|
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
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.
Fixes #5122
What
Removes the
CORS Preflightspan from@opentelemetry/instrumentation-fetchand@opentelemetry/instrumentation-xml-http-request.Both instrumentations carried an
_addChildSpanhelper that started aCORS Preflightchild span whenevergetResource()came back with acorsPreFlightRequest:That branch cannot be taken.
getResource()derivescorsPreFlightRequestby splitting twoPerformanceResourceTimingentries inside the span's time window, but under Resource Timing Level 2 a preflight is not reported as its own entry — it is folded into the main request's entry, alongside redirects and authentication challenges.Prior art
This is the follow-up @pichlermarc invited when closing #5130 ("please open a new PR if this is incorrect"). Context from that thread:
mswfor fetch instrumentation tests #5282 handled the test side but "The refactor to remove the runtime code (that does nothing, as you pointed out), is still needed, as I didn't want to touch the implementation while doing a big test refactor."So the runtime removal was never actually landed — #5130 was closed on the understanding it had been, when only the fetch tests had.
Verification
The premise was asserted from the spec in the original thread but never demonstrated, so I measured it. Two local origins, an endpoint that requires a preflight (custom request header), and both round trips deliberately delayed 300 ms so a separate entry would be trivially visible if one existed:
fetchinitiator:{ "initiatorType": "fetch", "fetchStart": 43.7, "requestStart": 347.6, "responseStart": 649.6, "responseEnd": 649.9, "duration": 606.2 }xmlhttprequestinitiator:{ "initiatorType": "xmlhttprequest", "fetchStart": 40.4, "requestStart": 345.1, "responseStart": 647.4, "responseEnd": 647.7, "duration": 607.3 }One entry in both cases, and the ~304 ms gap between
fetchStartandrequestStartis the preflight — absorbed into the single entry rather than reported beside it, withdurationspanning both round trips. There is no second entry for the split to find, and no way to recover the preflight's timing from what is exposed.This also matches the note already sitting in the fetch test suite, which points at this same issue:
https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts#L951-L961
Changes
instrumentation-fetch: drop_addChildSpanand its call site.instrumentation-xml-http-request: drop_addChildSpanand its call site, plus the now-unusedPerformanceTimingNames as PTNimport.xhr.test.ts: delete the 10 preflight assertions and re-index theexportSpy.args[...]lookups that were offset by the preflight span being exported first. (These tests fabricate two mock resource entries, which is why they exercised a shape browsers do not emit.)fetch.test.tsneeds no changes — [instrumentation-fetch] Usemswfor fetch instrumentation tests #5282 already removed its preflight assertions.Test results:
instrumentation-xml-http-request:Executed 124 of 136 (skipped 12) SUCCESS— the 12 skips are the pre-existing sync-mode abort/timeout cases.instrumentation-fetch:Executed 103 of 103 SUCCESS— unchanged.lintandprettier --checkclean for both packages.Scope — narrower than #5130, deliberately
#5130 also stripped
corsPreFlightRequestandfindMainRequest()out of@opentelemetry/sdk-trace-web. I've left that package untouched here, becausePerformanceResourceTimingInfois public API of a stable package and removing a field from it can't land outside a major. Keeping it also means the main-request selection behaviour is unchanged — the main span still takes its network events frommainRequestexactly as before, so this PR is purely a removal of spans that were never emitted.Happy to follow up with a deprecation on
corsPreFlightRequestto start that clock, if maintainers want it.One behavioural note for reviewers
Alongside the span, this also drops
this._markResourceAsUsed(corsPreFlightRequest). If a split ever did occur, the preflight entry would no longer be marked and could be matched against a later span. The measurement above says that can't happen, but I'd rather flag it than bury it — if you'd prefer the marking kept defensively, say the word and I'll restore it.