Uh oh!
There was an error while loading. Please reload this page.
feat(solidstart): Add server action instrumentation helper - #13035
Conversation
| const sentryTrace = (headers && headers.get('sentry-trace')) || undefined; | ||
| const baggage = (headers && headers.get('baggage')) || null; |
There was a problem hiding this comment.
L: is there a reason why one is set to null and one to undefined?
There was a problem hiding this comment.
I'm not sure tbh, the types were already like this. I think I could change it both to null but no strong feelings.
Lms24
left a comment
There was a problem hiding this comment.
LGTM! We could think about extracting the getTracePropagationData and flushIfServerless out to utils because we have them in at least 3 SDKs each. But for now it's also fine to live with the duplication.
| const hasValidLocation = typeof error.headers.get('location') === 'string'; | ||
| const hasValidStatus = error.status >= 300 && error.status <= 308; | ||
| return hasValidLocation && hasValidStatus; |
There was a problem hiding this comment.
out of curiosity/no action required: the status code makes total sense to me. Why is thelocation header important?
There was a problem hiding this comment.
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.
Ah nevermind, I read up on the location header. Makes sense to me!
chargome
left a comment
There was a problem hiding this comment.
LGTM 🚀
Maybe we should also check if this works as expected on vercel/edge
281463d to
426ce94Compareandreiborza
commented
Jul 29, 2024
I rewrote this since the previous push to not use Additionally, I only rewrite the transaction name when the target isn't |
| * 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.
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.
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 :(
| description: 'getPrefecture', | ||
| data: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.server_action', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Updated, please have another look.
| * 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.
Ah, good catch! I had it removed and the re-added after trying some things out. Forgot to remove it again :)
| op: 'function.server_action', | ||
| name: serverActionName, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yeah, component makes sense to me too! Sorry about the misdirection to use route 😅
size-limit report 📦
|
Can be used like this:
Shows up like this:
Can also be used for api routes like this:
Shows up like this for pageloads on a route that makes a fetch call to an api route
