Add support for Next.js cache instrumentation

[AbhiPrasad]

Problem Statement

We support manually instrumenting caching, but this is a pain to figure out: https://docs.sentry.io/platforms/javascript/guides/nextjs/tracing/instrumentation/custom-instrumentation/caches-module/

Solution Brainstorm

We should either
a) auto-instrument the Next.js cache via https://nextjs.org/docs/app/api-reference/next-config-js/incrementalCacheHandlerPath
b) update the cache module docs if auto-instrumentation is not possible.

[AbhiPrasad]

ref https://twitter.com/nhducit/status/1811459207847416085

[AbhiPrasad]

asking in nextjs discussions about extending default cache handler: vercel/next.js#67677

[timfish]

So some initial thoughts after a brief look...

We need to create some kind of cache wrapper that adds the instrumentation but this will only work for self-hosted. I guess hosted on Vercel is out of scope for this issue?

Passing the cacheHandler via require.resolve feels like a strange choice since it means that you can't just pass an already configured class instance 🤔. I need to check how the existing cache implementations accept their config and get to the bottom of why cacheHandler is a path before I can work out how wrapping this might work!

module.exports = {
  cacheHandler: require.resolve('./cache-handler.js'),
  cacheMaxMemorySize: 0, // disable default in-memory caching
}

I guess the default cache could be instrumented via require-in-the-middle of FileSystemCache 🤔

[timfish]

...and whereas the Sentry docs warn about possible performance issues with JSON.stringify(data).length, the next.js code doesn't seem to have a problem with doing this, at least for the fetch cache:
https://github.com/vercel/next.js/blob/a0f83f8271a34cbd3fb531cffb7107aa8adb5e52/packages/next/src/server/lib/incremental-cache/index.ts#L504-L505

[timfish]

Ok so it loads the cache implementation here:
https://github.com/vercel/next.js/blob/a0f83f8271a34cbd3fb531cffb7107aa8adb5e52/packages/next/src/export/helpers/create-incremental-cache.ts#L30-L34

  if (cacheHandler) {
    CacheHandler = interopDefault(
      await import(formatDynamicImportPath(dir, cacheHandler)).then(
        (mod) => mod.default || mod
      )
    )
  }

mod.default || mod isn't actually required because that's exactly what interopDefault does.

Passing around paths to modules like this is very inflexible so I'm hoping there's a good reason for it!

So if we offered a solution that wraps the handler at this config point:

• It would need to return a path*
• We'd need to find a way to pass the users cache handler path to us*
• When we resolve the the users cache handler, we'd need to have the same absolute/relative resolution
  (* in a way the bundler picks up if that's required)

Assuming no bundler issues, the best I've got at this late hour is passing the user handler path via an environment variable 😂

module.exports = {
  cacheHandler: sentryCacheWrapper(require.resolve('./cache-handler.js')),
  cacheMaxMemorySize: 0, // disable default in-memory caching
}

export function sentryCacheWrapper(userHandler: string): string {
  process.env.__SENTRY_USER_CACHE_HANDLER_PATH = userHandler;
  return require.resolve('./sentry-cache-handler.cjs');
}

sentry-cache-handler.cjs

const userHandler = require(process.env.__SENTRY_USER_CACHE_HANDLER_PATH);

export default class SentryCacheHandler {
  function get(key) {
    // do instrumentation here, etc
    // return userHandler.get(key);
  }
}

It might be easier to use require-in-the-middle and wrap the IncrementalCache which is what wraps both the default cache, or any user supplied cache. I guess these are all internals so they might not be stable enough to do this.

[AbhiPrasad]

JSON.stringify(data).length, the next.js code doesn't seem to have a problem with doing this, at least for the fetch cache

This would be worrying with arbitrary data (especially since stringifying certain primitives like booleans are not a good for the real world size), but because data is constrained to be IncrementalCacheValue, which seem to objects of fixed-key number, so should actually be pretty decently fast.

Though looking at https://github.com/vercel/next.js/blob/fd5581d7ef58e4e925485b8b3a2b30ea8a482732/packages/next/src/server/response-cache/types.ts#L87, I guess stringifying some of these fields buffer can get expensive. Maybe an LRU here? Would be interesting to profile IMO.

It might be easier to use require-in-the-middle and wrap the IncrementalCache which is what wraps both the default cache, or any user supplied cache. I guess these are all internals so they might not be stable enough to do this.

I'm leaning toward this option as long as we only wrap get and set - I don't think that part of the public API will change given this seems to be the public API surface of custom caches as well.

[lforst]

I think I like the approach of using require-in-the-middle but I would probably prefer wrapping the user's cache handler directly over wrapping IncrementalCache.

[timfish]

I would probably prefer wrapping the user's cache handler directly over wrapping IncrementalCache

Yep, probably less prone to breakage and it looks like IncrementalCache is also used for the fetch cache