refactor(next): reuse the shared middleware pipeline - #474
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 4 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 01557cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for following the naming conventions! 🙏 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
7b86d5e to
db05d32
Compare
507ee53 to
537dbbb
Compare
db05d32 to
7b4610b
Compare
537dbbb to
a4d458c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…rations `evlog/nestjs`, `evlog/react-router` and `evlog/sveltekit` called `createMiddlewareLogger()` directly and rebuilt by hand what `defineFrameworkIntegration()` already does: request extraction, the `crypto.randomUUID()` request-id fallback, `attachForkToLogger()` and the `storage.run()` wrapper. Hono, Express, Elysia, Fastify and oRPC were already on the helper. Behaviour is unchanged; they now inherit whatever the helper gains next, including `extractWaitUntil`. Adds `pickBaseEvlogOptions()` to the toolkit as the single place listing the `BaseEvlogOptions` fields. `toMiddlewareOptions()` and `evlog/eve` both duplicated that list; eve's copy omitted `waitUntil` and would have omitted any option added afterwards. `evlog/eve` keeps its own turn lifecycle — it is not an HTTP middleware, so it only adopts the shared option projection, not the integration helper.
7b4610b to
e4a3081
Compare
`evlog/next` reimplemented the request pipeline instead of calling `createMiddlewareLogger`: its own `shouldLog` / `getServiceForPath` calls, its own tail sampling, its own `emitRequestEvent` and `callEnrichAndDrain`. Keeping a second copy meant it drifted, and two options declared on `NextEvlogOptions` did nothing at all: - `plugins` was never applied — Next built no plugin runner - global `sampling.keep` conditions were never evaluated. Next called only the user's `keep` callback, never `shouldKeep()`, and its hand-rolled tail context carried no `duration`, so duration-based keep rules could not match even in principle `withEvlog` now goes through `defineFrameworkIntegration`, like every other integration. Next's `after()` is resolved once and passed as the pipeline's `waitUntil`, so drain work still runs after the response is sent. Behaviour change: enrich now runs before the response returns rather than inside `after()`. That is the documented `waitUntil` contract and matches every other integration; drain remains deferred. All 73 existing Next tests pass unmodified. Adds three specs covering the two gaps above and `ctx.duration` in the keep callback.
a4d458c to
b44b283
Compare
1 participant
What
evlog/nextwas the last integration reimplementing the request pipeline instead of reusing it. It had its ownshouldLog/getServiceForPathcalls, its own tail sampling, its ownemitRequestEvent, and its owncallEnrichAndDrain— a parallel copy ofcreateMiddlewareLogger+runEnrichAndDrain.withEvlognow goes throughdefineFrameworkIntegration, like every other integration. Net −110 lines.Two options that silently did nothing
Keeping a second copy of the pipeline meant Next drifted from the shared one. Both of these are declared on
NextEvlogOptions(which extendsBaseEvlogOptions) and were accepted without complaint:pluginswas never applied. Next built no plugin runner at all. Passingplugins: [...]tocreateEvlog()did nothing — noenrich, nodrain, no lifecycle hooks.sampling.keepconditions were never evaluated. Next called the user'skeepcallback but nevershouldKeep(). And its hand-rolled tail context carried noduration, so asampling: { keep: [{ duration: 500 }] }rule could not match even in principle.Both now work, and
keepcallbacks receivectx.duration. Error statuses are derived through the sharedextractErrorStatusrather than an inlinestatus ?? statusCode ?? 500.Next's
after()is the platform'swaitUntil— it is now wired as exactly that, so drain work still runs after the response is sent.But the old code passed both enrich and drain to
after(). The shared pipeline awaitsenrichinline and defers onlydrain, per the documentedwaitUntilcontract:If you have a slow async enricher, this moves that cost onto the response's critical path. The fix is to move that work into
drain, which stays deferred.I chose consistency with the other seven integrations over preserving Next's private timing, but this is the one judgment call in the series worth a second opinion — happy to invert it if you'd rather keep enrich deferred on Next.
Verification
ctx.durationreaches the keep callback,sampling.keepconditions firepnpm run test— 1631/1631 passpnpm run lint— 0 errors (2 pre-existingmax-paramswarnings innitro-v3/plugin.ts)pnpm run typecheck— 26/26 tasks passpnpm run api:snapshot— unchanged, Next's public exports are identicalPreserved untouched: instrumentation/
configureHandlerinterplay,unstable_rethrownavigation-signal handling (#436), dev error-stack enrichment, thex-evlog-startmiddleware header,EvlogErrorJSON responses, streaming defer (#321), and server-action support (non-Requestfirst argument).