feat(solidjs): Add solid router instrumentation #12263

andreiborza · 2024-05-28T15:49:59Z

I'd like to get some feedback on the general approach here. Unit and E2E tests would follow either in a follow up PR or after getting the approach validated here.

I've tested this in a sample app locally and it works on my machine(tm).

To use this integration:

import * as Sentry from "@sentry/solidjs";
import {Route, Router, useBeforeLeave, useLocation} from "@solidjs/router"

Sentry.init({
    dsn: <dsn>
    integrations: [
        Sentry.solidRouterBrowserTracingIntegration({ useBeforeLeave, useLocation }),
    ],
    tracesSampleRate: 1.0, //  Capture 100% of the transactions
    debug: true,
});

const SentryRouter = Sentry.withSentryRouterRouting(Router)

render(() => (
   <SentryRouter>
        // ... routes here
        // <Route .../>
   </SentryRouter>
), root!);

AbhiPrasad

Could you update the PR description with how this is meant to be set up?

AbhiPrasad · 2024-05-28T15:57:16Z

packages/solidjs/package.json

@@ -54,6 +55,9 @@
    "solid-js": "1.8.11",
    "vite-plugin-solid": "^2.8.2"
  },
+  "optionalDependencies": {
+    "@solidjs/router": "0.13.3"


m: should this be a more liberal range? Actually now that I think about it, should we also expand the range for solid-js?

Yeah, I'd do ^1.8.0 for solid-js. For the router it's harder because 🤷 damn 0.x versions 😬 at the very least we should also do ^0.13.3 but this means that for every minor release they do (e.g. 0.14.0) we'll have to update the SDK accordingly to match this 😬

From the docs for the router:

The docs are based on Solid Router v0.10.x. To use this version, you need to have Solid v1.8.4 or later installed.

Should I downgrade to that and see if it still runs?

That puts us at 1.8.4 as minimum for solid-js. We can probably support a lower version if we have some kind of warnings inside solidrouter.ts if it's used with a version lower than 1.8.4. Is that possible?

IMHO it's fine to say this is ^1.8.4 too, this is greenfield so let's start with support that makes it reasonably easy for us! :)

Updated to ^1.8.4.

AbhiPrasad · 2024-05-28T15:58:07Z

packages/solidjs/src/solidrouter.ts

+import { createComponent } from 'solid-js/web';
+import { DEBUG_BUILD } from './debug-build';
+
+const CLIENTS_WITH_INSTRUMENT_NAVIGATION: Client[] = [];


This should be a WeakMap/WeakSet so we don't need to iterate the array, and we can GC the client if need be.

andreiborza · 2024-05-28T16:04:58Z

Could you update the PR description with how this is meant to be set up?

Added

AbhiPrasad · 2024-05-28T16:10:54Z

packages/solidjs/src/solidrouter.ts

+      _useBeforeLeave = useBeforeLeave;
+      _useLocation = useLocation;


Can we directly import and use useLocation and useBeforeLeave from solidjs/router instead?

I hope we don't have to await import it to make the optionalDependencies work properly 😬

No we can't. I spent the entire day trying to figure this out with the help of @lforst - but basically the compiler detects that these hooks are used outside of the router and the invariant that detects this is baked into the bundle.

