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

SvelteKit SDK creates server transactions with [Method] null name #8199

Closed
3 tasks done
Lms24 opened this issue May 24, 2023 · 4 comments · Fixed by #8201
Closed
3 tasks done

SvelteKit SDK creates server transactions with [Method] null name #8199

Lms24 opened this issue May 24, 2023 · 4 comments · Fixed by #8201

Comments

@Lms24
Copy link
Member

Lms24 commented May 24, 2023

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/sveltekit

SDK Version

7.53.0

Framework Version

SvelteKit 1.x

Link to Sentry event

No response

SDK Setup

Basic SDK setup from wizard

Steps to Reproduce

  1. npm create svelte@latest
  2. npx @sentry/wizard@latest -i sveltekit
  3. curl localhost:PORT/something

Expected Result

Three options here:

  1. The SDK discards transactions that don't have a matching route
  2. The SDK falls back to the raw URL (sets the source accordingly) and the txn shows as <> in Sentry
  3. The SDK falls back to a "unknown route" transaction, sets the source as custom and sends it to Sentry where it will show up like a normal transaction

I'm currently leaning towards option 1 because I don't see value in these transactions and it's the simplest course of action

Actual Result

The SDK sends a transaction with with a name like GET|POST null to Sentry, with source route

This happens because we determine the txn name with event.route.id which evaluates to null.

@Lms24
Copy link
Member Author

Lms24 commented May 24, 2023

This issue was extracted from a conversation in #6692. Here are some important replies:

  1. SvelteKit SDK Roadmap #6692 (comment)

Hi the team 👋
In my dashboard, I have a lot of these: POST null
image
It's probably because I'm using GraphQL.
On the other side, as I want detailed diagnostics on GraphQL, I'm using @envelop/sentry to log more things and it's > working well.
I have the feeling that POST null is redundant with the detailed logs that I have. How to not send them?

Originally posted by @jycouet in #6692 (comment)


  1. SvelteKit SDK Roadmap #6692 (comment)

I'm seeing something similar. In my case, these are mostly 302's or 404's, all server-side. The 404's are a mixed bag of bot spam along with requests for *.js.map files that fire when you open the browser's devtools (I delete sourcemap when before deploying).

image

The transaction name Get Null could be the one set here, but tbh I'm not sure what/if there's anything to be done here. Maybe we can log the pathname to be able to identify them? The 302's in my example are especially difficult to track down so having a pathname associated would help in that regard.

name: `${event.request.method} ${event.route.id}`,

Originally posted by @thenbe in #6692 (comment)

@Lms24
Copy link
Member Author

Lms24 commented May 24, 2023

@thenbe and @jycouet RE GET null:

Thanks for raising this. I believe this is a bug in the SDK because event.route.id can very well evaluate to null, as you showed. I'm leaning towards simply discarding transactions without a route at this point as I believe that whenever a "valid" request is made to the SvelteKit server, route.id should be set. Meaning that whenever it is null, a server request was made with a route unknown to the Server, so chances are that the server would respond with a 404 or a redirect anyway.

Does this match your experience/make sense to you?

@thenbe

Maybe we can log the pathname to be able to identify them?

I don't think we should do this if discarding sounds reasonable. The reason is that we can't parameterize the path here (i.e. determine that /somepath/12312312/test has a parameter and is actually /somepath/:someId/test) and we'd need to send it as a "degraded" transaction to Sentry. This will result in the transaction only showing up under <<unparameterized>> in the UI which isn't desireable either.

@jycouet

I have the feeling that POST null is redundant with the detailed logs that I have

So just to confirm, you get additional transactions from @envelope/sentry to the POST null ones, so it's also safe to discard them in your case?

@jycouet
Copy link

jycouet commented May 24, 2023

Yes, I believe that discarding will have no effect on @envelope/sentry, so All good.
And for sure, in my case, I want to discard them (to not have them 2 times)

But, in general, I'm not sure it's a good idea to see null and discard!
Now thinking about it, they don't have route.id because in my case, it's done in a hook, so it's "dynamic".
If I would do this in/routes/api/graphql/+server.ts then you would see a route.id and everything is good.
Now, when I'm doing it in a handle, hooks.server.ts, somewhere in my code, I say "if URL === "/myroute" then..." and that's where it's getting null. So maybe you should discard and add a way (documentation?) on how to add it manually?

Hope it makes sense ^^

@Lms24
Copy link
Member Author

Lms24 commented May 24, 2023

So maybe you should discard and add a way (documentation?) on how to add it manually?

Yes, I think it should still be possible to opt-in. I opened #8201 which discards transactions for unknown routes by default but makes it possible for users to opt-in. In this case we'll fall back to the raw URL path name which can still be modified by users in beforeSendTransaction.

Lms24 added a commit that referenced this issue May 30, 2023
…tes (#8201)

Adds a `handleUnknownRoutes` option to the `sentryHandle` server
request handler. The option defaults to `false` which will effectively
reduce noise, such as random bot requests. Previously such requests
would yield e.g. `GET null` transactions because they could not be
matched to an existing route by the framework. See #8199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants