fix(nextjs): Fix nock wrapping conflict in integration tests

[lobsterkatie]

Background

In our integration tests for nextjs, we use nock to intercept outgoing http requests, which lets us both examine a request’s payload (useful for seeing what would get sent to Sentry) and mock its response. As part of its initialization, nock monkeypatches the get and request methods in both the http and https Node built-in modules. Our Http integration also monkeypatches those methods.

There’s one key difference between our monkeypatching and their monkeypatching, however: while we always eventually call the method we’re wrapping, nock’s substitute methods only sometimes call the method being wrapped. In practical terms, what that means is that the order in which the two monkeypatching operations happen matters.

If nock monkeypatches before we do, we effectively end up with sentryWrapper(nockWrapper(originalGet)), which means that no matter what nock does or doesn’t do with originalGet, our wrapper code will always run. But if nock monkeypatches after we do, we end up with nockWrapper(sentryWrapper(originalGet)), meaning that if nock chooses not to call the function it’s wrapping, our code never runs.

Up until now, the test we have for the Http integration has been working because it’s fallen into that first scenario. More specifically, the sequence of events when running the test has gone like this (monkeypatching steps have been starred):

(1)       `yarn test:integration` -> 
(2)       test runner starts test nextjs server -> 
(3)       test runner requires `Http` test file -> 
(4)       `Http` test file requires `nock` (which it will use to intercept the request it'll make to trigger span creation) -> 
(5) *     `nock` initializes, does monkeypatching -> 
(6)       `Http` test makes request to API route whose handler will trigger the outgoing request -> 
(7)       nextjs loads API route file, which has had `Sentry.init()` code injected during build -> 
(8)       `Sentry.init()` runs ->
(9) *     `Http` integration is initialized, does monkeypatching

Starting with Nextjs 12.1, however, things go in a different order:

(1)       `yarn test:integration` -> 
(2)       test runner starts test nextjs server -> 
(2a)      nextjs server loads `_app`, which has had `Sentry.init()` code injected during build
(8/2b)    `Sentry.init()` runs ->
(9/2c) *  `Http` integration is initialized, does monkeypatching ->
(3)       test runner requires `Http` test file -> 
(4)       `Http` test file requires `nock` (which it will use to intercept the request it'll make to trigger span creation) -> 
(5) *     `nock` initializes, does monkeypatching -> 
(6)       `Http` test makes request to API route whose handler will trigger the outgoing request -> 
(7)       nextjs loads API route file, which has had `Sentry.init()` code injected during build -> 
(8)       `Sentry.init()` runs ->
(9')      the SDK is already running, so `Sentry.init()` bails

Note the new steps after (2), including the former (8) and (9), and the new version of (9). As we can see, the order of the monkeypatching has been reversed.

Cause

The reason for this change is this PR, which causes Next to load the _app and _document pages as soon as the server starts, in the interest of serving the first requested page more quickly.

Impact

The consequence of this change is that in our integration test of the Http integration, our wrapper code isn't called. As a result, the span the test expects will be created when it makes an http request isn't actually created, and the test fails.

Solution

Though it's somewhat hacky, the solution in this PR is to force get (and request, though that's irrelevant here) to be wrapped again by Sentry after they are wrapped by nock. The result is a little messy, but fortunately turns out to be messy in ways which don't matter, as described in a comment in the new function which does the rewrapping:

  // This rewraps `http.get` and `http.request`, which, at this point, look like `nockWrapper(sentryWrapper(get))` and
  // `nockWrapper(sentryWrapper(request))`. By the time we're done with this function, they'll look like
  // `sentryWrapper(nockWrapper(sentryWrapper(get)))` and `sentryWrapper(nockWrapper(sentryWrapper(request)))`,
  // respectively. Though this seems less than ideal, we don't have to worry about our instrumentation being
  // (meaningfully) called twice because:
  //
  // 1) As long as we set up a `nock` interceptor for any outgoing http request, `nock`'s wrapper will call a replacement
  //    function for that request rather than call the function it's wrapping (in other words, it will look more like
  //    `sentryWrapper(nockWrapper(getSubstitute))` than `sentryWrapper(nockWrapper(sentryWrapper(get)))`), which means
  //    our code is only called once.
  // 2) In cases where we don't set up an interceptor (such as for the `wrappedGet` call in `getAsync` above), it's true
  //    that we can end up with `sentryWrapper(nockWrapper(sentryWrapper(get)))`, meaning our wrapper code will run
  //    twice. For now that's okay because in those cases we're not in the middle of a transactoin and therefore
  //    the two wrappers' respective attempts to start spans will both no-op.
  //
  // TL; DR - if the double-wrapping means you're seeing two spans where you really only want one, set up a nock
  // interceptor for the request.

There are other comments (TODOs) in the code discussing a somewhat simpler strategy, but other work will need to be done first, and this should at least get CI unblocked.

[github-actions]

size-limit report

Path	Base Size (1bf9883)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.69 KB	19.69 KB	-0.01% 🔽
@sentry/browser - ES5 CDN Bundle (minified)	63.27 KB	63.27 KB	0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.36 KB	18.36 KB	-0.01% 🔽
@sentry/browser - ES6 CDN Bundle (minified)	56.44 KB	56.44 KB	0%
@sentry/browser - Webpack (gzipped + minified)	22.14 KB	22.14 KB	0%
@sentry/browser - Webpack (minified)	76.14 KB	76.14 KB	0%
@sentry/react - Webpack (gzipped + minified)	22.17 KB	22.17 KB	0%
@sentry/nextjs Client - Webpack (gzipped + minified)	46.33 KB	46.33 KB	0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	27.15 KB	27.15 KB	-0.01% 🔽

[AbhiPrasad]

Awesome investigative work!

[AbhiPrasad]

I’m going to go ahead and merge this to unblock CI