-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(node): Use RequestData
integration in express handlers
#5990
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
ref(node): Use RequestData
integration in express handlers
#5990
Conversation
09dcbd4
to
d56ed1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this merges in, can we update #5194 about the possible v8 changes we need here?
packages/node/test/handlers.test.ts
Outdated
const origPushScope = hub.pushScope; | ||
hub.pushScope = function (this: Hub) { | ||
scope = origPushScope.call(this); | ||
return scope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: Why can't we just monkeypatch withScope
? This will break if withScope
stops uses pushScope
in the future, which it might.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That (wrapping withScope
instead) was my first thought, actually, but in order to do so I would have to copy its code rather than wrap it, because the scope in question isn't returned:
// the real version
public withScope(callback: (scope: Scope) => void): void {
const scope = this.pushScope();
try {
callback(scope);
} finally {
this.popScope();
}
}
So I'd have to do
// monkeypatched version
hub.withScope = function(this:Hub, callback: (scope: Scope) => void): void {
scope = this.pushScope();
try {
callback(scope);
} finally {
this.popScope();
}
}
which, to be fair, isn't that much more code, but a) is at least a little uglier than the pushScope
monkeypatching, and more importantly, b) doesn't save us from having to fix the test if the implementation of withScope
changes. Given that, monkeypatching pushScope
seemed a better option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then can we try something like so?
// eslint-disable-next-line @typescript-eslint/unbound-method
const oldCaptureException = hub.captureException;
hub.captureException = function (this: Hub, ...args: unknown[]) {
const scope = this.getScope();
expect((scope as any)._sdkProcessingMetadata.request).toEqual(req);
return oldCaptureException.call(this, ...args);
};
Which, basically checks if the scope was correct at the time of captureException
.
And we would also throw in an expect.assertions(1);
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. And actually, we don't care about captureException
's actual work, so we can just replace the original rather than wrapping, which simplifies things even further:
hub.captureException = function (this: Hub, _exception: any) {
const scope = this.getScope();
expect((scope as any)._sdkProcessingMetadata.request).toEqual(req);
}
d56ed1c
to
5932755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go after the spelling fix!
5932755
to
def4408
Compare
This switches the request, tracing, and error handlers from the Node SDK's
handlers.ts
to use the newRequestData
integration for adding request data to events rather than the current event processor.Notes:
sdkProcessingMetadata
alongside the request so that the integration will have access to them.)parseRequest
rather thanaddRequestDataToEvent
if the request handler's options are given in the old format. Because the integration uses onlyaddRequestDataToEvent
under the hood, the backwards compatibility is now achieved by converting the old-style options into equivalent new-style options, using a new helper functionconvertReqHandlerOptsToAddReqDataOpts
. (And yes, that function name is definitely a mouthful, but fortunately only one will has to last until v8.)sdkProcessingMetadata
, even though just doing so in the request handler should technically be sufficient.Ref: #5756