Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sveltekit): Auto-wrap load functions with proxy module #7994

Merged
merged 4 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/sveltekit/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
"@sentry/types": "7.49.0",
"@sentry/utils": "7.49.0",
"@sentry/vite-plugin": "^0.6.0",
"magic-string": "^0.30.0",
"sorcery": "0.11.0"
},
"devDependencies": {
Expand Down
8 changes: 8 additions & 0 deletions packages/sveltekit/src/server/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type { DynamicSamplingContext, StackFrame, TraceparentData } from '@sentr
import { baggageHeaderToDynamicSamplingContext, basename, extractTraceparentData } from '@sentry/utils';
import type { RequestEvent } from '@sveltejs/kit';

import { WRAPPED_MODULE_SUFFIX } from '../vite/autoInstrument';

/**
* Takes a request event and extracts traceparent and DSC data
* from the `sentry-trace` and `baggage` DSC headers.
Expand Down Expand Up @@ -52,5 +54,11 @@ export function rewriteFramesIteratee(frame: StackFrame): StackFrame {

delete frame.module;

// In dev-mode, the WRAPPED_MODULE_SUFFIX is still present in the frame's file name.
// We need to remove it to make sure that the frame's filename matches the actual file
if (frame.filename.endsWith(WRAPPED_MODULE_SUFFIX)) {
frame.filename = frame.filename.slice(0, -WRAPPED_MODULE_SUFFIX.length);
}

return frame;
}
123 changes: 123 additions & 0 deletions packages/sveltekit/src/vite/autoInstrument.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
import * as fs from 'fs';
import * as path from 'path';
import type { Plugin } from 'vite';

export const WRAPPED_MODULE_SUFFIX = '?sentry-auto-wrap';

export type AutoInstrumentSelection = {
/**
* If this flag is `true`, the Sentry plugins will automatically instrument the `load` function of
* your universal `load` functions declared in your `+page.(js|ts)` and `+layout.(js|ts)` files.
*
* @default true
*/
load?: boolean;

/**
* If this flag is `true`, the Sentry plugins will automatically instrument the `load` function of
* your server-only `load` functions declared in your `+page.server.(js|ts)`
* and `+layout.server.(js|ts)` files.
*
* @default true
*/
serverLoad?: boolean;
};

type AutoInstrumentPluginOptions = AutoInstrumentSelection & {
debug: boolean;
};

/**
* Creates a Vite plugin that automatically instruments the parts of the app
* specified in @param options. This includes
*
* - universal `load` functions from `+page.(js|ts)` and `+layout.(js|ts)` files
* - server-only `load` functions from `+page.server.(js|ts)` and `+layout.server.(js|ts)` files
*
* @returns the plugin
*/
export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Promise<Plugin> {
Lms24 marked this conversation as resolved.
Show resolved Hide resolved
const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options;

return {
name: 'sentry-auto-instrumentation',
// This plugin needs to run as early as possible, before the SvelteKit plugin virtualizes all paths and ids
enforce: 'pre',

async load(id) {
const applyUniversalLoadWrapper =
shouldWrapLoad &&
/^\+(page|layout)\.(js|ts|mjs|mts)$/.test(path.basename(id)) &&
(await canWrapLoad(id, debug));

if (applyUniversalLoadWrapper) {
// eslint-disable-next-line no-console
debug && console.log(`Wrapping ${id} with Sentry load wrapper`);
return getWrapperCode('wrapLoadWithSentry', `${id}${WRAPPED_MODULE_SUFFIX}`);
}

const applyServerLoadWrapper =
shouldWrapServerLoad &&
/^\+(page|layout)\.server\.(js|ts|mjs|mts)$/.test(path.basename(id)) &&
(await canWrapLoad(id, debug));

if (applyServerLoadWrapper) {
// eslint-disable-next-line no-console
debug && console.log(`Wrapping ${id} with Sentry load wrapper`);
return getWrapperCode('wrapServerLoadWithSentry', `${id}${WRAPPED_MODULE_SUFFIX}`);
}

return null;
},
};
}

