Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new tracing plugin for H3 using node:diagnostics_channel, exposes a new public export entry "./tracing" in package.json, implements runtime wrapping of middleware and route handlers to emit trace payloads, and adds comprehensive tests and test setup changes to exercise tracing behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
|
Tracing has been implemented as a plugin, now looking at a way to auto trace nested H3 instances, perhaps by calling the plugin recursively whenever an app is mounted. EDIT: Implemented but unsure what to do about fetch handlers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/tracing.ts (2)
131-148: Consider using.at(-1)for cleaner array access.The eslint disable comment suggests using
.at(-1)would be preferred. Since the project targets Node.js >=20.11.1 (per package.json),.at()is fully supported.- // eslint-disable-next-line unicorn/prefer-at - const lastRoute = instance["~routes"][instance["~routes"].length - 1]; + const lastRoute = instance["~routes"].at(-1);
82-99: Consider usingtraceSyncfor synchronous handlers to avoid unnecessary Promise wrapping.The
async () => await middleware(...args)pattern forces all handlers through a Promise, even synchronous ones. Node.jsdiagnostics_channelprovides bothtracePromise(for async operations) andtraceSync(for synchronous operations). UsingtracePromisefor sync handlers adds overhead and changes the execution model unnecessarily.This could be optimized by detecting handler synchronicity and using
traceSyncwhere appropriate, though this would require more complex wrapper logic to distinguish between sync and async handlers at runtime.test/_setup.ts (1)
160-160: Consider simplifying the conditional plugin array.The current pattern works but could be more concise using spread with conditional:
- plugins: [opts.tracing ? tracingPlugin() : undefined].filter((p) => !!p), + plugins: [...(opts.tracing ? [tracingPlugin()] : [])],Alternatively, if
pluginsacceptsundefinedvalues gracefully, the filter could be removed entirely.test/tracing.test.ts (1)
270-296: Minor: Unnecessary manual mock restoration.Line 294 manually assigns
originalOnErrorback tot.hooks.onError, but the test setup'safterEachalready callsvi.resetAllMocks(). This manual restoration is redundant and the pattern of reassigning a mock reference may not work as expected (it replaces the mock object rather than resetting it).} finally { listener.cleanup(); - t.hooks.onError = originalOnError; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/tracing.ts(1 hunks)test/_setup.ts(3 hunks)test/tracing.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/tracing.ts (1)
src/h3.ts (2)
H3Core(36-112)handler(53-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (14)
src/tracing.ts (3)
1-35: LGTM!The type definitions are well-structured. The payload interfaces (
H3InitPayload,H3MountPayload,H3THandlerTracePayload) provide clear contracts for tracing consumers, and theMaybeTracedMiddleware/MaybeTracedEventHandlertypes properly extend the base types with the__traced__marker.
40-78: LGTM!The graceful degradation pattern is well-implemented. The plugin correctly:
- Handles environments where
diagnostics_channelis unavailable- Stores and properly delegates to the original
mountmethod- Publishes events only when subscribers exist (avoiding unnecessary overhead)
150-170: LGTM!The argument parsing correctly handles both
use(middleware, opts)anduse(route, middleware, opts)signatures. The comment explains why middleware is wrapped at the arg level (to avoid tracing H3's internal wrappers), which differs from theonhandler wrapping strategy.test/_setup.ts (2)
8-8: LGTM!Clean import of the new tracing plugin.
189-189: LGTM!The optional
tracingflag inTestOptionsprovides a clean way to enable tracing in specific test suites.package.json (1)
25-29: LGTM!The new
./tracingexport entry follows the established pattern with proper TypeScript types and default module paths. This allows consumers to import tracing functionality separately:import { tracingPlugin } from 'h3/tracing'.test/tracing.test.ts (8)
1-10: LGTM!Clean imports with proper type imports for payload interfaces. The imports from
node:diagnostics_channeland the tracing module are well-organized.
11-25: LGTM!The
TracingEventtype accurately models the tracing channel lifecycle events, andcreateH3WithTracingprovides a clean factory for tests needing a standalone traced H3 instance.
27-70: LGTM!The
createTracingListenerhelper provides proper test isolation with:
- Separate handlers for each lifecycle event
- A cleanup function to unsubscribe all handlers
- Collection of events for assertions
The pattern of returning
{ events, cleanup }enables clean test structure withtry/finally.
75-91: LGTM!Good test structure with proper cleanup in
finallyblock. The test verifies that the init channel fires exactly once and provides the correct app reference.
177-212: LGTM!The test properly verifies that tracing events fire for handlers, checking for asyncStart/asyncEnd events and route-type events. The payload structure verification ensures the event contains required fields.
298-327: LGTM!Good test coverage for middleware tracing. The test verifies:
- Middleware events have
type: "middleware"- Route events have
type: "route"- Multiple middlewares produce expected event counts
377-394: LGTM!This is an important regression test ensuring the tracing instrumentation doesn't affect the normal request/response flow. It validates that response data is correctly returned despite the tracing wrapper.
468-470: LGTM!Properly enables tracing for the entire test matrix via the
{ tracing: true }option.
There was a problem hiding this comment.
Pull request overview
This PR implements tracing channel support for H3 apps, enabling observability instrumentation without monkey patching. The implementation uses Node.js's built-in diagnostics_channel API to emit lifecycle events for handlers and middleware execution.
Key changes:
- Adds a
tracingPluginthat emitsh3.init,h3.mount, andh3.request.handlertracing events - Wraps route handlers and middleware to emit tracing events with full lifecycle hooks (start, end, asyncStart, asyncEnd, error)
- Exports tracing types and plugin via a new
h3/tracingentry point
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
src/tracing.ts |
Core tracing plugin implementation with channel event publishers and handler wrappers |
test/tracing.test.ts |
Comprehensive test suite covering tracing channels, event payloads, and middleware/route tracing |
test/_setup.ts |
Adds optional tracing flag to test configuration to conditionally enable tracing plugin |
package.json |
Adds ./tracing export entry for the tracing module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
test/tracing.test.ts (2)
123-123: Inconsistent test naming style.This test uses a colon-separated format inconsistent with other tests in the file that use plain English descriptions.
- it("tracing:h3.request.handler:asyncStart/asyncEnd fire for async handlers", async () => { + it("tracing channels fire asyncStart and asyncEnd events for async handlers", async () => {
157-157: Inconsistent test naming style.Same naming inconsistency as the previous test.
- it("tracing:h3.request.handler:error fires when handler throws", async () => { + it("tracing channels fire error events when handler throws", async () => {src/tracing.ts (6)
10-24: Consider adding JSDoc documentation for public interfaces.The exported types
HandlerType,H3THandlerTracePayload, andTracingPluginOptionsare part of the public API for tracing consumers. Adding JSDoc comments would help users understand the purpose and expected usage of each type.
46-53: Async wrapper forces all handlers to become async.The
async () => await middleware(...args)pattern creates unnecessary promise overhead for synchronous handlers. SincetracePromiseshould handle both sync and async functions, consider passing the handler result directly.- return requestHandlerChannel.tracePromise( - async () => await middleware(...args), - { + return requestHandlerChannel.tracePromise( + () => middleware(...args), + {Also applies to: 65-72
99-103: Direct mutation of route object.The code mutates
lastRoute.handlerandlastRoute.middlewaredirectly. Consider creating a new route object using the spread pattern (like lines 81-87) to avoid unexpected side effects if routes are cached elsewhere.- if (lastRoute) { - lastRoute.handler = wrapEventHandler(lastRoute.handler); - lastRoute.middleware = lastRoute.middleware?.map((m) => - wrapMiddleware(m), - ); - } + if (lastRoute) { + const lastRouteIndex = instance["~routes"].length - 1; + instance["~routes"][lastRouteIndex] = { + ...lastRoute, + handler: wrapEventHandler(lastRoute.handler), + middleware: lastRoute.middleware?.map((m) => wrapMiddleware(m)), + } satisfies H3Route; + }
49-52: Theroutefield is never populated in trace payloads.The
H3THandlerTracePayloadinterface includesroute?: string, but neitherwrapMiddlewarenorwrapEventHandlersets this field. For route handlers, the matched route path could be obtained from the route configuration and included in the payload to improve tracing usefulness.Also applies to: 68-71
90-107: No guard against double-wrapping of h3.on.If the tracing plugin is registered multiple times,
h3.ongets wrapped repeatedly, leading to duplicate trace events and performance degradation. Consider adding a__traced__check similar to handler wrapping.+ if ((h3.on as any).__traced__) { + return; + } + const originalOn = h3.on; h3.on = (...args) => { // ... existing implementation }; + + Object.defineProperty(h3.on, "__traced__", { + value: true, + configurable: true, + enumerable: false, + writable: false, + });
109-129: No guard against double-wrapping of h3.use.Same concern as
h3.on— if registered multiple times,h3.usewill be wrapped repeatedly.
🧹 Nitpick comments (5)
test/tracing.test.ts (3)
160-165: Mock implementation restoration may not properly reset the mock.Saving the mock reference and reassigning it later doesn't properly restore the mock's original implementation. If
onErroris a Vitest mock, consider usingmockRestore()ormockReset()instead.- const originalOnError = t.hooks.onError; - t.hooks.onError.mockImplementation(() => { - // Silence error - we're testing the tracing channel - }); + t.hooks.onError.mockImplementation(() => { + // Silence error - we're testing the tracing channel + });And in the
finallyblock:- t.hooks.onError = originalOnError; + t.hooks.onError.mockRestore();
216-238: Consider adding test for TracingPluginOptions.The test suite validates middleware and route tracing well, but lacks coverage for
TracingPluginOptions(traceMiddlewares: falseandtraceRoutes: false). These options allow selective disabling of tracing but have no tests verifying the behavior.Would you like me to help generate tests that verify:
traceMiddlewares: falseprevents middleware trace eventstraceRoutes: falseprevents route handler trace events
240-262: Consider verifying theroutefield in payload.The test checks for
event,url, andreq.methodbut doesn't verify theroutefield defined inH3THandlerTracePayload. Sincerouteis part of the public API, it would be valuable to test its presence/value.src/tracing.ts (2)
131-131: TODO: Trace mount remains unaddressed.The TODO comment indicates mount tracing is still pending. This aligns with the PR comments mentioning exploration of auto-tracing nested H3 instances.
Would you like me to help implement mount tracing, or should this be tracked as a separate issue?
133-135: Return value is correct but consider early return consistency.The function returns
h3at the end, but note that the early return on line 36 returnsundefined(implicitly). For consistency, consider returningh3in the early bail-out case as well, or document that plugins may returnundefinedwhen no-op.// If tracingChannel is not available, then we can't trace request handlers if (!tracingChannel) { - return; + return h3; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/tracing.ts(1 hunks)test/tracing.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/tracing.test.ts (2)
src/tracing.ts (1)
H3THandlerTracePayload(12-16)test/_setup.ts (2)
TestOptions(185-190)describeMatrix(25-52)
src/tracing.ts (1)
src/index.ts (7)
H3Event(24-24)Middleware(43-43)EventHandler(35-35)H3Plugin(6-6)definePlugin(17-17)H3Route(7-7)MiddlewareOptions(12-12)
🔇 Additional comments (4)
test/tracing.test.ts (2)
1-11: LGTM!Clean imports and well-typed
TracingEventinterface that correctly represents all possible tracing lifecycle phases.
13-56: Well-structured listener helper with proper cleanup.The
createTracingListenerfunction correctly manages the subscription lifecycle and provides acleanupmethod that unsubscribes all handlers. This ensures tests don't leak event listeners.src/tracing.ts (2)
1-8: LGTM!Clean imports of the necessary H3 types for the tracing plugin implementation.
31-37: Good defensive handling for environments without diagnostics_channel.The graceful no-op when
tracingChannelis unavailable ensures the plugin doesn't break in environments that lack this Node.js feature.
logaretm
force-pushed
the
feat/tracing-channels
branch
from
c7af908 to
3ef3a60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/tracing.ts (6)
18-19: Consider using Symbol instead of string property for__traced__.Using a string property
__traced__on function objects could potentially conflict with user-defined properties. A Symbol would provide better isolation and follow JavaScript best practices for internal metadata.+const TRACED = Symbol('traced'); + -type MaybeTracedMiddleware = Middleware & { __traced__?: boolean }; -type MaybeTracedEventHandler = EventHandler & { __traced__?: boolean }; +type MaybeTracedMiddleware = Middleware & { [TRACED]?: boolean }; +type MaybeTracedEventHandler = EventHandler & { [TRACED]?: boolean };Then update the checks on lines 51, 64, 70, and 83 to use
[TRACED]instead of__traced__.
24-33: Add JSDoc documentation for public interface.The
TracingPluginOptionsinterface is part of the public API but lacks documentation explaining its purpose and usage. Consider adding JSDoc comments to improve the developer experience./** * Options for the tracing plugin. + * + * @example + * ```typescript + * tracingPlugin({ + * traceMiddlewares: true, + * traceRoutes: true + * }) + * ``` */ export interface TracingPluginOptions { /** * Whether to trace middleware executions. + * @default true */ traceMiddlewares?: boolean; /** * Whether to trace route executions. + * @default true */ traceRoutes?: boolean; }
55-57: Remove redundant async wrapper for better performance.The
async () => middleware(...args)pattern forces all middleware to become async, creating unnecessary promise overhead for synchronous middleware. SincetracePromiselikely handles both sync and async functions, you can call the middleware directly.const wrappedMiddleware: MaybeTracedMiddleware = (...args) => { return requestHandlerChannel.tracePromise( - async () => middleware(...args), + () => middleware(...args), { event: args[0], type: "middleware", } satisfies H3THandlerTracePayload, ); };
74-76: Remove redundant async wrapper for better performance.Same issue as the middleware wrapper: the
async () => handler(...args)pattern creates unnecessary promise overhead for synchronous handlers.const wrappedHandler: MaybeTracedEventHandler = (...args) => { return requestHandlerChannel.tracePromise( - async () => handler(...args), + () => handler(...args), { event: args[0], type: "route", } satisfies H3THandlerTracePayload, ); };
107-112: Inconsistent mutation pattern compared to initial route wrapping.This code directly mutates
lastRoute.handlerandlastRoute.middleware, which is inconsistent with the immutable pattern used in lines 89-97 where new route objects are created with spread operators. While the__traced__flag prevents double-wrapping, maintaining consistency with the immutable approach would be cleaner and less error-prone.// eslint-disable-next-line unicorn/prefer-at - const lastRoute = instance["~routes"][instance["~routes"].length - 1]; - if (lastRoute) { - lastRoute.handler = wrapEventHandler(lastRoute.handler); - lastRoute.middleware = lastRoute.middleware?.map((m) => - wrapMiddleware(m), - ); + const lastRouteIndex = instance["~routes"].length - 1; + if (lastRouteIndex >= 0) { + const lastRoute = instance["~routes"][lastRouteIndex]; + instance["~routes"][lastRouteIndex] = { + ...lastRoute, + handler: wrapEventHandler(lastRoute.handler), + middleware: lastRoute.middleware?.map((m) => wrapMiddleware(m)), + } satisfies H3Route; }
140-140: TODO: Trace mount functionality.The mount tracing is not yet implemented. This would complete the tracing coverage for all H3 lifecycle hooks.
Do you want me to generate an implementation for mount tracing or open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/tracing.ts(1 hunks)test/tracing.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tracing.test.ts
🔇 Additional comments (3)
src/tracing.ts (3)
88-97: LGTM: Proper immutable wrapping of existing middleware and routes.The code correctly uses
.map()with spread operators to create new route objects rather than mutating existing ones. This approach is safer and follows immutability best practices.
119-138: LGTM: h3.use override correctly handles multiple signatures.The wrapper properly handles both the route-based and standalone middleware signatures, wrapping middleware at the argument level as intended. This approach ensures tracing works correctly with H3's internal middleware processing.
38-39: The definePlugin usage here is correct. The type annotation(traceOpts?: TracingPluginOptions) => H3Pluginaccurately reflects whatdefinePluginreturns. The implementation patterndefinePlugin((h3, traceOpts?: TracingPluginOptions) => {...})correctly passes the optionaltraceOptsparameter through to the plugin body via closure, where it is properly used on lines 51 and 70 via optional chaining (traceOpts?.traceMiddlewaresandtraceOpts?.traceRoutes).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/tracing.test.ts (2)
124-124: Test naming is inconsistent with other tests in the file.This test uses the colon-separated format
"tracing:h3.request.handler:asyncStart/asyncEnd"while other tests use plain English descriptions. Consider renaming to match the style of other tests, such as"tracing channels fire asyncStart and asyncEnd events for async handlers".
158-158: Test naming is inconsistent with other tests in the file.This test uses the colon-separated format
"tracing:h3.request.handler:error"while other tests use plain English descriptions. Consider renaming to match the style of other tests, such as"tracing channels fire error events when handler throws".
🧹 Nitpick comments (5)
src/tracing.ts (5)
55-57: Consider removing redundant async wrapper for better performance.The
async () => middleware(...args)pattern forces synchronous middleware to become async, adding promise overhead. SincetracePromisetypically handles both sync and async functions, you can likely pass the function directly.Apply this diff:
const wrappedMiddleware: MaybeTracedMiddleware = (...args) => { return requestHandlerChannel.tracePromise( - async () => middleware(...args), + () => middleware(...args), { event: args[0], type: "middleware", } satisfies H3THandlerTracePayload, ); };Apply the same pattern to
wrapEventHandlerat Line 76.
108-112: Consider creating new route object for consistency.While direct mutation works here, it's inconsistent with lines 89-96 which create new route objects using the spread operator. Creating a new object would improve consistency and avoid potential issues if routes are referenced elsewhere.
Apply this diff:
const lastRoute = instance["~routes"][instance["~routes"].length - 1]; if (lastRoute) { - lastRoute.handler = wrapEventHandler(lastRoute.handler); - lastRoute.middleware = lastRoute.middleware?.map((m) => - wrapMiddleware(m), - ); + instance["~routes"][instance["~routes"].length - 1] = { + ...lastRoute, + handler: wrapEventHandler(lastRoute.handler), + middleware: lastRoute.middleware?.map((m) => wrapMiddleware(m)), + } satisfies H3Route; }
140-140: Offer to implement nested app tracing.The PR objectives mention that registering the plugin on nested apps remains an open TODO. This would complete the tracing functionality for mounted H3 instances.
Would you like me to help implement tracing for
h3.mountto automatically trace nested H3 apps? This could follow a similar pattern to theh3.onoverride.
10-16: Consider adding JSDoc describing event lifecycle.The
H3THandlerTracePayloadinterface would benefit from documentation explaining when tracing events fire (start, asyncStart, asyncEnd, end, error) and their relationship to request processing.Example:
/** * Payload sent to the tracing channels. * * Tracing events fire throughout the request lifecycle: * - start: Handler invocation begins (sync) * - asyncStart: Async handler execution begins * - asyncEnd: Async handler execution completes (includes result/error) * - end: Handler invocation ends (sync) * - error: Handler throws an error */ export interface H3THandlerTracePayload { event: H3Event; type: "middleware" | "route"; }
18-19: Consider using Symbol for__traced__flag.Using a string property like
__traced__could potentially conflict with user-defined properties. A Symbol would avoid naming conflicts and make the implementation more robust.Example:
const TRACED = Symbol('traced'); type MaybeTracedMiddleware = Middleware & { [TRACED]?: boolean }; type MaybeTracedEventHandler = EventHandler & { [TRACED]?: boolean }; // Then use: middleware[TRACED] instead of middleware.__traced__
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/tracing.ts(1 hunks)test/_setup.ts(3 hunks)test/tracing.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/_setup.ts
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
test/tracing.test.ts (3)
src/tracing.ts (2)
H3THandlerTracePayload(13-16)tracingPlugin(38-143)test/_setup.ts (2)
TestOptions(185-190)describeMatrix(25-52)src/index.ts (1)
H3(19-19)
src/tracing.ts (2)
src/index.ts (6)
H3Event(24-24)Middleware(43-43)EventHandler(35-35)H3Plugin(6-6)H3Route(7-7)MiddlewareOptions(12-12)src/h3.ts (1)
handler(53-67)
🔇 Additional comments (2)
test/tracing.test.ts (1)
14-57: LGTM!The tracing listener helper is well-designed with proper subscription/cleanup lifecycle and provides good test isolation.
src/tracing.ts (1)
38-48: LGTM!The plugin correctly uses
definePluginwith options as the second parameter and gracefully handles environments wherediagnostics_channelis unavailable.
logaretm
force-pushed
the
feat/tracing-channels
branch
from
e9c489c to
a630b35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/tracing.test.ts (2)
126-126: Inconsistent test naming style.The test name uses a colon-separated format that is inconsistent with other test names in the file. Other tests use plain English descriptions like "tracing channels fire for handlers".
160-160: Inconsistent test naming style.The test name uses a colon-separated format that is inconsistent with other test names in the file. Consider using a plain English description like "tracing channels fire error events when handler throws".
🧹 Nitpick comments (1)
test/tracing.test.ts (1)
789-1209: Consider extracting H3Core test setup to reduce duplication.The H3Core tests follow a repetitive pattern: dynamic imports, manual route/middleware setup, ~findRoute mocking, and event creation. While the tests provide valuable coverage, extracting the common setup into a helper function could improve maintainability.
Example helper:
async function createH3CoreTestSetup(config?: { traceMiddlewares?: boolean; traceRoutes?: boolean }) { const { H3Core } = await import("../src/h3.ts"); const { tracingPlugin } = await import("../src/tracing.ts"); const { H3Event } = await import("../src/event.ts"); return { H3Core, tracingPlugin, H3Event }; }This would reduce the 40+ lines of duplicated dynamic imports across the test suite.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/tracing.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/tracing.test.ts (5)
src/tracing.ts (2)
H3THandlerTracePayload(13-16)tracingPlugin(38-159)test/_setup.ts (1)
TestOptions(185-190)src/index.ts (4)
HTTPError(75-75)H3(19-19)H3Core(19-19)H3Event(24-24)src/event.ts (2)
res(77-79)H3Event(33-144)src/h3.ts (4)
H3(114-233)H3(235-235)H3Core(36-112)request(69-94)
🔇 Additional comments (2)
test/tracing.test.ts (2)
1-124: LGTM! Well-structured test foundation.The imports, type definitions, and
createTracingListenerhelper provide a solid foundation for the test suite. The first two tests properly verify basic tracing functionality and event pairing.
188-787: Excellent test coverage.This section provides comprehensive coverage of middleware tracing, nested apps, configuration options, and fetch handler scenarios. The tests properly verify payload structure, event isolation, and configuration behavior.
pi0
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/tracing.ts (1)
99-118: Route mutation after registration works but differs from the immutable pattern used earlier.Lines 110-113 mutate the
lastRouteobject directly, while lines 88-97 use an immutable spread pattern. For consistency and to avoid potential issues with cached routes, consider using the same immutable approach here.This was mentioned in a previous review comment. If you've intentionally kept the mutation pattern here for a specific reason (e.g., the route object is freshly created and not shared), no action is needed.
🧹 Nitpick comments (1)
src/tracing.ts (1)
144-155: Duck-typing for H3 instance detection could match non-H3 objects.The check
"register" in inputwill match any object with aregistermethod, not just H3 instances. If another framework or object happens to have aregistermethod, this could cause unexpected behavior.Consider a more specific check:
- if ("register" in input) { + if ("register" in input && "~routes" in input) { input.register(tracingPlugin(traceOpts)); }This adds an additional H3-specific internal property check to reduce false positives.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.jsonsrc/tracing.tstest/tracing.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tracing.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/tracing.ts (3)
src/event.ts (1)
H3Event(33-144)src/index.ts (8)
H3Event(24-24)Middleware(43-43)EventHandler(35-35)H3Plugin(6-6)H3(19-19)H3Core(19-19)H3Route(7-7)MiddlewareOptions(12-12)src/h3.ts (4)
H3(114-233)H3(235-235)H3Core(36-112)handler(53-67)
🔇 Additional comments (5)
src/tracing.ts (4)
1-19: Well-structured type definitions and imports.The
TracingRequestEventinterface clearly defines the payload shape for trace events, and theMaybeTracedMiddleware/MaybeTracedEventHandlertypes provide a clean way to track wrapping state.
38-48: Graceful degradation when diagnostics_channel is unavailable.The early return when
tracingChannelis not available is a good pattern—this allows the plugin to be registered unconditionally without runtime errors in environments that don't supportdiagnostics_channel.
88-97: Clean immutable transformation of existing routes and middleware.Using spread operator and
satisfiesto create new route objects instead of mutating is a good pattern that avoids issues with shared references.
50-86: The async wrappers are necessary and correctly implemented. Node.js'sdiagnostics_channel.TracingChannel.tracePromiseAPI specifically requires a promise-returning function, soasync () => middleware(...args)andasync () => handler(...args)cannot be simplified without breaking the tracing contract. The current approach is correct.package.json (1)
25-26: The new tracing export is correctly configured for TypeScript type resolution.The build configuration automatically includes
src/tracing.tsvia theinferExports()function that discovers entries from package.json exports. Withdts: { resolve: true }enabled in the obuild configuration, the build process will generate the corresponding./dist/tracing.d.mtsfile, enabling TypeScript consumers to import types fromh3/tracing.
As per the discussion I created, this PR implements support for tracing channel events, allow subscribers to create instrumentations without monkey patching.
Implementation
h3.request.handlertracing events with full lifecycle hooks (start, end, asyncStart, asyncEnd, error)Example usage
Span Relationships
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.
This is fine at
h3level because 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:
app.mountFetch handlersSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.