-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from 2 commits
33fa050
426ce94
c0c9ffe
977dd1c
9aaa360
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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', | ||
[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', | ||
}, | ||
}), | ||
]), | ||
); | ||
}); |
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): { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: do we still need this function? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L: is there a reason why one is set to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah nevermind, I read up on the |
||
} |
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} |
There was a problem hiding this comment.
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 callingstartSpan
. 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 usesauto.function.sveltekit
forwrapServerLoadWithSentry
.There was a problem hiding this comment.
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.