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

Sentry.Handlers.requestHandler migration steps required? #5282

Closed
3 tasks done
simllll opened this issue Jun 20, 2022 · 2 comments · Fixed by #5287
Closed
3 tasks done

Sentry.Handlers.requestHandler migration steps required? #5282

simllll opened this issue Jun 20, 2022 · 2 comments · Fixed by #5287
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@simllll
Copy link

simllll commented Jun 20, 2022

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which package are you using?

@sentry/node

SDK Version

7.2.0

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

According to https://docs.sentry.io/platforms/node/guides/express/ and other docs,
we have this in our code base:

app.use(
	Sentry.Handlers.requestHandler({
		user: ['_id', 'source'],
		request: true,
		serverName: true,
		version: true
	})
);

but since the latest refactorings (d69cafc) this does not compile anymore, we get the following error:

TS2345: Argument of type '{ user: string[]; request: boolean; serverName: boolean; version: boolean; }' is not assignable to parameter of type 'RequestHandlerOptions'.   Object literal may only specify known properties, and 'user' does not exist in type 'RequestHandlerOptions'.

I looked into RequestHandlerOptions and found there a new child called "include":
image
Should I move user + request inside of include, and if yes, what happened to serverName and version, where can I specify them now?

Expected Result

Either a mirgation guide or it should work as before ;)

Actual Result

latest 7.2.0 is not compilable with config parameters for requestHandler

@lobsterkatie
Copy link
Member

Shoot. You're totally right - apologies. I thought I'd handled backwards compatibility but I missed this bit of it. Will have a fix out shortly.

@simllll
Copy link
Author

simllll commented Jun 21, 2022

Thanks for the PR, just out of curiosity, is there a "new"/better way to provide the flags? As I've seen you check for "include" property, if it's there you use the new one, otherwise the old handler. does that mean there is a "new"/preferred way of passing the params. if so, is there any migration guide to find out how to set these four parameters: (user, request, serverName and version). I guess for user and request it is user -> include.user and request -> include.request, but not sure about serverName and version?

AbhiPrasad pushed a commit that referenced this issue Jun 22, 2022
#5287)

This fixes an issue introduced by #5257, which was designed to be  backwards-compatible, but which missed a key use case: When users pass options to our Express middleware, they might be (and in fact, for now, almost certainly are) passing them in the form of `ParseRequestOptions`, not `AddRequestDataToEventOptions`. This allows for that possibility, as a backwards-compatibility measure until v8. It also adjusts a spot in the serverless SDK where a similar problem can occur.

Fixes #5282.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants