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

ref(node): Use RequestData integration in express handlers #5990

Merged

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Oct 19, 2022

This switches the request, tracing, and error handlers from the Node SDK's handlers.ts to use the new RequestData integration for adding request data to events rather than the current event processor.

Notes:

  • So that this isn't a breaking change, for the moment (read: until v8) the integration will use the options passed to the request handler in place of its own options. (The request handler now stores its options in sdkProcessingMetadata alongside the request so that the integration will have access to them.)
  • Before this change, the event processor was backwards-compatible by dint of calling parseRequest rather than addRequestDataToEvent if the request handler's options are given in the old format. Because the integration uses only addRequestDataToEvent under the hood, the backwards compatibility is now achieved by converting the old-style options into equivalent new-style options, using a new helper function convertReqHandlerOptsToAddReqDataOpts. (And yes, that function name is definitely a mouthful, but fortunately only one will has to last until v8.)
  • Though in theory one should never use the error or tracing middleware without also using the request middleware, people do all sorts of weird things. All three middlewares therefore add the request to sdkProcessingMetadata, even though just doing so in the request handler should technically be sufficient.

Ref: #5756

@lobsterkatie lobsterkatie force-pushed the kmclb-use-requestdata-integration-in-express-handlers branch from 09dcbd4 to d56ed1c Compare October 19, 2022 06:42
Copy link
Member

@AbhiPrasad AbhiPrasad left a 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/types/src/transaction.ts Outdated Show resolved Hide resolved
const origPushScope = hub.pushScope;
hub.pushScope = function (this: Hub) {
scope = origPushScope.call(this);
return scope;
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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);
    }

@lobsterkatie lobsterkatie marked this pull request as ready for review October 19, 2022 07:23
@lobsterkatie lobsterkatie force-pushed the kmclb-use-requestdata-integration-in-express-handlers branch from d56ed1c to 5932755 Compare October 20, 2022 01:24
Copy link
Member

@AbhiPrasad AbhiPrasad left a 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!

@lobsterkatie lobsterkatie force-pushed the kmclb-use-requestdata-integration-in-express-handlers branch from 5932755 to def4408 Compare October 20, 2022 12:27
@lobsterkatie lobsterkatie merged commit 1f99c3b into master Oct 20, 2022
@lobsterkatie lobsterkatie deleted the kmclb-use-requestdata-integration-in-express-handlers branch October 20, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants