ref(core): Prevent redundant setup work (#3862)

This fixes two places where we do work unnecessarily:

1) When creating a new `Hub` instance we call `bindClient`, even if there's no client to bind.

2) When calling `Client.setupIntegrations()` (as we do when binding a client to a hub), we run through the whole integration-setup process, even if the client has already had its integrations set up.

(The latter situation comes up, among other places, any time we create a domain: each new domain gets its own hub, which then is bound to the current client, meaning up until now we've been calling `setupIntegrations()` for every incoming request handled by either our Express integration or our nextjs SDK.)

To prevent this, we now (as of this change) check a) if there's a client to bind to the new hub, and b) if the client having its integrations set up has already had that done.

Author: lobsterkatie

packages/core/src/baseclient.ts

@@ -207,7 +207,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
    * Sets up the integrations
    */
   public setupIntegrations(): void {
-    if (this._isEnabled()) {
+    if (this._isEnabled() && !this._integrations.initialized) {
       this._integrations = setupIntegrations(this._options);
     }
   }

packages/core/src/integration.ts

@@ -5,9 +5,9 @@ import { logger } from '@sentry/utils';
 export const installedIntegrations: string[] = [];
 
 /** Map of integrations assigned to a client */
-export interface IntegrationIndex {
+export type IntegrationIndex = {
   [key: string]: Integration;
-}
+} & { initialized?: boolean };
 
 /**
  * @private
@@ -74,5 +74,9 @@ export function setupIntegrations<O extends Options>(options: O): IntegrationInd
     integrations[integration.name] = integration;
     setupIntegration(integration);
   });
+  // set the `initialized` flag so we don't run through the process again unecessarily; use `Object.defineProperty`
+  // because by default it creates a property which is nonenumerable, which we want since `initialized` shouldn't be
+  // considered a member of the index the way the actual integrations are
+  Object.defineProperty(integrations, 'initialized', { value: true });
   return integrations;
 }

packages/core/test/lib/base.test.ts

@@ -2,6 +2,7 @@ import { Hub, Scope, Session } from '@sentry/hub';
 import { Event, Severity, Span } from '@sentry/types';
 import { logger, SentryError, SyncPromise } from '@sentry/utils';
 
+import * as integrationModule from '../../src/integration';
 import { TestBackend } from '../mocks/backend';
 import { TestClient } from '../mocks/client';
 import { TestIntegration } from '../mocks/integration';
@@ -872,6 +873,26 @@ describe('BaseClient', () => {
       expect(Object.keys((client as any)._integrations).length).toBe(0);
       expect(client.getIntegration(TestIntegration)).toBeFalsy();
     });
+
+    test('skips installation if integrations are already installed', () => {
+      expect.assertions(4);
+      const client = new TestClient({
+        dsn: PUBLIC_DSN,
+        integrations: [new TestIntegration()],
+      });
+      // note: not the `Client` method `setupIntegrations`, but the free-standing function which that method calls
+      const setupIntegrationsHelper = jest.spyOn(integrationModule, 'setupIntegrations');
+
+      // it should install the first time, because integrations aren't yet installed...
+      client.setupIntegrations();
+      expect(Object.keys((client as any)._integrations).length).toBe(1);
+      expect(client.getIntegration(TestIntegration)).toBeTruthy();
+      expect(setupIntegrationsHelper).toHaveBeenCalledTimes(1);
+
+      // ...but it shouldn't try to install a second time
+      client.setupIntegrations();
+      expect(setupIntegrationsHelper).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('flush/close', () => {

packages/hub/src/hub.ts

@@ -103,7 +103,9 @@ export class Hub implements HubInterface {
    */
   public constructor(client?: Client, scope: Scope = new Scope(), private readonly _version: number = API_VERSION) {
     this.getStackTop().scope = scope;
-    this.bindClient(client);
+    if (client) {
+      this.bindClient(client);
+    }
   }
 
   /**