Conflict with New Relic SDK causing expensive DOM I/O for "fetch"

[DrewML]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases (This is not true, but I don't see anything in the latest releases that would have addressed this issue)

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/react

SDK Version

6.9.18

Framework Version

17.0.2

Link to Sentry event

No response

Steps to Reproduce

1. Load the New Relic SDK, or any other library that monkey-patches the global window.fetch with a new function
2. Load the Sentry SDK
3. Run the browser profiler, and observe the time spent by BrowserBackend.prototype._setupTransport

Expected Result

No style recalculations in the DOM

Actual Result

This testing was done on a Moto G Power using the latest version of Chrome.

It looks like the wrapped copy of fetch fails the isNativeFetch check, which leads to the slow path with the iframe injection. The isNativeFetch function looks to have been added originally to avoid the cost of iframe injection

[AbhiPrasad]

This is tricky, because we require the I/O to make sure we are grabbing onto the correct instance of fetch. Important to note we heavily recommend initializing Sentry as soon as possible (before any other application code/instrumentation).

One thing we can probably do is make the transport initialization more lazy - but we probably will not backport these changes to v6, so you'll have to upgrade to v7 to take advantage of them. Do you have any ideas on your end?

For now, you can avoid this by using the XHR transport we offer, which should prevent the fetch functionality.

// v6
Sentry.init({
  transport: XHRTransport,
});

// v7
Sentry.init({
  transport: makeXHRTransport,
});

[DrewML]

@AbhiPrasad Thanks for the wicked fast reply! Y'all are great 😄

Important to note we heavily recommend initializing Sentry as soon as possible (before any other application code/instrumentation).

Definitely noted! The challenge here is that most observability vendors have a recommendation that they run first to capture intrinsics like fetch/setTimeout/etc. In a perfect world our app would only be using one observability vendor, but unfortunately not our reality currently 😂

but we probably will not backport these changes to v6, so you'll have to upgrade to v7 to take advantage of them

That's very reasonable - definitely don't expect y'all to backport any work. We recently attempted to upgrade to the 7.x releases, but there were some things breaking with one of our 3rd party vendors' code. We're still working through this, but understand we'd need to move to 7.x for any fixes to the fetch issue.

For now, you can avoid this by using the XHR transport we offer, which should prevent the fetch functionality.

We'll give this a shot! If the native XMLHTTPRequest object is needed, though, we may run into the same issue with the New Relic SDK already monkey-patching it.

Do you have any ideas on your end?

Can you help me understand the technical requirement that the Sentry SDK has access to the native fetch, vs a wrapped copy? Just trying to understand if the wrapping here is to capture fetch requests made by client code, or if Sentry wants the native copy so it can use it as a transport to Sentry servers without other libs getting in the way.

[AbhiPrasad]

I'm going to make some benchmarks to sanity check the DOM I/O, but generally that change should be good to go.

Can you help me understand the technical requirement that the Sentry SDK has access to the native fetch, vs a wrapped copy

We do this because of:

sentry-javascript/packages/browser/src/transports/utils.ts

Lines 10 to 38 in bfe819d

	* A special usecase for incorrectly wrapped Fetch APIs in conjunction with ad-blockers.
	* Whenever someone wraps the Fetch API and returns the wrong promise chain,
	* this chain becomes orphaned and there is no possible way to capture it's rejections
	* other than allowing it bubble up to this very handler. eg.
	*
	* const f = window.fetch;
	* window.fetch = function () {
	* const p = f.apply(this, arguments);
	*
	* p.then(function() {
	* console.log('hi.');
	* });
	*
	* return p;
	* }
	*
	* `p.then(function () { ... })` is producing a completely separate promise chain,
	* however, what's returned is `p` - the result of original `fetch` call.
	*
	* This mean, that whenever we use the Fetch API to send our own requests, _and_
	* some ad-blocker blocks it, this orphaned chain will _always_ reject,
	* effectively causing another event to be captured.
	* This makes a whole process become an infinite loop, which we need to somehow
	* deal with, and break it in one way or another.
	*
	* To deal with this issue, we are making sure that we _always_ use the real
	* browser Fetch API, instead of relying on what `window.fetch` exposes.
	* The only downside to this would be missing our own requests as breadcrumbs,
	* but because we are already not doing this, it should be just fine.

We'll give this a shot! If the native XMLHTTPRequest object is needed, though, we may run into the same issue with the New Relic SDK already monkey-patching it.

This should be fine afaik, but please let us know if there are issues, we can take a look!

but there were some things breaking with one of our 3rd party vendors' code

If (when?) you ever get around to the v7 upgrade again, please feel free to open a GitHub issue with the error you're encountering, we are more than happy to help y'all debug through it!

[DrewML]

@AbhiPrasad this is super helpful context, thank you! The reasoning makes sense to me.

We've got a ticket to switch over to the XHR transport, and I'll report back if that addresses the issue (assume it will tho).

Final question: Is it worth Sentry considering making the XHR transport the default? I can imagine we're not the only ones with multiple APM vendor SDKs competing to grab intrinsics

[AbhiPrasad]

Is it worth Sentry considering making the XHR transport the default

We thought thought about the opposite actually - #5927. Having the fetch keepalive option means we can make sure more data gets to Sentry, so we want it to be the default transport.

[ithinkdancan]

I was able to confirm that switching to the XHR transport did prevent the blocking IO for the transport setup, but it appears that the blocking is still happening in the instrumentFetch method that calls into supports.ts. Any recommendations here?

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