feat(core): Extract scope application to util (#9804)

This is a try to move the application of scope data to events out of the
scope class into a util function.

This changes the flow to be:

1. The scope has a `getScopeData()` method that returns data that should
be applied to events.
2. The `prepareEvent` method uses that to actually apply scope data to
the event etc.
3. This also makes it much easier to do the experimentation in
node-experimental
(#9799) because we
only need to overwrite this, and can leave the rest of the event
processing the same.

I _think_ this makes sense but would appreciate some more eyes on this
as well. For me the separation makes also more sense, as e.g. the
application of event processors etc. should not really be tied to the
scope at all.

This is also is a first step to then allow us to add global scopes more
easily.

---------

Co-authored-by: Lukas Stracke <[email protected]>

Author: mydea

packages/core/src/index.ts

@@ -63,6 +63,7 @@ export {
   convertIntegrationFnToClass,
 } from './integration';
 export { FunctionToString, InboundFilters, LinkedErrors } from './integrations';
+export { applyScopeDataToEvent } from './utils/applyScopeDataToEvent';
 export { prepareEvent } from './utils/prepareEvent';
 export { createCheckInEnvelope } from './checkin';
 export { hasTracingEnabled } from './utils/hasTracingEnabled';

packages/core/src/scope.ts

@@ -15,6 +15,7 @@ import type {
   RequestSession,
   Scope as ScopeInterface,
   ScopeContext,
+  ScopeData,
   Session,
   Severity,
   SeverityLevel,
@@ -26,6 +27,7 @@ import { arrayify, dateTimestampInSeconds, isPlainObject, uuid4 } from '@sentry/
 
 import { getGlobalEventProcessors, notifyEventProcessors } from './eventProcessors';
 import { updateSession } from './session';
+import { applyScopeDataToEvent } from './utils/applyScopeDataToEvent';
 
 /**
  * Default value for maximum number of breadcrumbs added to an event.
@@ -466,78 +468,65 @@ export class Scope implements ScopeInterface {
     return this;
   }
 
+  /** @inheritDoc */
+  public getScopeData(): ScopeData {
+    const {
+      _breadcrumbs,
+      _attachments,
+      _contexts,
+      _tags,
+      _extra,
+      _user,
+      _level,
+      _fingerprint,
+      _eventProcessors,
+      _propagationContext,
+      _sdkProcessingMetadata,
+      _transactionName,
+      _span,
+    } = this;
+
+    return {
+      breadcrumbs: _breadcrumbs,
+      attachments: _attachments,
+      contexts: _contexts,
+      tags: _tags,
+      extra: _extra,
+      user: _user,
+      level: _level,
+      fingerprint: _fingerprint || [],
+      eventProcessors: _eventProcessors,
+      propagationContext: _propagationContext,
+      sdkProcessingMetadata: _sdkProcessingMetadata,
+      transactionName: _transactionName,
+      span: _span,
+    };
+  }
+
   /**
    * Applies data from the scope to the event and runs all event processors on it.
    *
    * @param event Event
    * @param hint Object containing additional information about the original exception, for use by the event processors.
    * @hidden
+   * @deprecated Use `applyScopeDataToEvent()` directly
    */
   public applyToEvent(
     event: Event,
     hint: EventHint = {},
-    additionalEventProcessors?: EventProcessor[],
+    additionalEventProcessors: EventProcessor[] = [],
   ): PromiseLike<Event | null> {
-    if (this._extra && Object.keys(this._extra).length) {
-      event.extra = { ...this._extra, ...event.extra };
-    }
-    if (this._tags && Object.keys(this._tags).length) {
-      event.tags = { ...this._tags, ...event.tags };
-    }
-    if (this._user && Object.keys(this._user).length) {
-      event.user = { ...this._user, ...event.user };
-    }
-    if (this._contexts && Object.keys(this._contexts).length) {
-      event.contexts = { ...this._contexts, ...event.contexts };
-    }
-    if (this._level) {
-      event.level = this._level;
-    }
-    if (this._transactionName) {
-      event.transaction = this._transactionName;
-    }
-
-    // We want to set the trace context for normal events only if there isn't already
-    // a trace context on the event. There is a product feature in place where we link
-    // errors with transaction and it relies on that.
-    if (this._span) {
-      event.contexts = { trace: this._span.getTraceContext(), ...event.contexts };
-      const transaction = this._span.transaction;
-      if (transaction) {
-        event.sdkProcessingMetadata = {
-          dynamicSamplingContext: transaction.getDynamicSamplingContext(),
-          ...event.sdkProcessingMetadata,
-        };
-        const transactionName = transaction.name;
-        if (transactionName) {
-          event.tags = { transaction: transactionName, ...event.tags };
-        }
-      }
-    }
-
-    this._applyFingerprint(event);
-
-    const scopeBreadcrumbs = this._getBreadcrumbs();
-    const breadcrumbs = [...(event.breadcrumbs || []), ...scopeBreadcrumbs];
-    event.breadcrumbs = breadcrumbs.length > 0 ? breadcrumbs : undefined;
-
-    event.sdkProcessingMetadata = {
-      ...event.sdkProcessingMetadata,
-      ...this._sdkProcessingMetadata,
-      propagationContext: this._propagationContext,
-    };
+    applyScopeDataToEvent(event, this.getScopeData());
 
     // TODO (v8): Update this order to be: Global > Client > Scope
-    return notifyEventProcessors(
-      [
-        ...(additionalEventProcessors || []),
-        // eslint-disable-next-line deprecation/deprecation
-        ...getGlobalEventProcessors(),
-        ...this._eventProcessors,
-      ],
-      event,
-      hint,
-    );
+    const eventProcessors: EventProcessor[] = [
+      ...additionalEventProcessors,
+      // eslint-disable-next-line deprecation/deprecation
+      ...getGlobalEventProcessors(),
+      ...this._eventProcessors,
+    ];
+
+    return notifyEventProcessors(eventProcessors, event, hint);
   }
 
   /**
@@ -564,13 +553,6 @@ export class Scope implements ScopeInterface {
     return this._propagationContext;
   }
 
-  /**
-   * Get the breadcrumbs for this scope.
-   */
-  protected _getBreadcrumbs(): Breadcrumb[] {
-    return this._breadcrumbs;
-  }
-
   /**
    * This will be called on every set call.
    */
@@ -586,25 +568,6 @@ export class Scope implements ScopeInterface {
       this._notifyingListeners = false;
     }
   }
-
-  /**
-   * Applies fingerprint from the scope to the event if there's one,
-   * uses message if there's one instead or get rid of empty fingerprint
-   */
-  private _applyFingerprint(event: Event): void {
-    // Make sure it's an array first and we actually have something in place
-    event.fingerprint = event.fingerprint ? arrayify(event.fingerprint) : [];
-
-    // If we have something on the scope, then merge it with event
-    if (this._fingerprint) {
-      event.fingerprint = event.fingerprint.concat(this._fingerprint);
-    }
-
-    // If we have no data at all, remove empty array default
-    if (event.fingerprint && !event.fingerprint.length) {
-      delete event.fingerprint;
-    }
-  }
 }
 
 function generatePropagationContext(): PropagationContext {

packages/core/src/utils/applyScopeDataToEvent.ts

@@ -0,0 +1,97 @@
+import type { Breadcrumb, Event, PropagationContext, ScopeData, Span } from '@sentry/types';
+import { arrayify } from '@sentry/utils';
+
+/**
+ * Applies data from the scope to the event and runs all event processors on it.
+ */
+export function applyScopeDataToEvent(event: Event, data: ScopeData): void {
+  const { fingerprint, span, breadcrumbs, sdkProcessingMetadata, propagationContext } = data;
+
+  // Apply general data
+  applyDataToEvent(event, data);
+
+  // We want to set the trace context for normal events only if there isn't already
+  // a trace context on the event. There is a product feature in place where we link
+  // errors with transaction and it relies on that.
+  if (span) {
+    applySpanToEvent(event, span);
+  }
+
+  applyFingerprintToEvent(event, fingerprint);
+  applyBreadcrumbsToEvent(event, breadcrumbs);
+  applySdkMetadataToEvent(event, sdkProcessingMetadata, propagationContext);
+}
+
+function applyDataToEvent(event: Event, data: ScopeData): void {
+  const { extra, tags, user, contexts, level, transactionName } = data;
+
+  if (extra && Object.keys(extra).length) {
+    event.extra = { ...extra, ...event.extra };
+  }
+  if (tags && Object.keys(tags).length) {
+    event.tags = { ...tags, ...event.tags };
+  }
+  if (user && Object.keys(user).length) {
+    event.user = { ...user, ...event.user };
+  }
+  if (contexts && Object.keys(contexts).length) {
+    event.contexts = { ...contexts, ...event.contexts };
+  }
+  if (level) {
+    event.level = level;
+  }
+  if (transactionName) {
+    event.transaction = transactionName;
+  }
+}
+
+function applyBreadcrumbsToEvent(event: Event, breadcrumbs: Breadcrumb[]): void {
+  const mergedBreadcrumbs = [...(event.breadcrumbs || []), ...breadcrumbs];
+  event.breadcrumbs = mergedBreadcrumbs.length ? mergedBreadcrumbs : undefined;
+}
+
+function applySdkMetadataToEvent(
+  event: Event,
+  sdkProcessingMetadata: ScopeData['sdkProcessingMetadata'],
+  propagationContext: PropagationContext,
+): void {
+  event.sdkProcessingMetadata = {
+    ...event.sdkProcessingMetadata,
+    ...sdkProcessingMetadata,
+    propagationContext: propagationContext,
+  };
+}
+
+function applySpanToEvent(event: Event, span: Span): void {
+  event.contexts = { trace: span.getTraceContext(), ...event.contexts };
+  const transaction = span.transaction;
+  if (transaction) {
+    event.sdkProcessingMetadata = {
+      dynamicSamplingContext: transaction.getDynamicSamplingContext(),
+      ...event.sdkProcessingMetadata,
+    };
+    const transactionName = transaction.name;
+    if (transactionName) {
+      event.tags = { transaction: transactionName, ...event.tags };
+    }
+  }
+}
+
+/**
+ * Applies fingerprint from the scope to the event if there's one,
+ * uses message if there's one instead or get rid of empty fingerprint
+ */
+function applyFingerprintToEvent(event: Event, fingerprint: ScopeData['fingerprint'] | undefined): void {
+  // Make sure it's an array first and we actually have something in place
+  event.fingerprint = event.fingerprint ? arrayify(event.fingerprint) : [];
+
+  // If we have something on the scope, then merge it with event
+  if (fingerprint) {
+    event.fingerprint = event.fingerprint.concat(fingerprint);
+  }
+
+  // If we have no data at all, remove empty array default
+  if (event.fingerprint && !event.fingerprint.length) {
+    delete event.fingerprint;
+  }
+}

packages/core/src/utils/prepareEvent.ts

@@ -9,19 +9,12 @@ import type {
   StackFrame,
   StackParser,
 } from '@sentry/types';
-import {
-  GLOBAL_OBJ,
-  addExceptionMechanism,
-  dateTimestampInSeconds,
-  normalize,
-  resolvedSyncPromise,
-  truncate,
-  uuid4,
-} from '@sentry/utils';
+import { GLOBAL_OBJ, addExceptionMechanism, dateTimestampInSeconds, normalize, truncate, uuid4 } from '@sentry/utils';
 
 import { DEFAULT_ENVIRONMENT } from '../constants';
 import { getGlobalEventProcessors, notifyEventProcessors } from '../eventProcessors';
 import { Scope } from '../scope';
+import { applyScopeDataToEvent } from './applyScopeDataToEvent';
 
 /**
  * This type makes sure that we get either a CaptureContext, OR an EventHint.
@@ -80,10 +73,13 @@ export function prepareEvent(
     addExceptionMechanism(prepared, hint.mechanism);
   }
 
-  // We prepare the result here with a resolved Event.
-  let result = resolvedSyncPromise<Event | null>(prepared);
-
   const clientEventProcessors = client && client.getEventProcessors ? client.getEventProcessors() : [];
+  // TODO (v8): Update this order to be: Global > Client > Scope
+  const eventProcessors = [
+    ...clientEventProcessors,
+    // eslint-disable-next-line deprecation/deprecation
+    ...getGlobalEventProcessors(),
+  ];
 
   // This should be the last thing called, since we want that
   // {@link Hub.addEventProcessor} gets the finished prepared event.
@@ -102,22 +98,15 @@ export function prepareEvent(
       }
     }
 
-    // In case we have a hub we reassign it.
-    result = finalScope.applyToEvent(prepared, hint, clientEventProcessors);
-  } else {
-    // Apply client & global event processors even if there is no scope
-    // TODO (v8): Update the order to be Global > Client
-    result = notifyEventProcessors(
-      [
-        ...clientEventProcessors,
-        // eslint-disable-next-line deprecation/deprecation
-        ...getGlobalEventProcessors(),
-      ],
-      prepared,
-      hint,
-    );
+    const scopeData = finalScope.getScopeData();
+    applyScopeDataToEvent(prepared, scopeData);
+
+    // Run scope event processors _after_ all other processors
+    eventProcessors.push(...scopeData.eventProcessors);
   }
 
+  const result = notifyEventProcessors(eventProcessors, prepared, hint);
+
   return result.then(evt => {
     if (evt) {
       // We apply the debug_meta field only after all event processors have ran, so that if any event processors modified

packages/feedback/test/unit/util/prepareFeedbackEvent.test.ts

@@ -16,7 +16,7 @@ describe('Unit | util | prepareFeedbackEvent', () => {
     hub.bindClient(client);
 
     client = hub.getClient()!;
-    scope = hub.getScope()!;
+    scope = hub.getScope();
   });
 
   afterEach(() => {