Merge branch 'master' of https://github.com/getsentry/sentry-javascript into onur/test-bundles

Author: onurtemizkan

.github/ISSUE_TEMPLATE/bug.yml

@@ -0,0 +1,91 @@
+name: 🐞 Bug Report
+description: Tell us about something that's not working the way we (probably) intend.
+labels: 'Type: Bug'
+body:
+  - type: checkboxes
+    attributes:
+      label: Is there an existing issue for this?
+      description: Please search to see if an issue already exists for the bug you encountered.
+      options:
+        - label: I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
+          required: true
+        - label: I have reviewed the documentation https://docs.sentry.io/
+          required: true
+        - label: I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
+          required: true
+  - type: dropdown
+    id: type
+    attributes:
+      label: How do you use Sentry?
+      options:
+        - Sentry Saas (sentry.io)
+        - Self-hosted/on-premise
+    validations:
+      required: true
+  - type: dropdown
+    id: package
+    attributes:
+      label: Which package are you using?
+      options:
+        - "@sentry/angular"
+        - "@sentry/browser"
+        - "@sentry/ember"
+        - "@sentry/gatsby"
+        - "@sentry/nextjs"
+        - "@sentry/node"
+        - "@sentry/react"
+        - "@sentry/serverless"
+        - "@sentry/vue"
+        - "@sentry/wasm"
+    validations:
+      required: true
+  - type: input
+    id: sdk-version
+    attributes:
+      label: SDK Version
+      description: What version of the SDK are you using?
+      placeholder: ex. 1.5.2
+    validations:
+      required: true
+  - type: input
+    id: framework-version
+    attributes:
+      label: Framework Version
+      description: If you're using one of our framework-specific SDKs (`@sentry/react`, for example), what version of the framework are you using?
+      placeholder: ex. 1.5.2
+  - type: input
+    id: link-to-sentry
+    attributes:
+      label: Link to Sentry event
+      description: If applicable, provide a link to the affected event from your Sentry account. The event will only be viewable by Sentry staff.
+      placeholder: https://sentry.io/organizations/<org-slug>/issues/<issue-id>/events/<event-id>/?project=<project-id>
+  - type: textarea
+    id: repro
+    attributes:
+      label: Steps to Reproduce
+      description: How can we see what you're seeing? Specific is terrific.
+      placeholder: |-
+        1. What
+        2. you
+        3. did.
+    validations:
+      required: true
+  - type: textarea
+    id: expected
+    attributes:
+      label: Expected Result
+    validations:
+      required: true
+  - type: textarea
+    id: actual
+    attributes:
+      label: Actual Result
+      description: Logs? Screenshots? Yes, please.
+    validations:
+      required: true
+  - type: markdown
+    attributes:
+      value: |-
+        ## Thanks 🙏
+    validations:
+      required: false

.github/ISSUE_TEMPLATE/bug_report.md

@@ -0,0 +1,5 @@
+blank_issues_enabled: false
+contact_links:
+  - name: Ask a question
+    url: https://github.com/getsentry/sentry-javascript/issues
+    about: Ask questions and discuss with other community members

.github/ISSUE_TEMPLATE/config.yml

