feat(remix): Add custom `browserTracingIntegration` by onurtemizkan · Pull Request #10442 · getsentry/sentry-javascript

onurtemizkan · 2024-01-31T15:23:33Z

Adds a custom browserTracingIntegration for @sentry/remix.

This also marks remixRouterInstrumentation as deprecated.

It's still functional, and the logic checks the existence of globally defined _customStartTransaction to decide how to behave.

Added another integration test application for now using Remix v2 and the new browserTracingIntegration until we switch to it completely.

mydea · 2024-01-31T15:49:16Z

packages/remix/src/client/browserTracingIntegration.ts

+  return {
+    ...browserTracingIntegrationInstance,
+    afterAllSetup(client) {
+      browserTracingIntegrationInstance.afterAllSetup?.(client);


on latest (we changed recently) you should not need ?. as this should be properly typed!

mydea · 2024-01-31T15:49:58Z

packages/remix/src/client/browserTracingIntegration.ts

+  if (options.startTransactionOnPageLoad === undefined) {
+    options.startTransactionOnPageLoad = true;
+  }
+
+  if (options.startTransactionOnLocationChange === undefined) {
+    options.startTransactionOnLocationChange = true;
+  }


do we need this? Instead we should check instrumentPageLoad and instrumentNavigation here!

mydea · 2024-01-31T15:50:13Z

packages/remix/src/client/browserTracingIntegration.ts

+    useEffect: options.useEffect,
+    useLocation: options.useLocation,
+    useMatches: options.useMatches,
+    startTransactionOnLocationChange: options.startTransactionOnLocationChange,


use options.instrumentNavigation here instead?

mydea · 2024-01-31T15:50:32Z

packages/remix/src/client/browserTracingIntegration.ts

+    afterAllSetup(client) {
+      browserTracingIntegrationInstance.afterAllSetup?.(client);
+
+      if (options.startTransactionOnPageLoad) {


use options.instrumentPageLoad here instead

mydea · 2024-01-31T15:50:58Z

packages/remix/src/client/performance.tsx

+  startTransactionOnPageLoad?: boolean;
+  startTransactionOnLocationChange?: boolean;


we don't need this here and should instead rely on the new instrumentXXX options :)

mydea · 2024-01-31T15:51:59Z

packages/remix/src/client/performance.tsx

+  'routing.instrumentation': 'remix-router',
+} as const;
+
+let activeRootSpan: Span | undefined;


do we actually need the active root span here? 🤔 ideally we can get rid of this global/module state...!

github-actions · 2024-02-01T11:52:34Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	78.2 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	69.43 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped)	73.36 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	63.04 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	33.38 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped)	33.25 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	31.33 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped)	31.34 KB (0%)
@sentry/browser - Webpack (gzipped)	22.6 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	76.17 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	67.72 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	33.5 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped)	24.66 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	213.68 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	101.49 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	74.01 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	36.63 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	69.8 KB (0%)
@sentry/react - Webpack (gzipped)	22.63 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	86.56 KB (0%)
@sentry/nextjs Client - Webpack (gzipped)	50.8 KB (0%)
@sentry-internal/feedback - Webpack (gzipped)	17.21 KB (0%)

onurtemizkan · 2024-02-01T15:57:06Z

@mydea, thanks for the review! Pushed the updates. PR is ready for another pass.

mydea · 2024-02-01T16:10:20Z

packages/remix/src/index.server.ts

+// eslint-disable-next-line deprecation/deprecation
 export { remixRouterInstrumentation, withSentry } from './client/performance';
 export { captureRemixErrorBoundaryError } from './client/errors';
+export { browserTracingIntegration } from './client/browserTracingIntegration';


This should be exported from client, not server, I guess? 😅

actually it is exported from client, but we should not export it here.

We're sadly forced to export all client-side exports from the server-side as well, we previously had issues with unresolved imports and/or broken typings when we didn't.

cc @Lms24 and/or @lforst you've done something like this for nextjs/sveltekit, can you point in the right direction here?

