fix(sentry): stop billing spans that declared a parent as segments

shared/lib/trace.test.ts

@@ -512,6 +512,61 @@ describe('Trace', () => {
         expect.any(Function),
       );
     });
+
+    // Regression test for MetaMask-planning#7569.
+    //
+    // The case above covers a map-lookup HIT. This covers the MISS: an
+    // in-process parent whose span is not in `tracesByKey` (it ended, or was
+    // never registered). `resolveParentSpan` returns `null` for that, which is
+    // the same value it returns for "no parent intended" — so `startSpan`
+    // cannot tell the two apart, sees the serialized ids, and routes to
+    // `continueTrace` with `parentSpan: undefined`. That promotes the span to a
+    // segment, and a segment is a billed transaction.
+    //
+    // Measured consequence: ~44.6% of extension transactions carry a
+    // `parent_span` id while being billed as roots, across 25 families.
+    //
+    it('does not promote an unresolvable in-process parent to a segment', () => {
+      continueTraceMock.mockImplementation((_opts, fn) => fn());
+
+      // No pending trace is created, so the map lookup misses.
+      trace(
+        {
+          name: TraceName.Middleware,
+          parentContext: {
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            _name: TraceName.Transaction,
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            _id: 'absent-from-map',
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            _traceId: 'trace123',
+            // eslint-disable-next-line @typescript-eslint/naming-convention
+            _spanId: 'span456',
+          },
+        },
+        () => true,
+      );
+
+      // A parent was named. Failing to resolve it locally must not silently
+      // convert the span into a billed root.
+      expect(continueTraceMock).not.toHaveBeenCalled();
+    });
+
+    // `undefined` and `null` are different statements; only the first may be
+    // promoted to a transaction. See MetaMask/MetaMask-planning#7569.
+    it('does not promote a null parentContext to a segment', () => {
+      const activeSpanMock = {
+        spanContext: jest.fn(),
+      } as unknown as Sentry.Span;
+      getActiveSpanMock.mockReturnValue(activeSpanMock);
+
+      trace({ name: NAME_MOCK, parentContext: null }, () => true);
+
+      expect(startSpanMock).toHaveBeenCalledWith(
+        expect.objectContaining({ forceTransaction: undefined }),
+        expect.any(Function),
+      );
+    });
   });
 
   describe('getSerializedTraceContext', () => {

shared/lib/trace.ts

@@ -483,7 +483,9 @@ function isValidSentrySpan(value: unknown): value is Sentry.Span {
  * @returns Resolved Sentry Span or null.
  */
 function resolveParentSpan(parentContext: unknown): Sentry.Span | null {
-  if (!parentContext) {
+  // Nullish, not falsy: `TraceContext` is `unknown`, so `0` / `''` / `false`
+  // reach here, and none of them state "no parent was declared".
+  if (parentContext === null || parentContext === undefined) {
     return null;
   }
 
@@ -508,6 +510,22 @@ function resolveParentSpan(parentContext: unknown): Sentry.Span | null {
   return null;
 }
 
+/**
+ * Whether a parent context names a trace this process started and expects to
+ * resolve from `tracesByKey`. Such a context is not a cross-process
+ * continuation, even when it also carries serialized ids for propagation.
+ *
+ * @param value - Candidate parent context.
+ * @returns True when the context names an in-process parent.
+ */
+function hasInProcessIdentity(value: unknown): value is { _name: string } {
+  return (
+    isObject(value) &&
+    hasProperty(value, '_name') &&
+    typeof value._name === 'string'
+  );
+}
+
 function hasDistributedTraceIds(
   value: unknown,
 ): value is { _traceId: string; _spanId: string } {
@@ -529,13 +547,26 @@ function startSpan<T>(
   const { data: attributes, name, parentContext, startTime, op } = request;
   let parentSpan = resolveParentSpan(parentContext);
 
+  // `undefined` and `null` are different statements and must not be collapsed:
+  //
+  //   undefined -> no parent was declared. Inheriting the ambient span and
+  //                promoting to a transaction is the intended behaviour.
+  //   null      -> a parent WAS intended but no span exists for it.
+  //                `traceCallback` is typed `(span: Sentry.Span | null)`, so a
+  //                caller receives `null` whenever the SDK created no span and
+  //                passes it straight back. Promoting that to a transaction
+  //                bills a root for what the caller asked to be a child.
+  //
+  // See https://github.com/MetaMask/MetaMask-planning/issues/7569
+  const parentDeclared = parentContext !== undefined;
+
   // Inherit from active span (e.g. browserTracingIntegration's pageload/navigation)
   // when no explicit parent is provided. Must capture before withIsolationScope
   // severs the active span context chain.
   // forceTransaction preserves transaction-level visibility for monitoring while
   // linking to the auto-instrumentation hierarchy.
   let forceTransaction: boolean | undefined;
-  if (!parentSpan && !parentContext) {
+  if (!parentSpan && !parentDeclared) {
     const activeSpan = sentryGetActiveSpan();
     if (activeSpan) {
       parentSpan = activeSpan;
@@ -554,7 +585,14 @@ function startSpan<T>(
 
   // Cross-process propagation via continueTrace when we have serialized
   // trace/span IDs but couldn't resolve a local parent span from the map.
-  if (!parentSpan && hasDistributedTraceIds(parentContext)) {
+  // Only a context WITHOUT in-process identity is a genuine cross-process
+  // continuation. One that names a local trace was meant to resolve from the
+  // map; routing a lookup miss here mints a segment, and a segment is billed.
+  if (
+    !parentSpan &&
+    hasDistributedTraceIds(parentContext) &&
+    !hasInProcessIdentity(parentContext)
+  ) {
     const sentryTrace = `${parentContext._traceId}-${parentContext._spanId}-1`;
     return sentryContinueTrace(sentryTrace, () =>
       sentryWithIsolationScope((scope: Sentry.Scope) => {
@@ -564,6 +602,20 @@ function startSpan<T>(
     );
   }
 
+  // A parent was declared (`null`, or an in-process context that missed the
+  // map) but did not resolve. Nest under whatever is active rather than
+  // starting a new segment — and deliberately WITHOUT `forceTransaction`, since
+  // a failed lookup is not a statement that this operation is a root.
+  if (!parentSpan && parentDeclared) {
+    const activeSpan = sentryGetActiveSpan();
+    if (activeSpan) {
+      return sentryWithIsolationScope((scope: Sentry.Scope) => {
+        initScope(scope, request);
+        return callback({ ...spanOptions, parentSpan: activeSpan });
+      });
+    }
+  }
+
   return sentryWithIsolationScope((scope: Sentry.Scope) => {
     initScope(scope, request);
     return callback(spanOptions);