@@ -0,0 +1,30 @@
+name: 💡 Feature Request
+description: Create a feature request for a sentry-javascript SDK.
+labels: 'Type: Improvement'
+body:
+  - type: markdown
+    attributes:
+      value: Thanks for taking the time to file a feature request! Please fill out this form as completely as possible.
+  - type: textarea
+    id: problem
+    attributes:
+      label: Problem Statement
+      description: A clear and concise description of what you want and what your use case is.
+      placeholder: |-
+        I want to make whirled peas, but Sentry doesn't blend.
+    validations:
+      required: true
+  - type: textarea
+    id: expected
+    attributes:
+      label: Solution Brainstorm
+      description: We know you have bright ideas to share ... share away, friend.
+      placeholder: |-
+        Add a blender to Sentry.
+    validations:
+      required: true
+  - type: markdown
+    attributes:
+      value: |-
+        ## Thanks 🙏
+        Check our [triage docs](https://open.sentry.io/triage/) for what to expect next.

.github/ISSUE_TEMPLATE/feature.yml

@@ -72,7 +72,7 @@ We deprecated the `Status` enum in `@sentry/tracing` and it will be removed in t
 
 ```js
 // New in 6.17.0:
-import { spanStatusfromHttpCode } from '@sentry/utils';
+import { spanStatusfromHttpCode } from '@sentry/tracing';
 
 const status = spanStatusfromHttpCode(403);
 

MIGRATION.md

@@ -143,9 +143,10 @@ export function eventFromString(
   };
 
   if (options.attachStacktrace && syntheticException) {
-    event.stacktrace = {
-      frames: parseStackFrames(syntheticException),
-    };
+    const frames = parseStackFrames(syntheticException);
+    if (frames.length) {
+      event.stacktrace = { frames };
+    }
   }
 
   return event;

packages/browser/src/eventbuilder.ts

@@ -54,9 +54,10 @@ export function eventFromPlainObject(
   };
 
   if (syntheticException) {
-    event.stacktrace = {
-      frames: parseStackFrames(syntheticException),
-    };
+    const frames = parseStackFrames(syntheticException);
+    if (frames.length) {
+      event.stacktrace = { frames };
+    }
   }
 
   return event;

packages/browser/src/parsers.ts

@@ -23,12 +23,20 @@ const domain = domainModule as typeof domainModule & { active: (domainModule.Dom
 // During build, the main process is invoked by
 //   `node next build`
 // and child processes are invoked as
-//   `node <path>/node_modules/jest-worker/build/workers/processChild.js`,
-// whereas at runtime the process is invoked as
+//   `node <path>/node_modules/.../jest-worker/processChild.js`.
+// The former is (obviously) easy to recognize, but the latter could happen at runtime as well. Fortunately, the main
+// process hits this file before any of the child processes do, so we're able to set an env variable which the child
+// processes can then check. During runtime, the main process is invoked as
 //   `node next start`
 // or
-//   `node /var/runtime/index.js`.
-const isBuild = new RegExp('build').test(process.argv.toString());
+//   `node /var/runtime/index.js`,
+// so we never drop into the `if` in the first place.
+let isBuild = false;
+if (process.argv.includes('build') || process.env.SENTRY_BUILD_PHASE) {
+  process.env.SENTRY_BUILD_PHASE = 'true';
+  isBuild = true;
+}
+
 const isVercel = !!process.env.VERCEL;
 
 /** Inits the Sentry NextJS SDK on node. */

packages/nextjs/src/index.server.ts

@@ -1,4 +1,3 @@
-import { BaseClient } from '@sentry/core';
 import { RewriteFrames } from '@sentry/integrations';
 import * as SentryNode from '@sentry/node';
 import { getCurrentHub, NodeClient } from '@sentry/node';
@@ -17,7 +16,6 @@ const global = getGlobalObject();
 (global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__ = '.next';
 
 const nodeInit = jest.spyOn(SentryNode, 'init');
-const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
 const logError = jest.spyOn(logger, 'error');
 
 describe('Server init()', () => {
@@ -91,7 +89,7 @@ describe('Server init()', () => {
     expect(currentScope._tags.vercel).toBeUndefined();
   });
 
-  it('adds 404 transaction filter', () => {
+  it('adds 404 transaction filter', async () => {
     init({
       dsn: 'https://[email protected]/12312012',
       tracesSampleRate: 1.0,
@@ -102,8 +100,10 @@ describe('Server init()', () => {
     const transaction = hub.startTransaction({ name: '/404' });
     transaction.finish();
 
+    // We need to flush because the event processor pipeline is async whereas transaction.finish() is sync.
+    await SentryNode.flush();
+
     expect(sendEvent).not.toHaveBeenCalled();
-    expect(captureEvent.mock.results[0].value).toBeUndefined();
     expect(logError).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.'));
   });
 

packages/nextjs/test/index.server.test.ts

@@ -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/server/tracingHttp.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/integration/test/utils/server.js

@@ -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"

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

@@ -16,7 +16,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
    */
   // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
   public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
-    return eventFromException(this._options, exception, hint);
+    return eventFromException(exception, hint);
   }
 
   /**