build: Check for circular deps with madge (getsentry#3631)

* build: Check for circular dep with madge

Madge (https://www.npmjs.com/package/madge) is a library that can
traverse module dependency trees to check for circular dependencies.

We can leverage it using `madge --circular /path/to/index.ts` to make
sure that our packages don't contain circular deps. On average madge
takes around 2-3 sec, so we are not sacrificing a ton of CI time.

* fix(node): Remove circular dependency

To remove the circular relationship between backend and parsers,
we extract the NodeOptions type into a separate types.ts file.

* fix(hub): Remove circular dep between hub and interface

To remove this, the interface.ts was removed and all of it's
types were moved into hub.ts

* fix(hub): Remove circular dep from session

To remove this circular dep, we extract the SessionFlusher
from session.ts into it's own file.

* fix(nextjs): Remove circular dependency

To remove the circular dependency between index.server.ts and
utils/instrumentServer.ts, we rely on importing Sentry func (like
startTransaction and captureException) from `@sentry/node` directly.

* build(types): Don't run madge on @sentry/types
* build: Run circularDepCheck github action

Author: AbhiPrasad

.github/workflows/build.yml

@@ -139,6 +139,29 @@ jobs:
       - name: Run linter
         run: yarn lint
 
+  job_circular_dep_check:
+    name: Circular Dependency Check
+    needs: job_build
+    timeout-minutes: 10
+    runs-on: ubuntu-latest
+    steps:
+      - name: Check out current commit (${{ github.sha }})
+        uses: actions/checkout@v2
+      - name: Set up Node
+        uses: actions/setup-node@v1
+      - name: Check dependency cache
+        uses: actions/cache@v2
+        with:
+          path: ${{ env.CACHED_DEPENDENCY_PATHS }}
+          key: ${{ needs.job_build.outputs.dependency_cache_key }}
+      - name: Check build cache
+        uses: actions/cache@v2
+        with:
+          path: ${{ env.CACHED_BUILD_PATHS }}
+          key: ${{ env.BUILD_CACHE_KEY }}
+      - name: Run madge
+        run: yarn circularDepCheck
+
   job_unit_test:
     name: Test (Node ${{ matrix.node }})
     needs: job_build

package.json

@@ -14,7 +14,8 @@
     "codecov": "codecov",
     "pack:changed": "lerna run pack --since",
     "prepublishOnly": "lerna run --stream --concurrency 1 prepublishOnly",
-    "postpublish": "make publish-docs && lerna run --stream --concurrency 1 postpublish"
+    "postpublish": "make publish-docs && lerna run --stream --concurrency 1 postpublish",
+    "circularDepCheck": "lerna run --parallel circularDepCheck"
   },
   "volta": {
     "node": "14.17.0",
@@ -58,6 +59,7 @@
     "karma-browserstack-launcher": "^1.5.1",
     "karma-firefox-launcher": "^1.1.0",
     "lerna": "3.13.4",
+    "madge": "4.0.2",
     "mocha": "^6.1.4",
     "npm-run-all": "^4.1.2",
     "prettier": "1.19.0",

packages/angular/package.json

@@ -52,7 +52,8 @@
     "fix": "run-s fix:eslint fix:prettier",
     "fix:prettier": "prettier --write \"{src,test}/**/*.ts\"",
     "fix:eslint": "eslint . --format stylish --fix",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.ts"
   },
   "volta": {
     "extends": "../../package.json"

packages/browser/package.json

@@ -82,7 +82,8 @@
     "size:check": "run-p size:check:es5 size:check:es6",
     "size:check:es5": "cat build/bundle.min.js | gzip -9 | wc -c | awk '{$1=$1/1024; print \"ES5: \",$1,\"kB\";}'",
     "size:check:es6": "cat build/bundle.es6.min.js | gzip -9 | wc -c | awk '{$1=$1/1024; print \"ES6: \",$1,\"kB\";}'",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.ts"
   },
   "volta": {
     "extends": "../../package.json"

packages/core/package.json

@@ -48,7 +48,8 @@
     "test": "jest",
     "test:watch": "jest --watch",
     "pack": "npm pack",
-    "version": "node ../../scripts/versionbump.js src/version.ts"
+    "version": "node ../../scripts/versionbump.js src/version.ts",
+    "circularDepCheck": "madge --circular src/index.ts"
   },
   "volta": {
     "extends": "../../package.json"

packages/eslint-config-sdk/package.json

@@ -40,7 +40,8 @@
     "link:yarn": "yarn link",
     "lint": "prettier --check \"**/*.js\"",
     "fix": "prettier --write \"**/*.js\"",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.js"
   },
   "volta": {
     "extends": "../../package.json"

packages/eslint-plugin-sdk/package.json

@@ -31,7 +31,8 @@
     "lint": "prettier --check \"{src,test}/**/*.js\"",
     "fix": "prettier --write \"{src,test}/**/*.js\"",
     "test": "mocha test --recursive",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.js"
   },
   "volta": {
     "extends": "../../package.json"

packages/gatsby/package.json

@@ -59,7 +59,8 @@
     "fix:eslint": "eslint . --format stylish --fix",
     "test": "jest",
     "test:watch": "jest --watch",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.ts"
   },
   "volta": {
     "extends": "../../package.json"

packages/hub/package.json

@@ -45,7 +45,8 @@
     "fix:eslint": "eslint . --format stylish --fix",
     "test": "jest",
     "test:watch": "jest --watch",
-    "pack": "npm pack"
+    "pack": "npm pack",
+    "circularDepCheck": "madge --circular src/index.ts"
   },
   "volta": {
     "extends": "../../package.json"

packages/hub/src/hub.ts

@@ -23,7 +23,6 @@ import {
 } from '@sentry/types';
 import { consoleSandbox, dateTimestampInSeconds, getGlobalObject, isNodeEnv, logger, uuid4 } from '@sentry/utils';
 
-import { Carrier, DomainAsCarrier, Layer } from './interfaces';
 import { Scope } from './scope';
 import { Session } from './session';
 
@@ -49,6 +48,47 @@ const DEFAULT_BREADCRUMBS = 100;
  */
 const MAX_BREADCRUMBS = 100;
 
+/**
+ * A layer in the process stack.
+ * @hidden
+ */
+export interface Layer {
+  client?: Client;
+  scope?: Scope;
+}
+
+/**
+ * An object that contains a hub and maintains a scope stack.
+ * @hidden
+ */
+export interface Carrier {
+  __SENTRY__?: {
+    hub?: Hub;
+    /**
+     * Extra Hub properties injected by various SDKs
+     */
+    integrations?: Integration[];
+    extensions?: {
+      /** Hack to prevent bundlers from breaking our usage of the domain package in the cross-platform Hub package */
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      domain?: { [key: string]: any };
+    } & {
+      /** Extension methods for the hub, which are bound to the current Hub instance */
+      // eslint-disable-next-line @typescript-eslint/ban-types
+      [key: string]: Function;
+    };
+  };
+}
+
+/**
+ * @hidden
+ * @deprecated Can be removed once `Hub.getActiveDomain` is removed.
+ */
+export interface DomainAsCarrier extends Carrier {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  members: { [key: string]: any }[];
+}
+
 /**
  * @inheritDoc
  */

packages/hub/src/index.ts

@@ -1,7 +1,6 @@
-// eslint-disable-next-line deprecation/deprecation
-export { Carrier, DomainAsCarrier, Layer } from './interfaces';
 export { addGlobalEventProcessor, Scope } from './scope';
-export { Session, SessionFlusher } from './session';
+export { Session } from './session';
+export { SessionFlusher } from './sessionFlusher';
 export {
   // eslint-disable-next-line deprecation/deprecation
   getActiveDomain,
@@ -11,4 +10,8 @@ export {
   Hub,
   makeMain,
   setHubOnCarrier,
+  Carrier,
+  // eslint-disable-next-line deprecation/deprecation
+  DomainAsCarrier,
+  Layer,
 } from './hub';

packages/hub/src/interfaces.ts

@@ -450,11 +450,11 @@ export class Scope implements ScopeInterface {
       } else {
         const result = processor({ ...event }, hint) as Event | null;
         if (isThenable(result)) {
-          (result as PromiseLike<Event | null>)
+          void (result as PromiseLike<Event | null>)
             .then(final => this._notifyEventProcessors(processors, final, hint, index + 1).then(resolve))
             .then(null, reject);
         } else {
-          this._notifyEventProcessors(processors, result, hint, index + 1)
+          void this._notifyEventProcessors(processors, result, hint, index + 1)
             .then(resolve)
             .then(null, reject);
         }

packages/hub/src/scope.ts

@@ -1,16 +1,5 @@
-import {
-  AggregationCounts,
-  RequestSessionStatus,
-  Session as SessionInterface,
-  SessionAggregates,
-  SessionContext,
-  SessionFlusherLike,
-  SessionStatus,
-  Transport,
-} from '@sentry/types';
-import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';
-
-import { getCurrentHub } from './hub';
+import { Session as SessionInterface, SessionContext, SessionStatus } from '@sentry/types';
+import { dropUndefinedKeys, timestampInSeconds, uuid4 } from '@sentry/utils';
 
 /**
  * @inheritdoc
@@ -145,119 +134,3 @@ export class Session implements SessionInterface {
     });
   }
 }
-
-type ReleaseHealthAttributes = {
-  environment?: string;
-  release: string;
-};
-
-/**
- * @inheritdoc
- */
-export class SessionFlusher implements SessionFlusherLike {
-  public readonly flushTimeout: number = 60;
-  private _pendingAggregates: Record<number, AggregationCounts> = {};
-  private _sessionAttrs: ReleaseHealthAttributes;
-  private _intervalId: ReturnType<typeof setInterval>;
-  private _isEnabled: boolean = true;
-  private _transport: Transport;
-
-  public constructor(transport: Transport, attrs: ReleaseHealthAttributes) {
-    this._transport = transport;
-    // Call to setInterval, so that flush is called every 60 seconds
-    this._intervalId = setInterval(() => this.flush(), this.flushTimeout * 1000);
-    this._sessionAttrs = attrs;
-  }
-
-  /** Sends session aggregates to Transport */
-  public sendSessionAggregates(sessionAggregates: SessionAggregates): void {
-    if (!this._transport.sendSession) {
-      logger.warn("Dropping session because custom transport doesn't implement sendSession");
-      return;
-    }
-    void this._transport.sendSession(sessionAggregates).then(null, reason => {
-      logger.error(`Error while sending session: ${reason}`);
-    });
-  }
-
-  /** Checks if `pendingAggregates` has entries, and if it does flushes them by calling `sendSessions` */
-  public flush(): void {
-    const sessionAggregates = this.getSessionAggregates();
-    if (sessionAggregates.aggregates.length === 0) {
-      return;
-    }
-    this._pendingAggregates = {};
-    this.sendSessionAggregates(sessionAggregates);
-  }
-
-  /** Massages the entries in `pendingAggregates` and returns aggregated sessions */
-  public getSessionAggregates(): SessionAggregates {
-    const aggregates: AggregationCounts[] = Object.keys(this._pendingAggregates).map((key: string) => {
-      return this._pendingAggregates[parseInt(key)];
-    });
-
-    const sessionAggregates: SessionAggregates = {
-      attrs: this._sessionAttrs,
-      aggregates,
-    };
-    return dropUndefinedKeys(sessionAggregates);
-  }
-
-  /** JSDoc */
-  public close(): void {
-    clearInterval(this._intervalId);
-    this._isEnabled = false;
-    this.flush();
-  }
-
-  /**
-   * Wrapper function for _incrementSessionStatusCount that checks if the instance of SessionFlusher is enabled then
-   * fetches the session status of the request from `Scope.getRequestSession().status` on the scope and passes them to
-   * `_incrementSessionStatusCount` along with the start date
-   */
-  public incrementSessionStatusCount(): void {
-    if (!this._isEnabled) {
-      return;
-    }
-    const scope = getCurrentHub().getScope();
-    const requestSession = scope?.getRequestSession();
-
-    if (requestSession && requestSession.status) {
-      this._incrementSessionStatusCount(requestSession.status, new Date());
-      // This is not entirely necessarily but is added as a safe guard to indicate the bounds of a request and so in
-      // case captureRequestSession is called more than once to prevent double count
-      scope?.setRequestSession(undefined);
-
-      /* eslint-enable @typescript-eslint/no-unsafe-member-access */
-    }
-  }
-
-  /**
-   * Increments status bucket in pendingAggregates buffer (internal state) corresponding to status of
-   * the session received
-   */
-  private _incrementSessionStatusCount(status: RequestSessionStatus, date: Date): number {
-    // Truncate minutes and seconds on Session Started attribute to have one minute bucket keys
-    const sessionStartedTrunc = new Date(date).setSeconds(0, 0);
-    this._pendingAggregates[sessionStartedTrunc] = this._pendingAggregates[sessionStartedTrunc] || {};
-
-    // corresponds to aggregated sessions in one specific minute bucket
-    // for example, {"started":"2021-03-16T08:00:00.000Z","exited":4, "errored": 1}
-    const aggregationCounts: AggregationCounts = this._pendingAggregates[sessionStartedTrunc];
-    if (!aggregationCounts.started) {
-      aggregationCounts.started = new Date(sessionStartedTrunc).toISOString();
-    }
-
-    switch (status) {
-      case RequestSessionStatus.Errored:
-        aggregationCounts.errored = (aggregationCounts.errored || 0) + 1;
-        return aggregationCounts.errored;
-      case RequestSessionStatus.Ok:
-        aggregationCounts.exited = (aggregationCounts.exited || 0) + 1;
-        return aggregationCounts.exited;
-      case RequestSessionStatus.Crashed:
-        aggregationCounts.crashed = (aggregationCounts.crashed || 0) + 1;
-        return aggregationCounts.crashed;
-    }
-  }
-}