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(solidstart): Add server action instrumentation helper #13035

Merged
merged 5 commits into from
Jul 30, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"This is currently not an issue outside of our repo. See: https://github.com/nksaraf/vinxi/issues/177"
],
"preview": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi dev",
"start": "HOST=localhost PORT=3030 NODE_OPTIONS='--import ./src/instrument.server.mjs' vinxi start",
"test:prod": "TEST_ENV=production playwright test",
"test:build": "pnpm install && npx playwright install && pnpm build",
"test:assert": "pnpm test:prod"
Expand All @@ -31,7 +32,7 @@
"jsdom": "^24.0.0",
"solid-js": "1.8.17",
"typescript": "^5.4.5",
"vinxi": "^0.3.12",
"vinxi": "^0.4.0",
"vite": "^5.2.8",
"vite-plugin-solid": "^2.10.2",
"vitest": "^1.5.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Sentry.init({
tunnel: 'http://localhost:3031/', // proxy server
// Performance Monitoring
tracesSampleRate: 1.0, // Capture 100% of the transactions
debug: !!import.meta.env.DEBUG,
});

mount(() => <StartClient />, document.getElementById('app')!);
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
tracesSampleRate: 1.0, // Capture 100% of the transactions
tunnel: 'http://localhost:3031/', // proxy server
debug: !!process.env.DEBUG,
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ export default function Home() {
<li>
<A href="/client-error">Client error</A>
</li>
<li>
<A href="/server-error">Server error</A>
</li>
<li>
<A id="navLink" href="/users/5">
User 5
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { withServerActionInstrumentation } from '@sentry/solidstart';
import { createAsync } from '@solidjs/router';

const getPrefecture = async () => {
'use server';
return await withServerActionInstrumentation('getPrefecture', () => {
throw new Error('Error thrown from Solid Start E2E test app server route');

return { prefecture: 'Kanagawa' };
});
};

export default function ServerErrorPage() {
const data = createAsync(() => getPrefecture());

return <div>Prefecture: {data()?.prefecture}</div>;
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
import { useParams } from '@solidjs/router';
import { withServerActionInstrumentation } from '@sentry/solidstart';
import { createAsync, useParams } from '@solidjs/router';

const getPrefecture = async () => {
'use server';
return await withServerActionInstrumentation('getPrefecture', () => {
return { prefecture: 'Ehime' };
});
};
export default function User() {
const params = useParams();
return <div>User ID: {params.id}</div>;
const userData = createAsync(() => getPrefecture());

return (
<div>
User ID: {params.id}
<br />
Prefecture: {userData()?.prefecture}
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { expect, test } from '@playwright/test';
import { waitForError } from '@sentry-internal/test-utils';

test.describe('server-side errors', () => {
test('captures server action error', async ({ page }) => {
const errorEventPromise = waitForError('solidstart', errorEvent => {
return errorEvent?.exception?.values?.[0]?.value === 'Error thrown from Solid Start E2E test app server route';
});

await page.goto(`/server-error`);

const error = await errorEventPromise;

expect(error.tags).toMatchObject({ runtime: 'node' });
expect(error).toMatchObject({
exception: {
values: [
{
type: 'Error',
value: 'Error thrown from Solid Start E2E test app server route',
mechanism: {
type: 'solidstart',
handled: false,
},
},
],
},
transaction: 'GET /server-error',
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/test-utils';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/core';

test('sends a server action transaction on pageload', async ({ page }) => {
const transactionPromise = waitForTransaction('solidstart', transactionEvent => {
return transactionEvent?.transaction === 'GET /users/6';
});

await page.goto('/users/6');

const transaction = await transactionPromise;

expect(transaction.spans).toEqual(
expect.arrayContaining([
expect.objectContaining({
description: 'getPrefecture',
data: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
Copy link
Member

Choose a reason for hiding this comment

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

l/m: I'm not sure if manual is the correct origin here b/c it's not users calling startSpan. However, the way I understand the usage, users have call the wrapper manually, right? So it's probably fine as long as we don't auto instrument server actions.

Copy link
Member

Choose a reason for hiding this comment

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

nothing we emit from the SDK OOTB should have origin: manual - that's the base rule of thumb here :D We should always assign a proper origin that matches (see https://develop.sentry.dev/sdk/performance/trace-origin/) for any span that any SDK starts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, user's have to manually wrap their server action function bodies with the wrapper.

How about auto.function.solidstart? The sveltekit sdk uses auto.function.sveltekit for wrapServerLoadWithSentry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, please have another look.

[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
},
}),
]),
);
});

test('sends a server action transaction on client navigation', async ({ page }) => {
const transactionPromise = waitForTransaction('solidstart', transactionEvent => {
return transactionEvent?.transaction === 'POST getPrefecture';
});

await page.goto('/');
await page.locator('#navLink').click();
await page.waitForURL('/users/5');

const transaction = await transactionPromise;

expect(transaction.spans).toEqual(
expect.arrayContaining([
expect.objectContaining({
description: 'getPrefecture',
data: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
},
}),
]),
);
});
3 changes: 2 additions & 1 deletion packages/solidstart/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@
"@sentry/solid": "8.20.0",
"@sentry/types": "8.20.0",
"@sentry/utils": "8.20.0",
"@sentry/vite-plugin": "2.19.0"
"@sentry/vite-plugin": "2.19.0",
"@opentelemetry/instrumentation": "^0.52.1"
},
"devDependencies": {
"@solidjs/router": "^0.13.4",
Expand Down
2 changes: 1 addition & 1 deletion packages/solidstart/rollup.npm.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default makeNPMConfigVariants(
// prevent this internal code from ending up in our built package (this doesn't happen automatially because
// the name doesn't match an SDK dependency)
packageSpecificConfig: {
external: ['solid-js', '@sentry/solid', '@sentry/solid/solidrouter'],
external: ['solid-js/web', 'solid-js', '@sentry/solid', '@sentry/solid/solidrouter'],
output: {
dynamicImportInCjs: true,
},
Expand Down
2 changes: 1 addition & 1 deletion packages/solidstart/src/index.types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// We export everything from both the client part of the SDK and from the server part.
// Some of the exports collide, which is not allowed, unless we redifine the colliding
// Some of the exports collide, which is not allowed, unless we redefine the colliding
// exports in this file - which we do below.
export * from './client';
export * from './server';
Expand Down
2 changes: 2 additions & 0 deletions packages/solidstart/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,5 @@ export { withSentryErrorBoundary } from '@sentry/solid';
// -------------------------
// Solid Start SDK exports:
export { init } from './sdk';

export * from './withServerActionInstrumentation';
50 changes: 50 additions & 0 deletions packages/solidstart/src/server/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { flush } from '@sentry/node';
import { logger } from '@sentry/utils';
import type { RequestEvent } from 'solid-js/web';
import { DEBUG_BUILD } from '../common/debug-build';

/** Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda ends */
export async function flushIfServerless(): Promise<void> {
const isServerless = !!process.env.LAMBDA_TASK_ROOT || !!process.env.VERCEL;

if (isServerless) {
try {
DEBUG_BUILD && logger.log('Flushing events...');
await flush(2000);
DEBUG_BUILD && logger.log('Done flushing events');
} catch (e) {
DEBUG_BUILD && logger.log('Error while flushing events:\n', e);
}
}
}

/**
* Takes a request event and extracts traceparent and DSC data
* from the `sentry-trace` and `baggage` DSC headers.
*/
export function getTracePropagationData(event: RequestEvent | undefined): {
Copy link
Member

Choose a reason for hiding this comment

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

l: do we still need this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good catch! I had it removed and the re-added after trying some things out. Forgot to remove it again :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

sentryTrace: string | undefined;
baggage: string | null;
} {
const request = event && event.request;
const headers = request && request.headers;
const sentryTrace = (headers && headers.get('sentry-trace')) || undefined;
const baggage = (headers && headers.get('baggage')) || null;
Copy link
Member

Choose a reason for hiding this comment

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

L: is there a reason why one is set to null and one to undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure tbh, the types were already like this. I think I could change it both to null but no strong feelings.


return { sentryTrace, baggage };
}

/**
* Determines if a thrown "error" is a redirect Response which Solid Start users can throw to redirect to another route.
* see: https://docs.solidjs.com/solid-router/reference/data-apis/response-helpers#redirect
* @param error the potential redirect error
*/
export function isRedirect(error: unknown): boolean {
if (error == null || !(error instanceof Response)) {
return false;
}

const hasValidLocation = typeof error.headers.get('location') === 'string';
const hasValidStatus = error.status >= 300 && error.status <= 308;
return hasValidLocation && hasValidStatus;
Comment on lines +30 to +32
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity/no action required: the status code makes total sense to me. Why is thelocation header important?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose people might set some weird status codes on errors that aren't redirects and we wouldn't want to ignore those? Not sure tbh, no strong feelings on removing this check.

Copy link
Member

Choose a reason for hiding this comment

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

Ah nevermind, I read up on the location header. Makes sense to me!

}
57 changes: 57 additions & 0 deletions packages/solidstart/src/server/withServerActionInstrumentation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { SPAN_STATUS_ERROR, handleCallbackErrors } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, captureException, getActiveSpan, spanToJSON, startSpan } from '@sentry/node';
import { flushIfServerless, isRedirect } from './utils';

/**
* Wraps a server action (functions that use the 'use server' directive)
* function body with Sentry Error and Performance instrumentation.
*/
export async function withServerActionInstrumentation<A extends (...args: unknown[]) => unknown>(
serverActionName: string,
Copy link
Member

Choose a reason for hiding this comment

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

Is there some way to infer the name, possibly? That would be ideal... if not, this is fine for now!

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think so. Accessing a parent's function name isn't available in strict mode, also it wouldn't work for minifed code either. Unfortunately I think we have to live with this :(

callback: A,
): Promise<ReturnType<A>> {
const activeSpan = getActiveSpan();

if (activeSpan) {
const spanData = spanToJSON(activeSpan).data;

// In solid start, server function calls are made to `/_server` which doesn't tell us
// a lot. We rewrite the span's route to be that of the sever action name but only
// if the target is `/_server`, otherwise we'd overwrite pageloads on routes that use
// server actions (which are more meaningful, e.g. a request to `GET /users/5` is more
// meaningful than overwriting it with `GET doSomeFunctionCall`).
if (spanData && !spanData['http.route'] && spanData['http.target'] === '/_server') {
activeSpan.setAttribute('http.route', serverActionName);
activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
}
}

try {
return await startSpan(
{
op: 'function.server_action',
name: serverActionName,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
Copy link
Member

Choose a reason for hiding this comment

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

is this a route though? Users could specify anything here, right? I guess component makes more sense?
I tried looking up the transaction name source values but only found this orphaned page. To me the component definition makes sense but feel free to overrule me :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Judging from that page, component also makes the most sense to me. I initially had it on url but changed it to route after @mydea looked over it.

wdyt about using component instead @mydea?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, component makes sense to me too! Sorry about the misdirection to use route 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

},
},
async span => {
const result = await handleCallbackErrors(callback, error => {
if (!isRedirect(error)) {
span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
captureException(error, {
mechanism: {
handled: false,
type: 'solidstart',
},
});
}
});

return result;
},
);
} finally {
await flushIfServerless();
}
}
Loading
Loading