ref(node): Move non-handler code out of `handlers` module #5190

lobsterkatie · 2022-06-01T20:57:16Z

Currently, the handlers module includes not just our middleware handlers but also a bunch of code for extracting context data and applying it to events. That latter code is indeed used in the handlers, but it's also used in a number of other places in the repo. This pulls that code into its own module in @sentry/utils, to make it possible to use in a platform-agnostic context.

Key changes:

A new requestData module in @sentry/utils to hold the data extraction code (and a new test file for its tests).
Wrapper functions in the handlers module for backwards compatibility. (IOW, calling Handlers.someFunc() still works, but it's now just a passthrough to someFunc from the new module.)
Deprecation notices in the docstrings of the wrappers, and TODOs so we remember to make the switch in v8.
Wrapper describes in the tests, to run the tests against both the real functions and their wrappers, to make sure the wrappers work identically to the real functions. (This means that for the moment, the test file needs to live in @sentry/node, since it depends on the wrappers as well as the real functions. Once the wrappers are removed, the test file can be moved to @sentry/utils.)
parseRequest has been renamed addRequestDataToEvent for greater accuracy and clarity for the reader. (The corresponding options type has also been renamed.)
Two pieces of context data which have until now been set by parseRequest, but which have nothing to do with the request and are Node-only (namely runtime and server_name) are now set by the Node client. As a result, they no longer need to be set separately in the serverless package. (The exception is server_name in the AWS integration, because it pulls from a non-standard location.)

Notes:

The original impetus for this change is the need to call parseRequest/addRequestDataToEvent in the nextjs SDK, in code which can run either in Node or the browser, which means it can't be exported from @sentry/node.
Technically this PR has a breaking change, in that now all Node events will have runtime and servername data, whereas before only events generated by our middleware have had it. (So, for example, if you've built a CLI in Node and are monitoring it with Sentry, your events will now include this data where they didn't before.) This feels like an okay thing to me, but I'm open to other opinions.

github-actions · 2022-06-01T21:03:55Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.43 KB (-3.54% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	60.2 KB (-6.83% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.23 KB (-3.33% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	53.82 KB (-7.15% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.99 KB (-13.96% 🔽)
@sentry/browser - Webpack (minified)	65.19 KB (-20.22% 🔽)
@sentry/react - Webpack (gzipped + minified)	20.02 KB (-14.01% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	43.76 KB (-8.93% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.46 KB (-2.38% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.98 KB (-2.05% 🔽)

AbhiPrasad

Technically this PR has a breaking change, in that now all Node events will have runtime and servername data, whereas before only events generated by our middleware have had it. (So, for example, if you've built a CLI in Node and are monitoring it with Sentry, your events will now include this data.) This feels like an okay thing to me, but I'm open to other opinions.

I think it's a non-issue - more like a feature actually! We've never hesitated with adding additional tags/contexts before. I did ask about moving stuff into a default integration, maybe that might appease folks who would want to turn it off (idk who would though).

AbhiPrasad · 2022-06-02T15:16:58Z

packages/node/src/client.ts

+        version: global.process.version,
+      },
+    };
+    event.server_name = this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname();


What do you think about putting this event mutation into a default integration instead of in the prepareEvent step here?

This is the pattern most of the other SDKs follow for this.

Interesting idea. I looked but didn't actually find any examples where we're using an integration to just set a few pieces of metadata. Which ones were you thinking of?

TBH, my gut feeling on it is that it feels kinda like overkill - it's a lot of machinery to wrap two lines of code, no? (Also, I know bundle size isn't as much of a thing on the node side of things, but given serverless functions it's still something to be at least vaguely aware of, so I'd only want to do this if it seemed worth the extra code.)

Let's wait and see if anyone complains, and then if we need to pull it into an integration so that people can shut it off, we can do that.

WDYT?

Sure that sounds fine to me!

packages/node/src/requestdata.ts

AbhiPrasad · 2022-06-02T15:29:45Z

packages/serverless/src/gcpfunction/http.ts

-export interface HttpFunctionWrapperOptions extends WrapperOptions {
-  parseRequestOptions: ParseRequestOptions;
+// TODO (v8): Remove this
+interface OldHttpFunctionWrapperOptions extends WrapperOptions {


This applies to the other items that are todo v8. Would you mind updating #5194 when you merge this? In the issue we probably just want a link to the PR doing the deprecation, and a short sentence about how it is replaced.

If we do this now, it'll be much less work for us to figure it out what to do for v8, we can just look at the issue and check off the items.

yordis · 2022-06-02T16:04:51Z

packages/node/src/requestdata.ts

+      return extractExpressTransactionName(req, { path: true });
+    }
+    case 'handler': {
+      return req.route?.stack[0].name || '<anonymous>';


Suggested change

return req.route?.stack[0].name || '<anonymous>';

return req.route?.stack[0]?.name || '<anonymous>';

Feels like one of those that will happen, just a matter of time 😄

Happy to add it, but I actually am not sure it's possible for stack to be empty. If you look at the Express router here and here, you can see that every route gets at least a layer for / added to its stack. (It's been more than a year since I last mucked about in the Express innards trying to solve #3155, so I wouldn't swear that there isn't some way around stack having at least one entry, but I don't think so.)

Since you already doing route? I wouldn't mind, my only concern is that far too often, the correct assumptions were true only at X given time.

Again, there is nothing wrong at all as of today. Just that off-by-one errors are a thing 😄

It's true. And as I said, happy to add it. (You'll see that in fact, I already have!)

Lms24

Nice refactoring! Looks good to me overall. I agree that the "breaking" change is fine in this case.

Lms24 · 2022-06-02T17:01:28Z

packages/serverless/test/awslambda.test.ts

-      expect.assertions(10);
+      expect.assertions(9);


Why did the number of expected assertions decrease?

Because of the removal of the expect on line 49, which happened because that info is now set by the node client, not the aws integration.

)" This reverts commit 7c521e3.

timfish · 2022-06-07T20:44:20Z

This change broke the Electron SDK upgrade since it overrides the contexts.runtime.

sentry-javascript/packages/node/src/client.ts

Lines 143 to 149 in fb267e0

    
           event.contexts = { 
        
             ...event.contexts, 
        
             runtime: { 
        
               name: 'node', 
        
               version: global.process.version, 
        
             }, 
        
           };

AbhiPrasad · 2022-06-07T20:50:08Z

This change broke the Electron SDK upgrade since it overrides the contexts.runtime.name with node whereas it's been Electron:

Perhaps we should move that logic into an integration then! cc @lobsterkatie

)" (#5223) This reverts commit 7c521e3. This PR causes issues with users as reported here: #5222. This is because we added Node library imports in the utils package - which can be consumed in browser contexts.

…5257) (See #5190 for a detailed description of the motivation for and main substance of this change. TL;DR, isomorphic frameworks like Nextjs need this code to live in a neutral location (rather than in the node package) so it can be used in a browser setting as well.) This is a second take on #5190, which had to be reverted because it relied on a peer dependency (`cookie`) which browser-only apps wouldn't have installed. Even if those code didn't _use_ `cookie`, they still failed to compile because without `cookie` installed, `@sentry/utils` didn't typecheck correctly. This fixes that problem by using `cookie` (and `url`, for node 8 compatibility) only as injected dependencies, not as direct ones. It also creates node-specific versions of the relevant functions (`parseRequest`, now renamed `addRequestDataToEvent`, and `extractRequestData`) which do the injection automatically. If the dependencies aren't passed (as would be the case in a browser setting, or when using the functions directly from `@sentry/utils`), the code about cookies no-ops, and the code about URLs uses `URL`, which should be defined in all modern browsers and Node 10 and above. Other changes from the original PR: - Now only the backwards-compatibility-preserving wrappers of the legacy code live in `handlers.ts`, while the legacy code itself lives in its own file. This both makes it easier to delete in the future and ensures that treeshaking algorithms which only work file-by-file (rather than function-by-function) are able to exclude that code. (Though it's being kept until v8 because it's part of the node package's public API, it's no longer used anywhere in the SDK.) - Using DI changed the options interface for both of the functions in question, which in turn required a bit more gymnastics in terms of preserving and test backwards compatibility, specifically in the serverless package and the backwards-compatibility tests. This will make it a little harder to delete the old stuff when it comes time, but there are TODOs which hopefully will make it clear enough what needs to happen. - There's a new `CrossPlatformRequest` type for use in place of `ExpressRequest`. A few helper functions have also been renamed to make them less Express-centric.

lobsterkatie requested a review from Lms24 June 2, 2022 14:03

AbhiPrasad approved these changes Jun 2, 2022

View reviewed changes

yordis reviewed Jun 2, 2022

View reviewed changes

Lms24 approved these changes Jun 2, 2022

View reviewed changes

lobsterkatie mentioned this pull request Jun 3, 2022

JS SDK v8 Deprecations List #5194

Closed

lobsterkatie force-pushed the kmclb-split-up-node-handlers-module branch 3 times, most recently from de6b20f to c252036 Compare June 3, 2022 23:40

lobsterkatie added 4 commits June 3, 2022 16:47

break up handlers module and fix imports

663d2cd

s/parseRequest/addRequestDataToEvent

3dbd1c0

move node-version- and server-name-adding code to client

405b0c0

fix tests

31c6675

lobsterkatie force-pushed the kmclb-split-up-node-handlers-module branch from c252036 to 31c6675 Compare June 3, 2022 23:48

lobsterkatie merged commit 7c521e3 into master Jun 4, 2022

lobsterkatie deleted the kmclb-split-up-node-handlers-module branch June 4, 2022 00:04

AbhiPrasad mentioned this pull request Jun 7, 2022

Compilation error after upgrading to @sentry/react 7.1.0 #5222

Closed

3 tasks

AbhiPrasad added a commit that referenced this pull request Jun 7, 2022

Revert "ref(node): Move non-handler code out of handlers module (#5190

25e1219

)" This reverts commit 7c521e3.

AbhiPrasad mentioned this pull request Jun 7, 2022

Revert "ref(node): Move non-handler code out of handlers module" #5223

Merged

AbhiPrasad mentioned this pull request Jun 7, 2022

fix(node): Only set contexts runtime if not set. #5226

Closed

lobsterkatie mentioned this pull request Jun 13, 2022

ref(node): Move request-data-extraction functions to@sentry/utils #5257

Merged

matthew-white mentioned this pull request Sep 22, 2022

Sentry errors without server_name getodk/central-backend#483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(node): Move non-handler code out of `handlers` module #5190

ref(node): Move non-handler code out of `handlers` module #5190

lobsterkatie commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022 •

edited

Loading

AbhiPrasad left a comment •

edited

Loading

AbhiPrasad Jun 2, 2022 •

edited

Loading

lobsterkatie Jun 3, 2022

AbhiPrasad Jun 3, 2022

AbhiPrasad Jun 2, 2022 •

edited

Loading

lobsterkatie Jun 3, 2022

yordis Jun 2, 2022

lobsterkatie Jun 3, 2022 •

edited

Loading

yordis Jun 3, 2022

lobsterkatie Jun 3, 2022

Lms24 left a comment

Lms24 Jun 2, 2022

lobsterkatie Jun 3, 2022

timfish commented Jun 7, 2022 •

edited

Loading

AbhiPrasad commented Jun 7, 2022 •

edited

Loading

	return req.route?.stack[0].name \|\| '<anonymous>';
	return req.route?.stack[0]?.name \|\| '<anonymous>';

ref(node): Move non-handler code out of handlers module #5190

ref(node): Move non-handler code out of handlers module #5190

Conversation

lobsterkatie commented Jun 1, 2022 • edited Loading

github-actions bot commented Jun 1, 2022 • edited Loading

size-limit report 📦

AbhiPrasad left a comment • edited Loading

Choose a reason for hiding this comment

AbhiPrasad Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobsterkatie Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timfish commented Jun 7, 2022 • edited Loading

AbhiPrasad commented Jun 7, 2022 • edited Loading

ref(node): Move non-handler code out of `handlers` module #5190

ref(node): Move non-handler code out of `handlers` module #5190

lobsterkatie commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022 •

edited

Loading

AbhiPrasad left a comment •

edited

Loading

AbhiPrasad Jun 2, 2022 •

edited

Loading

AbhiPrasad Jun 2, 2022 •

edited

Loading

lobsterkatie Jun 3, 2022 •

edited

Loading

timfish commented Jun 7, 2022 •

edited

Loading

AbhiPrasad commented Jun 7, 2022 •

edited

Loading