Yeah we (unfortunately) export this on both sides, also in SvelteKit (and I believe NextJS). Kinda unfortunate but this is a limitation as long as we don't use sub-path/conditional package exports (or whatever they're called now).

I wonder if it would not be better then to export the shim integrations as we do for the bundles - we already have that code for the bundles anyhow, and it's def. safer in regards to avoiding accidentally including browser-specific stuff..? that also takes care of warning if it is used etc..?

We've had these discussions before and current status quo was to be mindful of where users are add the integration. Fwiw, the only common use case would be replay anyway. Only Remix requires users to additionally manually add BrowserTracing (something we should change long term btw).

related

Attempted import error: 'Replay' is not exported from '@sentry/nextjs' (imported as 'Sentry'). #9796

"captureRemixServerException" is not exported by "node_modules/@sentry/remix/esm/index.client.js" #9594

I guess shimming is not the end of the world but the problem is that we need to figure out where we draw the line.

Also, i don't think shimming has high prio and we might solve this with the package exports anyway.

👍 ok, so the outcome is we export browser tracing & replay in both client and server?

Lms24

Just had a couple of questions but looks good generally!

Lms24 · 2024-02-02T15:32:44Z

packages/remix/package.json

+    "test:integration:tracingIntegration": "export TRACING_INTEGRATION=true && run-s test:integration:v2",
    "test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server",
-    "test:integration:prepare": "(cd test/integration && yarn)",
+    "test:integration:prepare": "(cd test/integration && yarn install --force)",


q: why do we need the --force flag?

Nope, we don't need. It's a leftover from when I'd been yarn linking / unlinking a lot to test this. Removing.

Lms24 · 2024-02-02T15:38:53Z

packages/remix/src/client/performance.tsx

+  useEffect,
+  useLocation,
+  useMatches,
+  startTransactionOnLocationChange,


