diff --git a/src/response.ts b/src/response.ts index 70fa144..9ccbc75 100644 --- a/src/response.ts +++ b/src/response.ts @@ -1214,7 +1214,11 @@ export class HttpResponse extends Macroable { * Handle text based response */ if (this.content) { - httpResponseSerializer.traceSync(this.writeBody, undefined, this, ...this.content) + if (httpResponseSerializer.hasSubscribers) { + httpResponseSerializer.traceSync(this.writeBody, undefined, this, ...this.content) + } else { + this.writeBody(...this.content) + } return } diff --git a/src/router/executor.ts b/src/router/executor.ts index 524d0fa..8a558b9 100644 --- a/src/router/executor.ts +++ b/src/router/executor.ts @@ -33,46 +33,49 @@ export function execute( .runner() .errorHandler((error) => errorResponder(error, ctx)) .finalHandler(() => { + /** + * Tracing channels are only used when someone is listening. Otherwise + * the handlers are called directly to skip the tracePromise overhead. + */ if (typeof route.handler === 'function') { - return ( - httpRouteHandler.tracePromise( - ($ctx: HttpContext) => Promise.resolve((route.handler as Function)($ctx)), - httpRouteHandler.hasSubscribers ? { route } : undefined, - undefined, - ctx - ) as unknown as Promise - ).then(useReturnValue(ctx)) + const handler = route.handler + const promise = httpRouteHandler.hasSubscribers + ? httpRouteHandler.tracePromise( + ($ctx: HttpContext) => Promise.resolve(handler($ctx)), + { route }, + undefined, + ctx + ) + : Promise.resolve(handler(ctx)) + + return promise.then(useReturnValue(ctx)) } - return ( - httpRouteHandler.tracePromise( - route.handler.handle, - httpRouteHandler.hasSubscribers ? { route } : undefined, - undefined, - resolver, - ctx - ) as unknown as Promise - ).then(useReturnValue(ctx)) + const promise = httpRouteHandler.hasSubscribers + ? httpRouteHandler.tracePromise(route.handler.handle, { route }, undefined, resolver, ctx) + : route.handler.handle(resolver, ctx) + + return promise.then(useReturnValue(ctx)) }) .run(async (middleware, next) => { if (typeof middleware === 'function') { - return httpMiddleware.tracePromise( - middleware, - httpMiddleware.hasSubscribers ? { middleware } : undefined, - undefined, - ctx, - next - ) as unknown as Promise + if (!httpMiddleware.hasSubscribers) { + return middleware(ctx, next) + } + return httpMiddleware.tracePromise(middleware, { middleware }, undefined, ctx, next) } + if (!httpMiddleware.hasSubscribers) { + return middleware.handle(resolver, ctx, next, middleware.args) + } return httpMiddleware.tracePromise( middleware.handle, - httpMiddleware.hasSubscribers ? { middleware } : undefined, + { middleware }, undefined, resolver, ctx, next, middleware.args - ) as unknown as Promise + ) }) } diff --git a/src/server/factories/middleware_handler.ts b/src/server/factories/middleware_handler.ts index 5e38d6b..0831757 100644 --- a/src/server/factories/middleware_handler.ts +++ b/src/server/factories/middleware_handler.ts @@ -37,9 +37,18 @@ export function middlewareHandler(resolver: ContainerResolver, ctx: HttpCon */ return function (fn: ParsedGlobalMiddleware, next: NextFn) { debug('executing middleware %s', fn.name) + + /** + * Calling the handler directly avoids the variadic tracePromise call + * (rest args array + Reflect.apply) when nobody is listening. + */ + if (!httpMiddleware.hasSubscribers) { + return fn.handle(resolver, ctx, next) + } + return httpMiddleware.tracePromise( fn.handle, - httpMiddleware.hasSubscribers ? { middleware: fn } : undefined, + { middleware: fn }, undefined, resolver, ctx, diff --git a/src/server/main.ts b/src/server/main.ts index db096d9..d30db0b 100644 --- a/src/server/main.ts +++ b/src/server/main.ts @@ -148,6 +148,18 @@ export class Server { ) } + /** + * Handles the request inside the "adonisjs.http.request" tracing channel. + * The channel is skipped when nobody is listening to avoid the + * tracePromise overhead on every request. + */ + #traceRequest(ctx: HttpContext, resolver: ContainerResolver) { + if (!httpRequest.hasSubscribers) { + return this.#handleRequest(ctx, resolver) + } + return httpRequest.tracePromise(this.#handleRequest, { ctx }, this, ctx, resolver) + } + /** * Indicates whether the server has completed its boot process */ @@ -471,23 +483,9 @@ export class Server { * Handle request */ if (this.usingAsyncLocalStorage) { - return asyncLocalStorage.storage!.run(ctx, () => - httpRequest.tracePromise( - this.#handleRequest, - httpRequest.hasSubscribers ? { ctx } : undefined, - this, - ctx, - resolver - ) - ) + return asyncLocalStorage.storage!.run(ctx, () => this.#traceRequest(ctx, resolver)) } - return httpRequest.tracePromise( - this.#handleRequest, - httpRequest.hasSubscribers ? { ctx } : undefined, - this, - ctx, - resolver - ) + return this.#traceRequest(ctx, resolver) } } diff --git a/tests/server.spec.ts b/tests/server.spec.ts index b5d521b..492a65e 100644 --- a/tests/server.spec.ts +++ b/tests/server.spec.ts @@ -15,6 +15,12 @@ import { Emitter } from '@adonisjs/events' import type { NextFn } from '@poppinss/middleware/types' import { AppFactory } from '@adonisjs/application/factories' import { createServer, IncomingMessage, ServerResponse } from 'node:http' +import { + httpRequest, + httpMiddleware, + httpRouteHandler, + httpResponseSerializer, +} from '../src/tracing_channels.ts' import { Router } from '../src/router/main.ts' import { HttpContext } from '../src/http_context/main.ts' @@ -85,6 +91,77 @@ test.group('Server', () => { }) }) +test.group('Server | tracing', () => { + test('emit tracing events when subscribing after the server has booted', async ({ + assert, + cleanup, + }) => { + const app = new AppFactory().create(BASE_URL, () => {}) + const server = new ServerFactory().merge({ app }).create() + const httpServer = createServer(server.handle.bind(server)) + + await app.init() + + server.use([ + async () => { + return { + default: class GlobalMiddleware { + async handle(_: HttpContext, next: NextFn) { + await next() + } + }, + } + }, + ]) + server + .getRouter() + .get('/', async () => 'handled') + .middleware(async (_, next) => { + await next() + }) + + await server.boot() + + /** + * Requests made before subscribing must not prevent events + * from being emitted later on + */ + await supertest(httpServer).get('/').expect(200) + + const events: string[] = [] + const subscribe = (name: string, channel: { subscribe: Function; unsubscribe: Function }) => { + const handlers = { + start: () => events.push(`${name}:start`), + end: () => {}, + asyncStart: () => {}, + asyncEnd: () => events.push(`${name}:asyncEnd`), + error: () => {}, + } + channel.subscribe(handlers) + cleanup(() => channel.unsubscribe(handlers)) + } + + subscribe('request', httpRequest) + subscribe('middleware', httpMiddleware) + subscribe('route', httpRouteHandler) + subscribe('serializer', httpResponseSerializer) + + const { text } = await supertest(httpServer).get('/').expect(200) + assert.equal(text, 'handled') + assert.deepEqual(events, [ + 'request:start', + 'middleware:start', + 'middleware:start', + 'route:start', + 'route:asyncEnd', + 'middleware:asyncEnd', + 'middleware:asyncEnd', + 'serializer:start', + 'request:asyncEnd', + ]) + }) +}) + test.group('Server | Response handling', () => { test('invoke router handler', async ({ assert }) => { const app = new AppFactory().create(BASE_URL, () => {})