/**
* We only want to apply our wrapper to files that
*
* - Have no Sentry code yet in them. This is to avoid double-wrapping or interferance with custom
* Sentry calls.
* - Actually declare a `load` function. The second check of course is not 100% accurate, but it's good enough.
* Injecting our wrapper into files that don't declare a `load` function would result in a build-time warning
* because vite/rollup warns if we reference an export from the user's file in our wrapping code that
* doesn't exist.
*
* Exported for testing
*
* @returns `true` if we can wrap the given file, `false` otherwise
*/
export async function canWrapLoad(id: string, debug: boolean): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very curious how this will work out. I think this is a very sane approach 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah me too, I guess we'll learn it soon enough 😅

const code = (await fs.promises.readFile(id, 'utf8')).toString();

const codeWithoutComments = code.replace(/(\/\/.*| ?\/\*[^]*?\*\/)(,?)$/gm, '');

const hasSentryContent = codeWithoutComments.includes('@sentry/sveltekit');
if (hasSentryContent) {
// eslint-disable-next-line no-console
debug && console.log(`Skipping wrapping ${id} because it already contains Sentry code`);
}

const hasLoadDeclaration = /(const|let|var|function)\s+load\s*(=|\()/gm.test(codeWithoutComments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: What about the case where the function declaration is not load? I think it's fine, but maybe we leave a note in the docs about it (or even add a check).

const someThingElse = () => {

};

export {
  load: someThingElse
}

Copy link
Member Author

@Lms24 Lms24 May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, too but I think this isn't valid syntax: https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export#syntax (🤞 )

The closest thing to this example would be

const load = () => {};

export {
  load
}

which should be covered by the regex

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah sorry missed something in the snippet - you gotta use as for esm, but this still applies if I'm thinking about this correctly.

export {
  someThingElse as load
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh damnit, I missed this case. I think we could adjust the regex to test for /as\s+load/ but I guess this has the potential to interfere with TypeScript's type assertions. However, in the worst case, this would produce a build warning so I think it's fine.

if (!hasLoadDeclaration) {
// eslint-disable-next-line no-console
debug && console.log(`Skipping wrapping ${id} because it doesn't declare a \`load\` function`);
}

return !hasSentryContent && hasLoadDeclaration;
}

/**
* Return wrapper code fo the given module id and wrapping function
*/
function getWrapperCode(
wrapperFunction: 'wrapLoadWithSentry' | 'wrapServerLoadWithSentry',
idWithSuffix: string,
): string {
return (
`import { ${wrapperFunction} } from "@sentry/sveltekit";` +
`import * as userModule from ${JSON.stringify(idWithSuffix)};` +
`export const load = userModule.load ? ${wrapperFunction}(userModule.load) : undefined;` +
`export * from ${JSON.stringify(idWithSuffix)};`
);
}
Comment on lines +113 to +123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I really like that we just do template strings for this instead of typescript templates. Makes this so much simpler.

41 changes: 37 additions & 4 deletions packages/sveltekit/src/vite/sentryVitePlugins.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import type { SentryVitePluginOptions } from '@sentry/vite-plugin';
import type { Plugin } from 'vite';

import type { AutoInstrumentSelection } from './autoInstrument';
import { makeAutoInstrumentationPlugin } from './autoInstrument';
import { makeCustomSentryVitePlugin } from './sourceMaps';

type SourceMapsUploadOptions = {
/**
* If this flag is `true`, the Sentry plugins will automatically upload source maps to Sentry.
* Defaults to `true`.
* @default true`.
*/
autoUploadSourceMaps?: boolean;

Expand All @@ -17,16 +19,32 @@ type SourceMapsUploadOptions = {
sourceMapsUploadOptions?: Partial<SentryVitePluginOptions>;
};

type AutoInstrumentOptions = {
/**
* The Sentry plugin will automatically instrument certain parts of your SvelteKit application at build time.
* Set this option to `false` to disable this behavior or what is instrumentated by passing an object.
*
* Auto instrumentation includes:
* - Universal `load` functions in `+page.(js|ts)` files
* - Server-only `load` functions in `+page.server.(js|ts)` files
*
* @default true (meaning, the plugin will instrument all of the above)
*/
autoInstrument?: boolean | AutoInstrumentSelection;
};

export type SentrySvelteKitPluginOptions = {
/**
* If this flag is `true`, the Sentry plugins will log some useful debug information.
* Defaults to `false`.
* @default false.
*/
debug?: boolean;
} & SourceMapsUploadOptions;
} & SourceMapsUploadOptions &
AutoInstrumentOptions;

const DEFAULT_PLUGIN_OPTIONS: SentrySvelteKitPluginOptions = {
autoUploadSourceMaps: true,
autoInstrument: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to be now but we should think about adding an escape hatch for not auto instrumenting particular routes. I've seen users in nexts that found this useful (eg some people didn't want to instrument their auth routes).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. As discussed, I'll do this in a follow-up PR but I agree that it's also a great way of giving people a quick way to opt out of auto instrumenting in case they get an error.

debug: false,
};

Expand All @@ -43,7 +61,22 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {}
...options,
};

