integration improvements and only enabled with spans

Author: s1gr1d

packages/nextjs/src/server/index.ts

@@ -11,6 +11,7 @@ import {
   getRootSpan,
   getVercelEnv,
   GLOBAL_OBJ,
+  hasSpansEnabled,
 } from '@sentry/core';
 import type { NodeClient, NodeOptions } from '@sentry/node';
 import { getDefaultIntegrations, httpIntegration, init as nodeInit } from '@sentry/node';
@@ -28,7 +29,7 @@ import { createLiveRootSpanAdapter } from '../common/utils/liveRootSpanAdapter';
 import { enhanceHandleRequestRootSpan } from './enhanceHandleRequestRootSpan';
 import { handleOnSpanStart } from './handleOnSpanStart';
 import { prepareSafeIdGeneratorContext } from './prepareSafeIdGeneratorContext';
-import { instrumentUseCacheHandlers } from './useCacheInstrumentation';
+import { nextjsUseCacheIntegration } from './useCacheInstrumentation';
 import { maybeCompleteCronCheckIn } from './vercelCronsMonitoring';
 import { maybeCleanupQueueSpan } from './vercelQueuesMonitoring';
 
@@ -142,6 +143,10 @@ export function init(options: NodeOptions): NodeClient | undefined {
     customDefaultIntegrations.push(distDirRewriteFramesIntegration({ distDirName }));
   }
 
+  if (hasSpansEnabled(options)) {
+    customDefaultIntegrations.push(nextjsUseCacheIntegration());
+  }
+
   // Detect if running on OpenNext/Cloudflare and get runtime config
   const cloudflareConfig = getCloudflareRuntimeConfig();
 
