-
Notifications
You must be signed in to change notification settings - Fork 35
feat: experimental tracing channel support #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0ca8e01 to
4c49d41
Compare
📝 WalkthroughWalkthroughThis PR introduces a new tracing plugin system for srvx that instruments middleware and fetch handlers using Node's diagnostics_channel API. Changes include the core tracing implementation, build configuration updates, documentation additions, comprehensive test coverage, and an example project demonstrating the feature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
a89a68d to
c8e96a0
Compare
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
pi0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start 🚀
I have made a few refactors:
- Plugin simplified and accepts an init options object
- Tracing channel is conditionally loaded. So if unavailable, it won't break
- Merged events format (ideas welcome until we make it stable on structure!)
- Added a dummy example to show how to subscribe
Let's iterate and see how integration of this looks like with higher level like nitro and sentry SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
examples/tracing/package.json (1)
1-11: Consider pinningsrvxinstead of"latest"for reproducible examplesUsing
"srvx": "latest"means the example behavior can drift over time as new versions are released. For more reproducible starter templates (especially when users scaffold from a specific commit or tag), consider pinning to a caret range like^0.9.7or similar, or documenting that this example intentionally tracks the latest release.test/tracing.test.ts (1)
1-443: Ensureunsubscribeuses the same subscriber object and consider a small helper to DRY testsThe test coverage here is solid and exercises fetch, middleware, async, and error paths well. One subtle point:
For each channel, you do:
const channel = tracingChannel("srvx.middleware"); const startHandler = /* ... */; const endHandler = /* ... */; channel.subscribe({ start: startHandler, end: endHandler, asyncStart: noop, asyncEnd: noop, error: noop, }); cleanupFns.push(() => { channel.unsubscribe({ start: startHandler, end: endHandler, asyncStart: noop, asyncEnd: noop, error: noop, }); });If
tracingChannel().unsubscribeexpects the same subscriber object instance that was passed tosubscribe, theseunsubscribecalls won’t actually detach the handlers, and you’ll accumulate live subscriptions across tests (each still closing over its owneventsarray). This might not fail tests immediately but can lead to leaks and surprising behavior.Two suggestions:
Reuse the same subscriber object so
unsubscribeis guaranteed to match what was subscribed:const subscriber = { start: startHandler, end: endHandler, asyncStart: noop, asyncEnd: noop, error: noop, }; channel.subscribe(subscriber); cleanupFns.push(() => { channel.unsubscribe(subscriber); });Optionally introduce a tiny helper to reduce repetition and centralize this pattern:
type Subscriber = Parameters< ReturnType<typeof tracingChannel>["subscribe"] >[0]; function subscribeWithCleanup( name: string, subscriber: Subscriber, cleanupFns: Array<() => void>, ) { const channel = tracingChannel(name); channel.subscribe(subscriber); cleanupFns.push(() => channel.unsubscribe(subscriber)); return channel; }Then in tests:
const subscriber = { start: startHandler, end: endHandler, asyncStart: noop, asyncEnd: noop, error: noop }; const fetchChannel = subscribeWithCleanup("srvx.fetch", subscriber, cleanupFns);This keeps the lifecycle explicit and guards against any identity-based behavior in the diagnostics/tracing APIs.
Please double‑check Node’s
diagnostics_channel.tracingChannelsubscribe/unsubscribecontract to confirm whether it keys unsubscription by subscriber object identity; if it does, the refactor above becomes important to avoid leaks.examples/tracing/server.ts (1)
1-45: Example should match the defensive pattern used insrc/tracing.tsThe
debugChannelfunction in this example does not guard against missingprocess.getBuiltinModuleordiagnostics_channel.tracingChannel, unlike the actualtracingPluginimplementation insrc/tracing.ts(which usesglobalThis.process?.getBuiltinModule?.("node:diagnostics_channel")). While all supported Node versions (>=20.16.0, perpackage.json) have both APIs available, the example should match the defensive pattern for consistency and to serve as better documentation of the recommended approach.Update
debugChannelto guard against missing APIs and makeserializeDatahandle unexpected payloads:-function debugChannel(name: string) { - const { tracingChannel } = process.getBuiltinModule( - "node:diagnostics_channel", - ); +function debugChannel(name: string) { + const { tracingChannel } = + globalThis.process?.getBuiltinModule?.("node:diagnostics_channel") || {}; + if (!tracingChannel) { + // diagnostics tracing not available; skip debug wiring + return; + } @@ - const noop = () => {}; - const serializeData = (data: any) => - Object.entries(data) - .map(([key, value]) => { + const noop = () => {}; + const serializeData = (data: any) => + data && typeof data === "object" + ? Object.entries(data).map(([key, value]) => { if (key === "request") { return `request(url=${(value as Request).url})`; } if (key === "server") { return "server"; } if (key === "result") { return `result(status=${(value as Response).status})`; } return `${key}=${value}`; - }) - .join(", "); + }).join(", ") + : String(data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
README.md(1 hunks)build.config.mjs(1 hunks)docs/1.guide/1.index.md(1 hunks)examples/tracing/package.json(1 hunks)examples/tracing/server.ts(1 hunks)package.json(1 hunks)src/tracing.ts(1 hunks)test/tracing.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/tracing.test.ts (2)
src/tracing.ts (1)
tracingPlugin(35-84)src/types.ts (1)
ServerMiddleware(40-43)
src/tracing.ts (1)
src/types.ts (4)
Server(189-257)ServerRequest(303-328)ServerMiddleware(40-43)ServerPlugin(45-45)
examples/tracing/server.ts (2)
src/tracing.ts (1)
tracingPlugin(35-84)src/log.ts (1)
log(9-23)
🔇 Additional comments (5)
build.config.mjs (1)
10-24: Includingsrc/tracing.tsin bundle inputs looks correctAdding
"src/tracing.ts"to the bundle inputs is consistent with the new./tracingexport and ensuresdist/tracing.mjsis produced as part of the build. No further changes needed here.docs/1.guide/1.index.md (1)
90-103: Tracing starter example row looks consistentThe new
tracingstarter example entry matches the existing examples’ pattern (source path +gigetcommand) and is aligned with theexamples/tracingdirectory. No issues from a docs perspective.README.md (1)
52-64: README tracing example entry is aligned and clearThe added
tracingrow in the Starter Examples table matches the docs entry and theexamples/tracinglayout. Good way to surface the new feature.src/tracing.ts (1)
8-83: Tracing plugin implementation matches the intended behavior and looks solid
- The
RequestEventshape (server + request + optional{ index, handler }middleware descriptor) aligns with how tests consume the tracing payload.- The plugin defensively no-ops when
diagnostics_channel.tracingChannelisn’t available, which avoids hard Node coupling for non-Node runtimes.- Fetch and middleware wrapping via
tracePromiseis clean, and the in-placemiddleware.splice(...)preserves any existing references to the middleware array while still replacing handlers with wrapped versions.Overall this is a good, minimal surface for consumers to build diagnostics integrations on.
If you haven’t already, it’s worth validating against the current Node
diagnostics_channel/tracingChanneldocs thattracePromiseemits the expectedstart/end/asyncStart/asyncEnd/errorevents with the context object you’re passing, ensuring long‑term compatibility.package.json (1)
10-30: The./tracingexport is properly configured.The export path in
package.jsonis correctly wired:
"./tracing": "./dist/tracing.mjs"is defined in exportssrc/tracing.tsexists and properly exports bothtracingPluginandRequestEventbuild.config.mjsexplicitly includessrc/tracing.tsin the entries list, sodist/tracing.mjswill be generated- Type definitions are centralized in
dist/types.d.mts(configured viatsconfig.jsonwithisolatedDeclarations: true)- Usage in
examples/tracing/server.tsandtest/tracing.test.tsconfirms the import path works as expectedNo changes needed; the build will generate the artifact and types correctly.
This PR adds tracing instrumentation for middleware and fetch handlers using Node.js diagnostics/tracing channels, enabling integration with observability tools like OpenTelemetry and Sentry.
Implementation
srvx.middlewareandsrvx.fetchtracing events with full lifecycle hooks (start, end, asyncStart, asyncEnd, error)I think this has to be available out of the box, so it "just works" with SDK providers rather than OPT in.
Example usage
Span Relationships
Something I noticed is since we have an onion effect here with each middleware being able to wait for the response/next, then it means if the SDK provider isn't careful, they might end up creating middleware spans as children of one another rather than siblings.
Now this can be fine since it is technically correct from an execution standpoint, but each provider can handle this manually when subscribing to those diagnostic events, and they can manually unscope each span from the previous one to get the desired effect.
TODO:
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.