diff --git a/packages/nextjs/test/integration/test/server/tracingHttp.js b/packages/nextjs/test/integration/test/server/tracingHttp.js index 02a61a78a047..6ce6ca14def6 100644 --- a/packages/nextjs/test/integration/test/server/tracingHttp.js +++ b/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'); diff --git a/packages/nextjs/test/integration/test/utils/server.js b/packages/nextjs/test/integration/test/utils/server.js index 09c460986d96..8844c8cc1799 100644 --- a/packages/nextjs/test/integration/test/utils/server.js +++ b/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, diff --git a/packages/nextjs/test/run-integration-tests.sh b/packages/nextjs/test/run-integration-tests.sh index de4807dc549f..4f31ce75cd2b 100755 --- a/packages/nextjs/test/run-integration-tests.sh +++ b/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"