const sentryPlugins = [];
const sentryPlugins: Plugin[] = [];

if (mergedOptions.autoInstrument) {
const pluginOptions: AutoInstrumentSelection = {
load: true,
serverLoad: true,
...(typeof mergedOptions.autoInstrument === 'object' ? mergedOptions.autoInstrument : {}),
};

sentryPlugins.push(
await makeAutoInstrumentationPlugin({
...pluginOptions,
debug: options.debug || false,
}),
);
}

if (mergedOptions.autoUploadSourceMaps) {
const pluginOptions = {
Expand Down
21 changes: 18 additions & 3 deletions packages/sveltekit/src/vite/sourceMaps.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getSentryRelease } from '@sentry/node';
import { uuid4 } from '@sentry/utils';
import { escapeStringForRegex, uuid4 } from '@sentry/utils';
import type { SentryVitePluginOptions } from '@sentry/vite-plugin';
import { sentryVitePlugin } from '@sentry/vite-plugin';
import * as child_process from 'child_process';
Expand All @@ -10,6 +10,7 @@ import * as path from 'path';
import * as sorcery from 'sorcery';
import type { Plugin } from 'vite';

import { WRAPPED_MODULE_SUFFIX } from './autoInstrument';
import { getAdapterOutputDir, loadSvelteConfig } from './svelteConfig';

// sorcery has no types, so these are some basic type definitions:
Expand Down Expand Up @@ -74,9 +75,9 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio
let isSSRBuild = true;

const customPlugin: Plugin = {
name: 'sentry-vite-plugin-custom',
name: 'sentry-upload-source-maps',
apply: 'build', // only apply this plugin at build time
enforce: 'post',
enforce: 'post', // this needs to be set to post, otherwise we don't pick up the output from the SvelteKit adapter

// These hooks are copied from the original Sentry Vite plugin.
// They're mostly responsible for options parsing and release injection.
Expand Down Expand Up @@ -117,6 +118,8 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio
}

const outDir = path.resolve(process.cwd(), outputDir);
// eslint-disable-next-line no-console
debug && console.log('[Source Maps Plugin] Looking up source maps in', outDir);

const jsFiles = getFiles(outDir).filter(file => file.endsWith('.js'));
// eslint-disable-next-line no-console
Expand Down Expand Up @@ -145,6 +148,18 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio
console.error('[Source Maps Plugin] error while flattening', file, e);
}
}

// We need to remove the query string from the source map files that our auto-instrument plugin added
// to proxy the load functions during building.
const mapFile = `${file}.map`;
if (fs.existsSync(mapFile)) {
const mapContent = (await fs.promises.readFile(mapFile, 'utf-8')).toString();
const cleanedMapContent = mapContent.replace(
new RegExp(escapeStringForRegex(WRAPPED_MODULE_SUFFIX), 'gm'),
'',
);
await fs.promises.writeFile(mapFile, cleanedMapContent);
}
});

try {
Expand Down
Loading