@@ -190,8 +195,6 @@ export function init(options: NodeOptions): NodeClient | undefined {
 
   const client = nodeInit(opts);
 
-  instrumentUseCacheHandlers();
-
   client?.on('beforeSampling', ({ spanAttributes }, samplingDecision) => {
     // There are situations where the Next.js Node.js server forwards requests for the Edge Runtime server (e.g. in
     // middleware) and this causes spans for Sentry ingest requests to be created. These are not exempt from our tracing

packages/nextjs/src/server/useCacheInstrumentation.ts

@@ -3,38 +3,45 @@ import { CACHE_ITEM_AGE, CACHE_OPERATION, CACHE_TAGS, CACHE_TTL } from '@sentry/
 import { CACHE_GET, CACHE_PUT } from '@sentry/conventions/op';
 import type { Span } from '@sentry/core';
 import {
-  _INTERNAL_safeDateNow,
   CACHE_OPERATION_NAMES,
   debug,
+  defineIntegration,
   fill,
+  getActiveSpan,
   getClient,
   hasSpanStreamingEnabled,
   SEMANTIC_ATTRIBUTE_CACHE_HIT,
   SEMANTIC_ATTRIBUTE_CACHE_KEY,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  spanIsSampled,
   startSpan,
+  timestampInSeconds,
 } from '@sentry/core';
 import { DEBUG_BUILD } from '../common/debug-build';
 
 // Next.js shares its `use cache` handlers across bundles via `globalThis`
 // (`next/src/server/use-cache/handlers.ts`). This module can load once per bundle, so all
-// double-wrap guards must also live on `globalThis` or on the handler objects.
+// double-wrap guards must also live on `globalThis`.
 const NEXT_CACHE_HANDLERS_MAP = Symbol.for('@next/cache-handlers-map');
 const NEXT_PRIVATE_CACHE_HANDLER = Symbol.for('@next/cache-handlers-private');
 const SENTRY_CACHE_INSTRUMENTED = Symbol.for('sentry.nextjs.cacheHandlersInstrumented');
-const SENTRY_HANDLER_WRAPPED = Symbol.for('sentry.nextjs.wrappedCacheHandler');
+const SENTRY_WRAPPED_HANDLERS = Symbol.for('sentry.nextjs.wrappedCacheHandlers');
 
+const INTEGRATION_NAME = 'NextjsUseCache';
 const CACHE_SPAN_ORIGIN = 'auto.cache.nextjs';
 
 // Next.js' `INFINITE_CACHE` sentinel. An `expire` at or above it means "never expires", which carries no signal as a TTL attribute.
 // https://github.com/vercel/next.js/blob/ed1aab5d386d07ee2f553107dd39995251a6e44e/packages/next/src/lib/constants.ts#L43-L46
 const NEXT_INFINITE_CACHE = 0xfffffffe;
 
+// Next.js' `MIN_PRERENDERABLE_EXPIRE` (seconds). The dev server keeps entries at least this long,
+// even when their `expire` is shorter.
+const NEXT_DEV_MIN_EXPIRE = 300;
+
 // Next.js vendored types below: https://github.com/vercel/next.js/blob/ed1aab5d386d07ee2f553107dd39995251a6e44e/packages/next/src/server/lib/cache-handlers/types.ts
 
 // Subset of Next.js' `CacheHandler`
 interface UseCacheHandler {
-  [SENTRY_HANDLER_WRAPPED]?: boolean;
   get(cacheKey: string, softTags?: string[]): Promise<unknown>;
   set(cacheKey: string, pendingEntry: Promise<unknown>): Promise<void>;
 }
@@ -46,13 +53,14 @@ interface UseCacheEntry {
   /** seconds; hard limit after which the entry is discarded on read */
   expire?: number;
   /** `cacheTag()` tags, excluding implicit soft tags */
-  tags?: string[];
+  tags?: unknown[];
 }
 
 type GlobalWithCacheHandlers = typeof globalThis & {
   [NEXT_CACHE_HANDLERS_MAP]?: Map<string, UseCacheHandler>;
   [NEXT_PRIVATE_CACHE_HANDLER]?: UseCacheHandler;
   [SENTRY_CACHE_INSTRUMENTED]?: boolean;
+  [SENTRY_WRAPPED_HANDLERS]?: WeakSet<object>;
 };
 
 /**
@@ -63,19 +71,25 @@ function keyDigest(cacheKey: string): string {
   return createHash('sha1').update(cacheKey).digest('hex').slice(0, 12);
 }
 
+/**
+ * Cache reads can be hot, so all span work (including key hashing) is skipped without a sampled
+ * parent span. Background revalidations still pass: they parent to the serving request's
+ * (possibly already finished) root span.
+ */
+function shouldRecordCacheSpan(): boolean {
+  const activeSpan = getActiveSpan();
+  return !!activeSpan && spanIsSampled(activeSpan);
+}
+
 function startCacheSpan<T>(op: typeof CACHE_GET | typeof CACHE_PUT, cacheKey: string, callback: (span: Span) => T): T {
   const client = getClient();
   const digest = keyDigest(cacheKey);
 
   return startSpan(
     {
-      // low cardinality name for span streaming
+      // low cardinality name for span streaming, so we can't fall back to the cache key
       name: client && hasSpanStreamingEnabled(client) ? op : digest,
       op,
-      // Without an active parent span (e.g. a detached worker context), a cache span would become
-      // its own orphan transaction. This does not filter background revalidations; those still
-      // parent to the serving request's (possibly already finished) root span.
-      onlyIfParent: true,
       attributes: {
         [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: CACHE_SPAN_ORIGIN,
         [SEMANTIC_ATTRIBUTE_CACHE_KEY]: [digest],
@@ -87,9 +101,20 @@ function startCacheSpan<T>(op: typeof CACHE_GET | typeof CACHE_PUT, cacheKey: st
 }
 
 /**
- * `cache.hit` mirrors Next.js' use-cache wrapper: it discards entries past their hard `expire`
- * limit and re-runs the function, so those count as misses here. Tag-revalidation discards are
- * not detectable at the handler level and still count as hits.
+ * Safety net for custom handlers that return entries past `expire`: Next.js discards those and
+ * re-runs the function. Its default handler already returns no entry in that case.
+ */
+function isExpired(ageMs: number | undefined, expire: number | undefined): boolean {
+  if (ageMs === undefined || typeof expire !== 'number') {
+    return false;
+  }
+  const effectiveExpire = process.env.NODE_ENV === 'development' ? Math.max(expire, NEXT_DEV_MIN_EXPIRE) : expire;
+  return ageMs > effectiveExpire * 1000;
+}
+
+/**
+ * A missing entry is a miss. Next.js' default handler also returns no entry for expired, evicted,
+ * or tag-invalidated entries, so those count as misses too.
  */
 function setEntryAttributes(span: Span, entry: unknown): void {
   if (entry === undefined) {
@@ -98,54 +123,83 @@ function setEntryAttributes(span: Span, entry: unknown): void {
   }
 
   const { timestamp, expire, tags } = (entry ?? {}) as UseCacheEntry;
-  const ageMs = typeof timestamp === 'number' ? _INTERNAL_safeDateNow() - timestamp : undefined;
-  const isExpired = ageMs !== undefined && typeof expire === 'number' && ageMs > expire * 1000;
+  const ageMs = typeof timestamp === 'number' ? timestampInSeconds() * 1000 - timestamp : undefined;
 
-  span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, !isExpired);
+  span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, !isExpired(ageMs, expire));
 
   if (ageMs !== undefined) {
     // Clamped: with a remote handler, the filling and the reading machine's clocks can drift.
     span.setAttribute(CACHE_ITEM_AGE, Math.max(0, Math.round(ageMs / 1000)));
   }
-  if (typeof expire === 'number' && expire < NEXT_INFINITE_CACHE) {
+  // A negative `expire` is Next.js' eviction sentinel, not a TTL.
+  if (typeof expire === 'number' && expire >= 0 && expire < NEXT_INFINITE_CACHE) {
     span.setAttribute(CACHE_TTL, expire);
   }
-  if (Array.isArray(tags) && tags.length > 0) {
-    span.setAttribute(CACHE_TAGS, tags);
+  const stringTags = Array.isArray(tags) ? tags.filter((tag): tag is string => typeof tag === 'string') : [];
+  if (stringTags.length > 0) {
+    span.setAttribute(CACHE_TAGS, stringTags);
   }
 }
 
-function instrumentHandler(handler: unknown): void {
-  if (
-    !handler ||
-    typeof handler !== 'object' ||
-    typeof (handler as UseCacheHandler).get !== 'function' ||
-    typeof (handler as UseCacheHandler).set !== 'function' ||
-    (handler as UseCacheHandler)[SENTRY_HANDLER_WRAPPED]
-  ) {
-    return;
+function isCacheHandler(value: unknown): value is UseCacheHandler {
+  return (
+    typeof value === 'object' &&
+    value !== null &&
+    typeof (value as UseCacheHandler).get === 'function' &&
+    typeof (value as UseCacheHandler).set === 'function'
+  );
+}
+
+// A WeakSet instead of a marker property, because frozen or proxied handlers reject new properties.
+function getWrappedHandlers(): WeakSet<object> {
+  const globalWithCacheHandlers = globalThis as GlobalWithCacheHandlers;
+  if (!globalWithCacheHandlers[SENTRY_WRAPPED_HANDLERS]) {
+    globalWithCacheHandlers[SENTRY_WRAPPED_HANDLERS] = new WeakSet();
   }
+  return globalWithCacheHandlers[SENTRY_WRAPPED_HANDLERS];
+}
 
-  const cacheHandler = handler as UseCacheHandler;
-  cacheHandler[SENTRY_HANDLER_WRAPPED] = true;
+function instrumentHandler(handler: unknown): void {
+  // Runs inside Next.js' handler registration, which must never fail because of Sentry.
+  try {
+    const wrappedHandlers = getWrappedHandlers();
+    if (!isCacheHandler(handler) || wrappedHandlers.has(handler)) {
+      return;
+    }
+    wrappedHandlers.add(handler);
 
-  fill(cacheHandler, 'get', (originalGet: UseCacheHandler['get']) => {
-    return function (this: UseCacheHandler, cacheKey: string, softTags?: string[]): Promise<unknown> {
-      return startCacheSpan(CACHE_GET, cacheKey, async span => {
-        const entry = await originalGet.call(this, cacheKey, softTags);
-        setEntryAttributes(span, entry);
-        return entry;
-      });
-    };
-  });
+    fill(handler, 'get', (originalGet: UseCacheHandler['get']) => {
+      return function (this: UseCacheHandler, cacheKey: string, softTags?: string[]): Promise<unknown> {
+        if (!shouldRecordCacheSpan()) {
+          return originalGet.call(this, cacheKey, softTags);
+        }
+        return startCacheSpan(CACHE_GET, cacheKey, span =>
+          // `Promise.resolve` because custom handlers may return the entry synchronously.
+          Promise.resolve(originalGet.call(this, cacheKey, softTags)).then(entry => {
+            try {
+              setEntryAttributes(span, entry);
+            } catch (error) {
+              DEBUG_BUILD && debug.warn('Failed to read Next.js cache entry metadata', error);
+            }
+            return entry;
+          }),
+        );
+      };
+    });
 
-  fill(cacheHandler, 'set', (originalSet: UseCacheHandler['set']) => {
-    return function (this: UseCacheHandler, cacheKey: string, pendingEntry: Promise<unknown>): Promise<void> {
-      // The handler drains `pendingEntry` (the still-streaming entry) before storing, so this
-      // span covers producing and storing the entry, not just the write.
-      return startCacheSpan(CACHE_PUT, cacheKey, () => originalSet.call(this, cacheKey, pendingEntry));
-    };
-  });
+    fill(handler, 'set', (originalSet: UseCacheHandler['set']) => {
+      return function (this: UseCacheHandler, cacheKey: string, pendingEntry: Promise<unknown>): Promise<void> {
+        if (!shouldRecordCacheSpan()) {
+          return originalSet.call(this, cacheKey, pendingEntry);
+        }
+        // The handler drains `pendingEntry` (the still-streaming entry) before storing, so this
+        // span covers producing and storing the entry, not just the write.
+        return startCacheSpan(CACHE_PUT, cacheKey, () => originalSet.call(this, cacheKey, pendingEntry));
+      };
+    });
+  } catch (error) {
+    DEBUG_BUILD && debug.warn('Failed to instrument a Next.js cache handler', error);
+  }
 }
 
 function instrumentHandlersMap(handlersMap: Map<string, UseCacheHandler>): void {
@@ -156,8 +210,9 @@ function instrumentHandlersMap(handlersMap: Map<string, UseCacheHandler>): void
   // Custom handlers can be registered later (`setCacheHandler`), so wrap new entries as they arrive.
   fill(handlersMap, 'set', (originalSet: Map<string, UseCacheHandler>['set']) => {
     return function (this: Map<string, UseCacheHandler>, kind: string, handler: UseCacheHandler) {
+      const result = originalSet.call(this, kind, handler);
       instrumentHandler(handler);
-      return originalSet.call(this, kind, handler);
+      return result;
     };
   });
 }
@@ -189,12 +244,13 @@ function instrumentWhenAssigned(symbol: symbol, onValue: (value: unknown) => voi
   });
 }
 
-/**
- * Wraps Next.js' `use cache` handlers with `cache.get`/`cache.put` spans, so cached function
- * reads and fills show up in traces with hit/miss information. The registry only exists with
- * `cacheComponents`/`useCache` enabled; otherwise this only installs inert interceptors.
+/** Installs the `use cache` handler instrumentation once per process.
+ *
+ *  Only exported for testing.
+ *
+ *  @internal
  */
-export function instrumentUseCacheHandlers(): void {
+export function _instrumentUseCacheHandlers(): void {
   try {
     const globalWithCacheHandlers = globalThis as GlobalWithCacheHandlers;
 
@@ -215,3 +271,16 @@ export function instrumentUseCacheHandlers(): void {
     DEBUG_BUILD && debug.warn('Failed to instrument Next.js cache handlers', error);
   }
 }
+
+/**
+ * Wraps Next.js' `use cache` handlers with `cache.get`/`cache.put` spans, so cached function
+ * reads and fills show up in traces with hit/miss information.
+ */
+export const nextjsUseCacheIntegration = defineIntegration(() => {
+  return {
+    name: INTEGRATION_NAME,
+    setupOnce() {
+      _instrumentUseCacheHandlers();
+    },
+  };
+});