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

In our integration tests for nextjs, we use `nock` to intercept outgoing http requests, which lets us both examine a request’s payload 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.

The order in which the two monkeypatching operations happen matters. If `nock` monkeypatches before Sentry does we 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.

Next 12.1 introduces a change in [this PR](vercel/next.js#23261), 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. This causes the order of the monkey patching to change, causing the http tests to fail for nextjs. 

This patch solves this by forcing `get` and `request` to be wrapped again by Sentry after they are wrapped by `nock`.

There are some TODOs that need to be addressed, but merging this patch to unblock CI.

Author: lobsterkatie

packages/nextjs/test/integration/test/server/tracingHttp.js

@@ -40,7 +40,10 @@ module.exports = async ({ url: urlBase, argv }) => {
     'tracingHttp',
   );
 
-  await getAsync(url);
+  // The `true` causes `getAsync` to rewrap `http.get` in next 12, since it will have been overwritten by the import of
+  // `nock` above. See https://github.com/getsentry/sentry-javascript/pull/4619.
+  // TODO: see note in `getAsync` about removing the boolean
+  await getAsync(url, true);
   await sleep(250);
 
   assert.ok(capturedRequest.isDone(), 'Did not intercept expected request');

packages/nextjs/test/integration/test/utils/server.js

@@ -1,12 +1,22 @@
 const { get } = require('http');
 const nock = require('nock');
+const nodeSDK = require('@sentry/node');
 const { logIf, parseEnvelope } = require('./common');
 
 Error.stackTraceLimit = Infinity;
 
-const getAsync = url => {
+const getAsync = (url, rewrap = false) => {
+  // Depending on what version of Nextjs we're testing, the wrapping which happens in the `Http` integration may have
+  // happened too early and gotten overwritten by `nock`. This redoes the wrapping if so.
+  //
+  // TODO: This works but is pretty hacky in that it has the potential to wrap things multiple times, more even than the
+  // double-wrapping which is discussed at length in the comment in `ensureWrappedGet` below, which is why we need
+  // `rewrap`. Once we fix `fill` to not wrap things twice, we should be able to take this out and just always call
+  // `ensureWrappedGet`.
+  const wrappedGet = rewrap ? ensureWrappedGet(get, url) : get;
+
   return new Promise((resolve, reject) => {
-    get(url, res => {
+    wrappedGet(url, res => {
       res.setEncoding('utf8');
       let rawData = '';
       res.on('data', chunk => {
@@ -99,6 +109,60 @@ const objectMatches = (actual, expected) => {
   return true;
 };
 
+/**
+ * Rewrap `http.get` if the wrapped version has been overridden by `nock`.
+ *
+ * This is only relevant for Nextjs >= 12.1, which changed when `_app` is initialized, which in turn changed the order
+ * in which our SDK and `nock` wrap `http.get`. See https://github.com/getsentry/sentry-javascript/pull/4619.
+ *
+ * TODO: We'll have to do this for `ClientRequest` also if we decide to start wrapping that.
+ * TODO: Can we fix the wrapping-things-twice problem discussed in the comment below?
+ */
+function ensureWrappedGet(importedGet, url) {
+  // we always test against the latest minor for any given Nextjs major version, so if we're testing Next 12, it's
+  // definitely at least 12.1, making this check against the major version sufficient
+  if (Number(process.env.NEXTJS_VERSION) < 12) {
+    return importedGet;
+  }
+
+  // As of Next 12.1, creating a `NextServer` instance (which we do immediately upon starting this test runner) loads
+  // `_app`, which has the effect of initializing the SDK. So, unless something's gone wrong, we should always be able
+  // to find the integration
+  let httpIntegration;
+  try {
+    httpIntegration = nodeSDK.getCurrentHub().getClient().getIntegration(nodeSDK.Integrations.Http);
+  } catch (err) {
+    console.warn(`Warning: Sentry SDK not set up at \`NextServer\` initialization. Request URL: ${url}`);
+    return importedGet;
+  }
+
+  // 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.
+  //
+  // TODO: add in a "don't do this twice" check (in `fill`, maybe moved from `wrap`), so that we don't wrap the outer
+  // wrapper with a third wrapper
+  httpIntegration.setupOnce();
+
+  // now that we've rewrapped it, grab the correct version of `get` for use in our tests
+  const httpModule = require('http');
+  return httpModule.get;
+}
+
 module.exports = {
   getAsync,
   interceptEventRequest,

packages/nextjs/test/run-integration-tests.sh

@@ -32,6 +32,11 @@ mv next.config.js next.config.js.bak
 
 for NEXTJS_VERSION in 10 11 12; do
 
+  # export this to the env so that we can behave differently depending on which version of next we're testing, without
+  # having to pass this value from function to function to function to the one spot, deep in some callstack, where we
+  # actually need it
+  export NEXTJS_VERSION=$NEXTJS_VERSION
+
   # Next 10 requires at least Node v10
   if [ "$NODE_MAJOR" -lt "10" ]; then
     echo "[nextjs] Next.js is not compatible with versions of Node older than v10. Current version $NODE_VERSION"