feat(utils): Refactor `addInstrumentationHandler` to dedicated methods #9542

mydea · 2023-11-13T12:06:14Z

This is mostly an internal utility, but changes a bit. The previous implementation for adding instrumentation had a few issues:

It was impossible to tree shake. So even if some instrumentation is never used, the whole code for it is always included. Splitting this up opens up future improvements there as well.
It was not type safe, and actually there were some subtle issues in the code because of this - things could fail in unexpected ways, as we have been doing a lot of type casting from/to any etc.

This PR splits up addInstrumentationHandler into e.g. addFetchInstrumentationHandler etc. methods, which improves the actual type safety a lot, because currently we've been duplicating the types everywhere, sometimes in slightly differing ways, leading to potential issues. We had a bunch of issues with xhr & fetch data in Replay recently, so I figured it would finally be a good time to clean this up so we can ensure that the data we get is consistent & reliable. Hopefully this change will prevent issues like we have/had in replay from popping up in the future.

Some additional notes:

For the dom instrumentation, I picked the name addClickKeypressInstrumentationHandler(). I'm open for other naming ideas, but I found addDomInstrumentation a bit too broad, as this could mean a lot of things - and we are strictly only instrumenting clicks and keypresses here. Another name (if we want to be a bit broader) could be e.g. addDomEventInstrumentation or something like this?
I updated the type for the XHR data handler to make the method & url required. This was previously optional (but not in some of the code, where we expected this to always be), plus we also did not handle the case that url can be a URL instead of a string, which we now convert.
On the XHR data, we have one field args that 1. is not really used and 2. was actually "buggy", as we always returned the args, but for open and send the args are very different things (but we pretended it's always the "open" args). Now, this is actually always the method & url, and I also deprecated this for v8.
On the XHR data, we also had an unused/unset data field, which I removed (this was not set anywhere)
All old code should still work normally, but I deprecated the generic addInstrumentationHandler() method, for removal in v8.
I also took the opportunity and split the instrumentations into dedicated files, previously this was one mega-file which made it harder to reason about things.

mydea · 2023-11-13T12:07:09Z

packages/browser/src/integrations/breadcrumbs.ts

 let from: string | undefined = handlerData.from;
 let to: string | undefined = handlerData.to;
 const parsedLoc = parseUrl(WINDOW.location.href);
- let parsedFrom = parseUrl(from);
+ let parsedFrom = from ? parseUrl(from) : undefined;


this was actually incorrect because from can technically be undefined.

mydea · 2023-11-13T12:07:38Z

packages/browser/src/sdk.ts

 // Don't create an additional session for the initial route or if the location did not change
- if (!(from === undefined || from === to)) {
+ if (from !== undefined && from !== to) {


this condition is a bit easier to read IMHO

mydea · 2023-11-13T12:08:45Z

packages/replay/src/coreHandlers/handleDom.ts

 // Ignore clicks if ctrl/alt/meta/shift keys are held down as they alter behavior of clicks (e.g. open in new tab)
 if (
 isClick &&
 replay.clickDetector &&
 event &&
+ event.target &&


it was technically possible to have no target here (which was surfaced by a test with changed setup) if you have a syntethic event like new Event('click').

mydea · 2023-11-13T12:10:22Z

packages/types/src/instrument.ts

 export interface SentryWrappedXMLHttpRequest {
- __sentry_xhr_v2__?: SentryXhrData;
+ __sentry_xhr_v3__?: SentryXhrData;


bumped this because method & url are required now.

mydea · 2023-11-13T12:11:09Z

packages/utils/src/instrument/dom.ts

+ * Use at your own risk, this might break without changelog notice, only used internally.
+ * @hidden
+ */
+export function addClickKeypressInstrumentationHandler(handler: (data: HandlerDataDom) => void): void {


this name is pretty explicit, alternatively we could do addDomEventInstrumentationHandler?

mydea · 2023-11-13T12:12:15Z

packages/utils/src/instrument/history.ts

+ WINDOW.onpopstate = function (this: WindowEventHandlers, ...args: any[]): any {
+ const to = WINDOW.location.href;
+ // keep track of the current URL state, as we always receive only the updated state
+ const from = lastHref;


due to this, from can actually be undefined the first time this is run.

mydea · 2023-11-13T12:12:55Z

packages/utils/src/instrument/xhr.ts

+ const method = isString(args[0]) ? args[0].toUpperCase() : undefined;
+ const url = parseUrl(args[1]);
+
+ if (!method || !url) {


this should never happen, but in the spirit of being very cautious...

mydea · 2023-11-13T12:14:30Z

packages/utils/src/instrument/xhr.ts

+
+ const xhrInfo = this[SENTRY_XHR_DATA_KEY];
+
+ if (xhrInfo && isString(header) && isString(value)) {


Being explicit in checking the types here, as theoretically a user could do xhr.setRequestHeader(1, 1) (that may/should fail, but shouldn't fail on our end).

mydea · 2023-11-13T12:15:00Z

packages/utils/src/instrument/xhr.ts

+ }
+
+ const handlerData: HandlerDataXhr = {
+ args: [sentryXhrData.method, sentryXhrData.url],


we used to just pass args: args as [string, string] here which is actually "wrong" or not really what is expected (I think), because args in here is the args passed to send, which is the body (that we capture separately above).

github-actions · 2023-11-13T12:15:03Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	65.99 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	56.17 KB (+0.27% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.15 KB (+0.4% 🔺)
@sentry/browser - Webpack (gzipped)	21.4 KB (+0.35% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	62.53 KB (+0.28% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	29.37 KB (+0.78% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	21.47 KB (+0.92% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	197.17 KB (+0.28% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	88.98 KB (+0.62% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	63.96 KB (+0.89% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	32.02 KB (+0.5% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	66.38 KB (+0.23% 🔺)
@sentry/react - Webpack (gzipped)	21.45 KB (+0.37% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	83.13 KB (+0.17% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	48.27 KB (+0.23% 🔺)
@sentry-internal/feedback - Webpack (gzipped)	16.19 KB (+0.55% 🔺)

mydea · 2023-11-13T12:15:33Z

packages/utils/src/instrument/xhr.ts

+ // but since URL is not available in IE11, we do not check for it,
+ // but simply assume it is an URL and return `toString()` from it (which returns the full URL)
+ // If that fails, we just return undefined
+ return (url as URL).toString();


We did not handle this case at all, but it is a valid usage of open() according to MDN.

billyvg

These changes seem reasonable to me, I only really looked through the replay changes but they seem pretty straightforward

AbhiPrasad

good refactor, feels way better to work with

mydea · 2023-11-22T09:33:33Z

packages/browser/src/integrations/globalhandlers.ts


- if (shouldIgnoreOnError() || (error && error.__sentry_own_request__)) {


Note here: __sentry_own_request__ is not actually set anymore anywhere except on the XHR object itself. So I figured we can remove this check here and above for the global errors, as no error can/should ever get this property anymore 🤔

This is mostly an internal utility, but changes a bit. This improves the actual type safety a lot, because currently we've been duplicating the types everywhere, sometimes in slightly differing ways, leading to potential issues.

This was introduced in #9542 😬 Ideally we could have a lint rule for this, but probably this is rather hard to do - so reminder to self to keep an eye out for this in the future! Closes #9747

mydea requested review from lforst, Lms24 and AbhiPrasad November 13, 2023 12:06

mydea self-assigned this Nov 13, 2023

mydea requested a review from billyvg November 13, 2023 12:06

mydea commented Nov 13, 2023

View reviewed changes

billyvg reviewed Nov 15, 2023

View reviewed changes

AbhiPrasad approved these changes Nov 20, 2023

View reviewed changes

mydea force-pushed the fn/instrument branch from 273a335 to bcbf941 Compare November 22, 2023 09:25

mydea commented Nov 22, 2023

View reviewed changes

mydea added 7 commits November 22, 2023 14:38

fix unhandled rejection for null

d53401b

avoid test

1071982

fixes

6b2f328

fixes

3f07f3c

fix test

279671c

fix

8f368db

mydea force-pushed the fn/instrument branch from 4e903d4 to 8f368db Compare November 22, 2023 13:50

mydea added 2 commits November 22, 2023 15:59

fix

be2f51e

fix for tests

7cc1856

mydea merged commit 2787643 into develop Nov 22, 2023
87 checks passed

mydea deleted the fn/instrument branch November 22, 2023 16:48

This was referenced Dec 7, 2023

Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. #9747

Closed

fix(utils): Fix XHR instrumentation early return #9770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utils): Refactor `addInstrumentationHandler` to dedicated methods #9542

feat(utils): Refactor `addInstrumentationHandler` to dedicated methods #9542

mydea commented Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023

mydea Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading

mydea Nov 13, 2023

billyvg left a comment

AbhiPrasad left a comment

mydea Nov 22, 2023


		const xhrInfo = this[SENTRY_XHR_DATA_KEY];

		if (xhrInfo && isString(header) && isString(value)) {


		if (shouldIgnoreOnError() \|\| (error && error.__sentry_own_request__)) {

feat(utils): Refactor addInstrumentationHandler to dedicated methods #9542

feat(utils): Refactor addInstrumentationHandler to dedicated methods #9542

Conversation

mydea commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2023 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

billyvg left a comment

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(utils): Refactor `addInstrumentationHandler` to dedicated methods #9542

feat(utils): Refactor `addInstrumentationHandler` to dedicated methods #9542

mydea Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023 •

edited

Loading