l: Can we rename this to instrumentNavigation to align more with the new API? (not sure if we'd break something but looks like this is a new function)

Lms24 · 2024-02-02T15:39:22Z

packages/remix/src/client/performance.tsx

+  useLocation,
+  useMatches,
+  startTransactionOnLocationChange,
+  customStartTransaction,


I guess this is still there for compatibility with the old routing instrumentation?

Yes, we can remove this while removing the old instrumentation completely.

Lms24 · 2024-02-02T15:42:55Z

packages/remix/src/client/browserTracingIntegration.ts

+  setGlobals({
+    useEffect: options.useEffect,
+    useLocation: options.useLocation,
+    useMatches: options.useMatches,
+    startTransactionOnLocationChange: options.instrumentNavigation,
+  });


To confirm: We can't get rid of setting the hooks globally because we can't really pass them into withSentry?

I believe we can pass them into withSentry. We can try switching to that.

cc @AbhiPrasad / @mydea wdyt would this be a good change?

My PoV:

cleaner as we don't need to set anything globally

but requires more changes for users when migrating (also more wizard changes for us).

I remembered why, this instrumentation is very similar to what we have for react-router 6. So it was for consistency.

sentry-javascript/packages/react/src/reactrouterv6.tsx

Lines 44 to 80 in cc0fcb8

export function reactRouterV6Instrumentation(

useEffect: UseEffect,

useLocation: UseLocation,

useNavigationType: UseNavigationType,

createRoutesFromChildren: CreateRoutesFromChildren,

matchRoutes: MatchRoutes,

stripBasename?: boolean,

) {

return (

customStartTransaction: (context: TransactionContext) => Transaction | undefined,

startTransactionOnPageLoad = true,

startTransactionOnLocationChange = true,

): void => {

const initPathName = WINDOW && WINDOW.location && WINDOW.location.pathname;

if (startTransactionOnPageLoad && initPathName) {

activeTransaction = customStartTransaction({

name: initPathName,

op: 'pageload',

origin: 'auto.pageload.react.reactrouterv6',

tags: SENTRY_TAGS,

metadata: {

source: 'url',

},

});

}

_useEffect = useEffect;

_useLocation = useLocation;

_useNavigationType = useNavigationType;

_matchRoutes = matchRoutes;

_createRoutesFromChildren = createRoutesFromChildren;

_stripBasename = stripBasename || false;

_customStartTransaction = customStartTransaction;

_startTransactionOnLocationChange = startTransactionOnLocationChange;

};

}

ahh I see, good point. apart from getting rid of the globals there's not really much value from a user perspective. So let's leave it as is.

Lms24

Thanks!

onurtemizkan requested a review from mydea January 31, 2024 15:23

onurtemizkan force-pushed the onur/remix-browserTracingIntegration branch from fd6503a to 69ea641 Compare January 31, 2024 15:23

onurtemizkan changed the title ~~feat(remix): Add remixBrowserTracingIntegration~~ feat(remix): Add browserTracingIntegration Jan 31, 2024

mydea reviewed Jan 31, 2024

View reviewed changes

onurtemizkan force-pushed the onur/remix-browserTracingIntegration branch from 76b1b23 to cd65704 Compare February 1, 2024 11:43

onurtemizkan changed the title ~~feat(remix): Add browserTracingIntegration~~ feat(remix): Add custom browserTracingIntegration Feb 1, 2024

onurtemizkan force-pushed the onur/remix-browserTracingIntegration branch from 3ab138f to cf07f41 Compare February 1, 2024 12:31

onurtemizkan marked this pull request as ready for review February 1, 2024 15:57

onurtemizkan force-pushed the onur/remix-browserTracingIntegration branch from cf07f41 to 8889d6f Compare February 1, 2024 16:01

mydea reviewed Feb 1, 2024

View reviewed changes

Lms24 mentioned this pull request Feb 2, 2024

[v8] Replace custom BrowserTracing routing instrumentations #10387

Closed

Lms24 reviewed Feb 2, 2024

View reviewed changes

onurtemizkan added 7 commits February 2, 2024 16:17

feat(remix): Add remixBrowserTracingIntegration

244acf1

Rename integration to browserTracingIntegration

c2a2a43

Add deprecation flag to remixRouterInstrumentation

8a3311c

Address review comments.

e47fcdc

Fix lint

695db9c

Update CI workflow

6b2fb30

Address review comments.

18683ec

onurtemizkan force-pushed the onur/remix-browserTracingIntegration branch from 1d4be11 to 18683ec Compare February 2, 2024 16:17

Lms24 approved these changes Feb 2, 2024

View reviewed changes

Address review comments.

b028a6e

mydea approved these changes Feb 5, 2024

View reviewed changes

mydea merged commit baf5f90 into develop Feb 5, 2024

mydea deleted the onur/remix-browserTracingIntegration branch February 5, 2024 14:39

		startTransactionOnPageLoad?: boolean;
		startTransactionOnLocationChange?: boolean;

	export function reactRouterV6Instrumentation(
	useEffect: UseEffect,
	useLocation: UseLocation,
	useNavigationType: UseNavigationType,
	createRoutesFromChildren: CreateRoutesFromChildren,
	matchRoutes: MatchRoutes,
	stripBasename?: boolean,
	) {
	return (
	customStartTransaction: (context: TransactionContext) => Transaction \| undefined,
	startTransactionOnPageLoad = true,
	startTransactionOnLocationChange = true,
	): void => {
	const initPathName = WINDOW && WINDOW.location && WINDOW.location.pathname;
	if (startTransactionOnPageLoad && initPathName) {
	activeTransaction = customStartTransaction({
	name: initPathName,
	op: 'pageload',
	origin: 'auto.pageload.react.reactrouterv6',
	tags: SENTRY_TAGS,
	metadata: {
	source: 'url',
	},
	});
	}

	_useEffect = useEffect;
	_useLocation = useLocation;
	_useNavigationType = useNavigationType;
	_matchRoutes = matchRoutes;
	_createRoutesFromChildren = createRoutesFromChildren;
	_stripBasename = stripBasename \|\| false;

	_customStartTransaction = customStartTransaction;
	_startTransactionOnLocationChange = startTransactionOnLocationChange;
	};
	}

Uh oh!

Conversation

onurtemizkan commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

onurtemizkan commented Feb 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

onurtemizkan commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading