fix: Prevent scope from storing more than 100 breadcrumbs at the time (getsentry#3677)

Author: kamilogorek

packages/hub/src/hub.ts

@@ -42,12 +42,6 @@ export const API_VERSION = 4;
  */
 const DEFAULT_BREADCRUMBS = 100;
 
-/**
- * Absolute maximum number of breadcrumbs added to an event. The
- * `maxBreadcrumbs` option cannot be higher than this value.
- */
-const MAX_BREADCRUMBS = 100;
-
 /**
  * A layer in the process stack.
  * @hidden
@@ -289,7 +283,7 @@ export class Hub implements HubInterface {
 
     if (finalBreadcrumb === null) return;
 
-    scope.addBreadcrumb(finalBreadcrumb, Math.min(maxBreadcrumbs, MAX_BREADCRUMBS));
+    scope.addBreadcrumb(finalBreadcrumb, maxBreadcrumbs);
   }
 
   /**

packages/hub/src/scope.ts

@@ -22,6 +22,12 @@ import { dateTimestampInSeconds, getGlobalObject, isPlainObject, isThenable, Syn
 
 import { Session } from './session';
 
+/**
+ * Absolute maximum number of breadcrumbs added to an event.
+ * The `maxBreadcrumbs` option cannot be higher than this value.
+ */
+const MAX_BREADCRUMBS = 100;
+
 /**
  * Holds additional event information. {@link Scope.applyToEvent} will be
  * called by the client before an event will be sent.
@@ -366,16 +372,20 @@ export class Scope implements ScopeInterface {
    * @inheritDoc
    */
   public addBreadcrumb(breadcrumb: Breadcrumb, maxBreadcrumbs?: number): this {
+    const maxCrumbs = typeof maxBreadcrumbs === 'number' ? Math.min(maxBreadcrumbs, MAX_BREADCRUMBS) : MAX_BREADCRUMBS;
+
+    // No data has been changed, so don't notify scope listeners
+    if (maxCrumbs <= 0) {
+      return this;
+    }
+
     const mergedBreadcrumb = {
       timestamp: dateTimestampInSeconds(),
       ...breadcrumb,
     };
-
-    this._breadcrumbs =
-      maxBreadcrumbs !== undefined && maxBreadcrumbs >= 0
-        ? [...this._breadcrumbs, mergedBreadcrumb].slice(-maxBreadcrumbs)
-        : [...this._breadcrumbs, mergedBreadcrumb];
+    this._breadcrumbs = [...this._breadcrumbs, mergedBreadcrumb].slice(-maxCrumbs);
     this._notifyScopeListeners();
+
     return this;
   }
 

packages/hub/test/scope.test.ts

@@ -63,10 +63,26 @@ describe('Scope', () => {
 
     test('addBreadcrumb', () => {
       const scope = new Scope();
-      scope.addBreadcrumb({ message: 'test' }, 100);
+      scope.addBreadcrumb({ message: 'test' });
       expect((scope as any)._breadcrumbs[0]).toHaveProperty('message', 'test');
     });
 
+    test('addBreadcrumb can be limited to hold up to N breadcrumbs', () => {
+      const scope = new Scope();
+      for (let i = 0; i < 10; i++) {
+        scope.addBreadcrumb({ message: 'test' }, 5);
+      }
+      expect((scope as any)._breadcrumbs).toHaveLength(5);
+    });
+
+    test('addBreadcrumb cannot go over MAX_BREADCRUMBS value', () => {
+      const scope = new Scope();
+      for (let i = 0; i < 111; i++) {
+        scope.addBreadcrumb({ message: 'test' }, 111);
+      }
+      expect((scope as any)._breadcrumbs).toHaveLength(100);
+    });
+
     test('setLevel', () => {
       const scope = new Scope();
       scope.setLevel(Severity.Critical);
@@ -181,7 +197,7 @@ describe('Scope', () => {
       scope.setFingerprint(['abcd']);
       scope.setLevel(Severity.Warning);
       scope.setTransactionName('/abc');
-      scope.addBreadcrumb({ message: 'test' }, 100);
+      scope.addBreadcrumb({ message: 'test' });
       scope.setContext('os', { id: '1' });
       const event: Event = {};
       return scope.applyToEvent(event).then(processedEvent => {
@@ -203,7 +219,7 @@ describe('Scope', () => {
       scope.setTag('a', 'b');
       scope.setUser({ id: '1' });
       scope.setFingerprint(['abcd']);
-      scope.addBreadcrumb({ message: 'test' }, 100);
+      scope.addBreadcrumb({ message: 'test' });
       scope.setContext('server', { id: '2' });
       const event: Event = {
         breadcrumbs: [{ message: 'test1' }],
@@ -358,7 +374,7 @@ describe('Scope', () => {
     scope.setTag('a', 'b');
     scope.setUser({ id: '1' });
     scope.setFingerprint(['abcd']);
-    scope.addBreadcrumb({ message: 'test' }, 100);
+    scope.addBreadcrumb({ message: 'test' });
     scope.setRequestSession({ status: RequestSessionStatus.Ok });
     expect((scope as any)._extra).toEqual({ a: 2 });
     scope.clear();
@@ -368,7 +384,7 @@ describe('Scope', () => {
 
   test('clearBreadcrumbs', () => {
     const scope = new Scope();
-    scope.addBreadcrumb({ message: 'test' }, 100);
+    scope.addBreadcrumb({ message: 'test' });
     expect((scope as any)._breadcrumbs).toHaveLength(1);
     scope.clearBreadcrumbs();
     expect((scope as any)._breadcrumbs).toHaveLength(0);