Uh oh!
There was an error while loading. Please reload this page.
feat(node): Add maxIncomingRequestBodySize - #16225
Conversation
560976f to
7a21af7Comparesize-limit report 📦
|
| * | ||
| * @default 'medium' | ||
| */ | ||
| maxRequestBodySize?: 'none' | 'small' | 'medium' | 'always'; |
There was a problem hiding this comment.
should we rename this to maxIncomingRequestBodySize? It is not clear right now that this only applies to incoming requests, not outgoing ones 😅
There was a problem hiding this comment.
I got the name from the general Sentry docs: https://develop.sentry.dev/sdk/expected-features/#attaching-request-body-in-server-sdks
To be uniform with other SDKs, it would be beneficial to keep the name, but I agree that it would be clearer with incoming. At least, the information is available in the JSDoc 😅
There was a problem hiding this comment.
we don't need to have the same name, if we think something else is better :) maybe you could also check with other sdk folks, if this also applies to outgoing requests in other implementations or just to incoming?
| function patchRequestToCaptureBody( | ||
| req: http.IncomingMessage, | ||
| isolationScope: Scope, | ||
| maxRequestBodySize: 'none' | 'small' | 'medium' | 'always', |
There was a problem hiding this comment.
l: We could actually type this to omit none as allowed value, and then avoid the further check in here, as we actually guard agains this already before calling this?
| if (event === 'data') { | ||
| DEBUG_BUILD && logger.log(INSTRUMENTATION_NAME, 'Handling request.on("data")'); | ||
| if (DEBUG_BUILD) { | ||
| logger.log(INSTRUMENTATION_NAME, 'Handling request.on("data")'); |
There was a problem hiding this comment.
l: Let's combine these into a single log line, e.g. Handling request.on("data") with maximum request body size ...
maxRequestBodySizemaxIncomingRequestBodySize| * | ||
| * @default 'medium' | ||
| */ | ||
| maxRequestBodySize?: 'none' | 'small' | 'medium' | 'always'; |
There was a problem hiding this comment.
m: Just to confirm - you're still going to change the name here?
There was a problem hiding this comment.
ah thanks - seems like my push of the commit where I changed the naming didn't get through 😄
| await runner.completed(); | ||
| }); | ||
| test('does not capture any request bodies with "none" setting and "ignoreIncomingRequestBody"', async () => { |
There was a problem hiding this comment.
does it make sense to combine these two? should we not combine the filter function with e.g. always setting? 🤔
There was a problem hiding this comment.
I added this test because I was actually running into an issue 😄
I had a logic problem in the if condition and it was sending the request body.
It's basically testing that the if condition is correct: https://github.com/getsentry/sentry-javascript/pull/16225/files#diff-964fee03880b5de6d711130f1ad4111d04e194bc066ea1725b4e6b7c1c722a42R266
…mingRequestBody` (#13698) ## DESCRIBE YOUR PR Documents `ignoreIncomingRequestBody `: getsentry/sentry-javascript#15959 Documents `maxIncomingRequestBodySize`: getsentry/sentry-javascript#16225 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [x] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
…mingRequestBody` (#13698) ## DESCRIBE YOUR PR Documents `ignoreIncomingRequestBody `: getsentry/sentry-javascript#15959 Documents `maxIncomingRequestBodySize`: getsentry/sentry-javascript#16225 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [x] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
Adds
maxIncomingRequestBodySizeto the NodehttpIntegration.The setting controls the maximum size of HTTP request bodies attached to events.
There is the option
maxRequestBodySize(docs) in other SDKs, but to be more specific, this is named withincoming.Available options:
closes#16179