feat(node): Ensure request bodies are reliably captured for http requests #13746
Conversation
size-limit report 📦
|
❌ 6 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
| // This is non-standard, but may be set on e.g. Next.js or Express requests | ||
| const cookies = (req as PolymorphicRequest).cookies; | ||
|
|
||
| const normalizedRequest: Request = { |
There was a problem hiding this comment.
l/m: I have been burned in the past because Request is a native type. I suggest we rename this to smth like NormalizedRequest or ProcessingMetadataRequest.
There was a problem hiding this comment.
this type already exists, it is not new 😬 but I do agree, but we probably need to alias it then and keep the old name around (deprecated)...?
| @@ -148,7 +149,27 @@ export const instrumentHttp = Object.assign( | |||
| const isolationScope = (scopes.isolationScope || getIsolationScope()).clone(); | |||
| const scope = scopes.scope || getCurrentScope(); | |||
|
|
|||
| const headers = req.headers; | |||
| const host = headers.host || '<no host>'; | |||
There was a problem hiding this comment.
l: I haven't put too much thought into this but is <no host> a good default?
There was a problem hiding this comment.
yeah no idea, just kept this around ^^
| @@ -101,7 +101,7 @@ export type ProfileItem = BaseEnvelopeItem<ProfileItemHeaders, Profile>; | |||
| export type ProfileChunkItem = BaseEnvelopeItem<ProfileChunkItemHeaders, ProfileChunk>; | |||
| export type SpanItem = BaseEnvelopeItem<SpanItemHeaders, Partial<SpanJSON>>; | |||
|
|
|||
| export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: DynamicSamplingContext }; | |||
| export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: Partial<DynamicSamplingContext> }; | |||
There was a problem hiding this comment.
sorry, forgot to mention this here: this was just not correctly annotated before, really. Since we did not type dynamicSamplingContext on the sdkProcessingMetadata before, this worked by accident. but because this is really Partial<DSC> (based on what we set on it, which is also partial), it makes this necessary to be correct.
We could also choose to leave this and cast it in the place where we set this, but really that means lying to ourselves because we don't know it is complete. 🤷 no strong feelings though, if we feel this is a bad type change I can also cast it.
| const { sdkProcessingMetadata = {} } = event; | ||
| const req = sdkProcessingMetadata.request; | ||
| const { request, normalizedRequest } = sdkProcessingMetadata; | ||
|
|
||
| if (!req) { | ||
| return event; | ||
| const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); | ||
|
|
||
| // If this is set, it takes precedence over the plain request object | ||
| if (normalizedRequest) { | ||
| // Some other data is not available in standard HTTP requests, but can sometimes be augmented by e.g. Express or Next.js | ||
| const ipAddress = request ? request.ip || (request.socket && request.socket.remoteAddress) : undefined; | ||
| const user = request ? request.user : undefined; | ||
|
|
||
| return addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions); | ||
| } | ||
|
|
||
| const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); | ||
| if (!request) { | ||
| return event; | ||
| } | ||
|
|
||
| return addRequestDataToEvent(event, req, addRequestDataOptions); | ||
| return addRequestDataToEvent(event, request, addRequestDataOptions); |
There was a problem hiding this comment.
I would mayyyybee do it like this?:
| const { sdkProcessingMetadata = {} } = event; | |
| const req = sdkProcessingMetadata.request; | |
| const { request, normalizedRequest } = sdkProcessingMetadata; | |
| if (!req) { | |
| return event; | |
| const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); | |
| // If this is set, it takes precedence over the plain request object | |
| if (normalizedRequest) { | |
| // Some other data is not available in standard HTTP requests, but can sometimes be augmented by e.g. Express or Next.js | |
| const ipAddress = request ? request.ip || (request.socket && request.socket.remoteAddress) : undefined; | |
| const user = request ? request.user : undefined; | |
| return addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions); | |
| } | |
| const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); | |
| if (!request) { | |
| return event; | |
| } | |
| return addRequestDataToEvent(event, req, addRequestDataOptions); | |
| return addRequestDataToEvent(event, request, addRequestDataOptions); | |
| const { sdkProcessingMetadata = {} } = event; | |
| const { request, normalizedRequest } = sdkProcessingMetadata; | |
| const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); | |
| if (request) { | |
| addRequestDataToEvent(event, request, addRequestDataOptions); | |
| } | |
| // If this is set, it takes precedence over the plain request object | |
| if (normalizedRequest) { | |
| addNormalizedRequestDataToEvent(event, normalizedRequest, addRequestDataOptions); | |
| } | |
| return event; |
So that
- We always use all of the data.
- For now we can just use the old logic from
addRequestDataToEventto extract the "non-standard" fields on the request. Removing the need for theadditionalDataarg which I admittedly find a bit offputting.
There was a problem hiding this comment.
(Sorry, took some time to come back to this 😅 )
I guess my main problem with this is that we do all the work of parsing the "old" request etc. even though we don't need it? I do feel like it would be easier to understand to have a single function be responsible for it - otherwise, we end up in the state of "why is this field set here? let's look through all of the old code to find if it sets it somewhere, then look over the new code, ...".
By not calling the old method anymore it makes it easier to follow the "new, happy" path imho 🤔 though I do agree that the additionalData part is not great...
|
🥹 |
mydea
force-pushed
the
fn/node-request-data
branch
from
183be5b
to
a5821ce
Compare
This PR started out as trying to fix capturing request bodies for Koa.
Some investigation later, we found out that the fundamental problem was that we relied on the request body being on
request.body, which is non-standard and thus does not necessarily works. It seems that in express this works because it under the hood writes the body there, but this is non-standard and rather undefined behavior. For other frameworks (e.g. Koa and probably more) this did not work, the body was not on the request and thus never captured. We also had no test coverage for this overall.This PR ended up doing a few things:
sdkProcessingMetadata- this used to beany, which lead to any usage of this not really being typed at all. I added proper types for this now.Most importantly, I opted to not force this into the existing, rather complicated and hard to follow request data integration flow. This used to take an IsomorphicRequest and then did a bunch of conversion etc.
Since now in Node, we always have the same, proper http request (for any framework, because this always goes through http instrumentation), we can actually streamline this and normalize this properly at the time where we set this.
So with this PR, we set a
normalizedRequestwhich already has the url, headers etc. set in a way that we need it/it makes sense.Additionally, the parsed & stringified request body will be set on this too.
If this normalized request is set in sdkProcessingMetadata, we will use it as source of truth instead of the plain
request. (Note that we still need the plain request for some auxiliary data that is non-standard, e.g.request.user).For the body parsing itself, we monkey-patch
req.on('data'). this way, we ensure to not add more handlers than a user has, and we only extract the body if the user is extracting it anyhow, ensuring we do not alter behavior.Closes #13722