The router context is different :(

Dang that sucks :(

Yea, was hoping it would work. Maybe we can talk to the maintainer to find some workaround.

mydea · 2024-05-29T07:28:25Z

packages/solidjs/rollup.npm.config.mjs

+export default makeNPMConfigVariants(
+  makeBaseNPMConfig({
+    packageSpecificConfig: {
+      external: ['solid-js', 'solid-js/web', '@solidjs/router'],


hmm, is this needed? shouldn't this be automatically marked as external? 🤔

I think it might not be needed. I didn't have solid-js in there before and everything built and worked fine. Will try to remove.

Confirmed, don't need. Removed it.

I am pretty confident solid-js/web is still needed because it is not added to external because it is not inside any dependencies.

It's not in the build output, I double-checked again.

mydea · 2024-05-29T07:29:07Z

packages/solidjs/package.json

@@ -54,6 +55,9 @@
    "solid-js": "1.8.11",
    "vite-plugin-solid": "^2.8.2"
  },
+  "optionalDependencies": {


Is optionalDependencies the best thing for this? What about making this an optional peer dependency? (not sure how well supported etc. this is though...)

Because I guess optionalDependencies means this will generally always be installed along by the SDK, right...?

I also realized this. I thought optionalDependencies would be just like a "soft" peer dep but package managers will actually try to install these. We should simply document the version requirements instead in JS Doc and in the actual docs.

Optional peer dependencies are a npm 7.x thing. Not sure if that's too high for us.

As for optionalDependencies - I've only tried with a tarball, but solid router was not pulled in if the user didn't already have it and there were no warnings about requiring it either. Or am I misunderstanding your question?

edit: nvm I'm a liar. It did pull in solid router as well.

I mean maybe it is fine to have it as optionalDependencies, just feels like a non-ideal solution 😬 I guess this is why in react we do not depend on react-router at all 🤔

We only use types from solid router, I could vendor these in, wdyt?

yeah, I think that's probably the best solution here! We also do that in other places (it has it's own set of problems obviously, but I think this is the best trade off 🤔 )

Please only add dependencies as devDependencies. Everything else only has downsides and no upsides (that I am aware of).

I vendored the types in and removed the dep on solid router.

mydea · 2024-05-29T07:47:16Z

packages/solidjs/src/solidrouter.ts

+    afterAllSetup(client) {
+      integration.afterAllSetup(client);
+
+      const initPathName = WINDOW && WINDOW.location && WINDOW.location.pathname;


m: Do we actually need this? Is this different from just letting the base browserTracingIntegration create the pageload span? If not, we can just pass instrumentPageLoad through (instead of forcing this to false) and only handle the navigation span in a custom way here?

Is it okay that the origin won't be auto.pageload.solidjs.solidrouter in that case? Or should I be getting and updating the pageload span?

yeah, this is fine, this is really just for ourselves :)

github-actions · 2024-05-29T12:23:51Z

size-limit report 📦

Path	Size
@sentry/browser	21.74 KB (0%)
@sentry/browser (incl. Tracing)	32.73 KB (0%)
@sentry/browser (incl. Tracing, Replay)	68.3 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.62 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas)	72.36 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback)	84.44 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	86.29 KB (0%)
@sentry/browser (incl. metrics)	25.91 KB (+0.01% 🔺)
@sentry/browser (incl. Feedback)	37.86 KB (0%)
@sentry/browser (incl. sendFeedback)	26.32 KB (0%)
@sentry/browser (incl. FeedbackAsync)	30.83 KB (0%)
@sentry/react	24.51 KB (0%)
@sentry/react (incl. Tracing)	35.78 KB (0%)
@sentry/vue	25.72 KB (+0.01% 🔺)
@sentry/vue (incl. Tracing)	34.57 KB (+0.01% 🔺)
@sentry/svelte	21.87 KB (-0.01% 🔽)
CDN Bundle	23.11 KB (0%)
CDN Bundle (incl. Tracing)	34.46 KB (0%)
CDN Bundle (incl. Tracing, Replay)	68.39 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback)	73.53 KB (0%)
CDN Bundle - uncompressed	68 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed	102.1 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed	212 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	224.37 KB (0%)
@sentry/nextjs (client)	35.12 KB (0%)
@sentry/sveltekit (client)	33.36 KB (0%)
@sentry/node	115.24 KB (0%)
@sentry/aws-serverless	103.73 KB (0%)

…cingIntegration's

andreiborza requested review from mydea, lforst, Lms24 and AbhiPrasad May 28, 2024 15:49

andreiborza mentioned this pull request May 28, 2024

Sentry SDK for SolidJS #12142

Closed

AbhiPrasad reviewed May 28, 2024

View reviewed changes

mydea reviewed May 29, 2024

View reviewed changes

andreiborza mentioned this pull request May 29, 2024

ref(react): Use WeakSet to keep track of navigation instrumented clients #12273

Merged

andreiborza force-pushed the ab/solidjs-routing-instrumentation branch 2 times, most recently from d50c649 to 04d2dd3 Compare May 29, 2024 12:13

andreiborza force-pushed the ab/solidjs-routing-instrumentation branch from f7f60ce to c008bac Compare May 29, 2024 12:55

mydea approved these changes May 29, 2024

View reviewed changes

andreiborza force-pushed the ab/solidjs-routing-instrumentation branch from 956b46d to 9354905 Compare May 29, 2024 15:08

andreiborza added 6 commits May 29, 2024 23:34

feat(solidjs): Add solid router instrumentation

79a77bf

feat(solidjs): Remove unnecessary external deps in rollup config

752b618

feat(solidjs): Use WeakSet to check for instrumented clients

88fad51

feat(solidjs): Drop solid router dep and vendor in types instead

523d9d5

feat(solidjs): Update README with routing instrumentation instructions

c326cec

feat(solidjs): Drop custom pageload navigation in favor of browserTra…

5b74d49

…cingIntegration's

andreiborza force-pushed the ab/solidjs-routing-instrumentation branch from 9354905 to 5b74d49 Compare May 29, 2024 21:35

andreiborza merged commit 396112c into develop May 30, 2024
108 checks passed

andreiborza deleted the ab/solidjs-routing-instrumentation branch May 30, 2024 19:25

andreiborza added the Package: solid Issues related to the Sentry Solid SDK label May 30, 2024

		_useBeforeLeave = useBeforeLeave;
		_useLocation = useLocation;

Uh oh!

feat(solidjs): Add solid router instrumentation #12263

feat(solidjs): Add solid router instrumentation #12263

Uh oh!

Conversation

andreiborza commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhiPrasad 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!

andreiborza commented May 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreiborza May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

size-limit report 📦

Uh oh!

Uh oh!

Uh oh!

andreiborza commented May 28, 2024 •

edited

Loading

andreiborza May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading