Add support for Next.js cache instrumentation

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.

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

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

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 🤔

...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

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.

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.

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.

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

feat(nextjs): Record cache spans for `use cache` functions (#24302)

feat(nextjs): Link `use cache` hits to their fill across instances (#…

Navigation Menu

Repository navigation

Add support for Next.js cache instrumentation #12888

Problem Statement

Solution Brainstorm

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Add support for Next.js cache instrumentation #12888

Description

Problem Statement

Solution Brainstorm

Activity

AbhiPrasad commented on Jul 11, 2024

AbhiPrasad commented on Jul 11, 2024

timfish commented on Jul 29, 2024

timfish commented on Jul 29, 2024

timfish commented on Jul 29, 2024

AbhiPrasad commented on Jul 30, 2024

lforst commented on Jul 30, 2024

timfish commented on Jul 30, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions