diff --git a/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs index d081af0a4295..52989be7c7c9 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs @@ -18,6 +18,12 @@ const run = async () => { }, }); + server.route({ + method: ['GET', 'POST'], + path: '/multi', + handler: () => 'ok', + }); + server.route({ method: 'GET', path: '/error', diff --git a/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts b/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts index 24a9d1b2adba..376f5d974470 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts @@ -1,3 +1,5 @@ +import { HTTP_REQUEST_METHOD, HTTP_ROUTE, SENTRY_OP, SENTRY_ORIGIN } from '@sentry/conventions/attributes'; +import { ROUTER } from '@sentry/conventions/op'; import { afterAll, describe, expect } from 'vitest'; import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; @@ -47,6 +49,51 @@ describe('hapi auto-instrumentation', () => { await runner.completed(); }); + test('should instrument a route registered with an array of methods.', async () => { + const runner = createRunner() + .unordered() + .expect({ + span: container => { + expect(container.items.find(item => item.is_segment)?.name).toBe('POST /multi'); + expect(container.items).toContainEqual( + expect.objectContaining({ + name: '/multi', + status: 'ok', + attributes: expect.objectContaining({ + [HTTP_ROUTE]: { type: 'string', value: '/multi' }, + [HTTP_REQUEST_METHOD]: { type: 'string', value: 'POST' }, + 'hapi.type': { type: 'string', value: 'router' }, + [SENTRY_ORIGIN]: { type: 'string', value: origin }, + [SENTRY_OP]: { type: 'string', value: ROUTER }, + }), + }), + ); + }, + }) + .expect({ + span: container => { + expect(container.items.find(item => item.is_segment)?.name).toBe('GET /multi'); + expect(container.items).toContainEqual( + expect.objectContaining({ + name: '/multi', + status: 'ok', + attributes: expect.objectContaining({ + [HTTP_ROUTE]: { type: 'string', value: '/multi' }, + [HTTP_REQUEST_METHOD]: { type: 'string', value: 'GET' }, + 'hapi.type': { type: 'string', value: 'router' }, + [SENTRY_ORIGIN]: { type: 'string', value: origin }, + [SENTRY_OP]: { type: 'string', value: ROUTER }, + }), + }), + ); + }, + }) + .start(); + await runner.makeRequest('post', '/multi'); + await runner.makeRequest('get', '/multi'); + await runner.completed(); + }); + test('should instrument plugin routes and server extensions.', async () => { const runner = createRunner() .expect({ diff --git a/packages/server-utils/src/integrations/hapi/hapi-types.ts b/packages/server-utils/src/integrations/hapi/hapi-types.ts index 0afb23cc1448..7c3d8ff4231e 100644 --- a/packages/server-utils/src/integrations/hapi/hapi-types.ts +++ b/packages/server-utils/src/integrations/hapi/hapi-types.ts @@ -29,7 +29,8 @@ export interface ServerRouteOptions { export interface ServerRoute { path: string; - method: string; + /** A single method (including `'*'`), or several methods registered together. */ + method: string | string[]; handler?: LifecycleMethod | unknown; options?: ((server: unknown) => ServerRouteOptions) | ServerRouteOptions; [key: string]: unknown; diff --git a/packages/server-utils/src/integrations/hapi/hapi-utils.ts b/packages/server-utils/src/integrations/hapi/hapi-utils.ts index 5c7afa11a268..86d3f390ab10 100644 --- a/packages/server-utils/src/integrations/hapi/hapi-utils.ts +++ b/packages/server-utils/src/integrations/hapi/hapi-utils.ts @@ -107,13 +107,40 @@ const isPatchableExtMethod = ( return !Array.isArray(variableToCheck); }; +/** + * Method hapi has already resolved for this request. + * + * `server.route()` accepts `method` as a string or an array and expands an array + * into one route per method only after instrumentation wraps the handler, so the + * closed-over config can still hold the array. The request carries the one method + * that matched (lower case). + */ +function matchedRequestMethod(request: unknown): string | undefined { + if (isObjectLike(request) && typeof request.method === 'string' && request.method.length > 0) { + return request.method; + } + return undefined; +} + +/** A single method string safe to put on a span. Non-strings never reach `toUpperCase`. */ +function routeMethodForSpan(route: ServerRoute, requestMethod?: string): string | undefined { + if (typeof route.method === 'string') { + return route.method; + } + return requestMethod ? requestMethod.toUpperCase() : undefined; +} + /** Build the span name and attributes for a Hapi route. */ -export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanMetadata => { +export const getRouteMetadata = (route: ServerRoute, pluginName?: string, requestMethod?: string): SpanMetadata => { + const method = routeMethodForSpan(route, requestMethod); const attributes: SpanAttributes = { [HTTP_ROUTE]: route.path, - [HTTP_REQUEST_METHOD]: route.method, }; + if (method !== undefined) { + attributes[HTTP_REQUEST_METHOD] = method; + } + if (pluginName) { attributes[AttributeNames.HAPI_TYPE] = HapiLayerType.PLUGIN; attributes[AttributeNames.PLUGIN_NAME] = pluginName; @@ -129,7 +156,11 @@ export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanM return { attributes, - name: isStreamedSpan ? route.path || fallbackName : `${route.method.toUpperCase()} ${route.path}`, + name: isStreamedSpan + ? route.path || fallbackName + : method !== undefined + ? `${method.toUpperCase()} ${route.path}` + : route.path || fallbackName, }; }; @@ -199,7 +230,7 @@ function wrapRouteHandler(route: PatchableServerRoute, pluginName?: string): Pat return oldHandler.call(this, ...params); } setHttpServerSpanRouteAttribute(route.path); - const metadata = getRouteMetadata(route, pluginName); + const metadata = getRouteMetadata(route, pluginName, matchedRequestMethod(params[0])); return startMetadataSpan(metadata, () => oldHandler.call(this, ...params)); }; }; diff --git a/packages/server-utils/test/integrations/hapi-utils.test.ts b/packages/server-utils/test/integrations/hapi-utils.test.ts index a0a7a43971fa..fd85c9fd2e3e 100644 --- a/packages/server-utils/test/integrations/hapi-utils.test.ts +++ b/packages/server-utils/test/integrations/hapi-utils.test.ts @@ -1,6 +1,8 @@ -import { setCurrentClient } from '@sentry/core'; +import { HTTP_REQUEST_METHOD, HTTP_ROUTE } from '@sentry/conventions/attributes'; +import { setCurrentClient, spanToJSON, startSpan } from '@sentry/core'; +import type { Span } from '@sentry/core'; import { afterEach, describe, expect, it } from 'vitest'; -import { getExtMetadata, getRouteMetadata } from '../../src/integrations/hapi/hapi-utils'; +import { getExtMetadata, getRouteMetadata, wrapRouteArguments } from '../../src/integrations/hapi/hapi-utils'; import { getDefaultTestClientOptions, TestClient } from '../mocks/client'; describe('getRouteMetadata', () => { @@ -56,6 +58,205 @@ describe('getRouteMetadata', () => { expect(getRouteMetadata(pathlessRoute).name).toBe('Router'); expect(getRouteMetadata(pathlessRoute, 'my-plugin').name).toBe('Request handler'); }); + + it('uses one uppercase method when the route was registered with an array of methods', () => { + expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, undefined, 'post')).toEqual({ + name: 'POST /multi', + attributes: { + [HTTP_ROUTE]: '/multi', + [HTTP_REQUEST_METHOD]: 'POST', + 'hapi.type': 'router', + }, + }); + }); + + it('uses one uppercase method for a plugin route registered with an array of methods', () => { + expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, 'my-plugin', 'get')).toEqual({ + name: 'GET /multi', + attributes: { + [HTTP_ROUTE]: '/multi', + [HTTP_REQUEST_METHOD]: 'GET', + 'hapi.type': 'plugin', + 'hapi.plugin.name': 'my-plugin', + }, + }); + }); + + it('records one uppercase method and keeps the path span name when span streaming is enabled', () => { + const client = new TestClient(getDefaultTestClientOptions({ traceLifecycle: 'stream' })); + setCurrentClient(client); + + expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, undefined, 'post')).toEqual({ + name: '/multi', + attributes: { + [HTTP_ROUTE]: '/multi', + [HTTP_REQUEST_METHOD]: 'POST', + 'hapi.type': 'router', + }, + }); + }); + + it('does not throw when an array method has no resolved request method', () => { + expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any)).toEqual({ + name: '/multi', + attributes: { + [HTTP_ROUTE]: '/multi', + 'hapi.type': 'router', + }, + }); + }); + + it('keeps a string route method when a request method is also provided', () => { + expect(getRouteMetadata({ path: '/single', method: 'GET' } as any, undefined, 'post')).toEqual({ + name: 'GET /single', + attributes: { + [HTTP_ROUTE]: '/single', + [HTTP_REQUEST_METHOD]: 'GET', + 'hapi.type': 'router', + }, + }); + }); +}); + +describe('wrapRouteArguments', () => { + afterEach(() => { + setCurrentClient(undefined as unknown as TestClient); + }); + + function captureSpans(traceLifecycle?: 'stream'): Span[] { + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + tracesSampleRate: 1, + ...(traceLifecycle ? { traceLifecycle } : {}), + }), + ); + setCurrentClient(client); + client.init(); + + const endedSpans: Span[] = []; + client.on('spanEnd', span => { + endedSpans.push(span); + }); + return endedSpans; + } + + function invokeWrapped( + route: { method: string | string[]; path: string; handler: (request: unknown, h: unknown) => unknown }, + request: { method: string }, + ): unknown { + wrapRouteArguments([route]); + return startSpan({ name: 'incoming' }, () => route.handler(request, {})); + } + + function routerSpan(endedSpans: Span[]) { + return endedSpans.map(span => spanToJSON(span)).find(span => span.attributes['hapi.type'] === 'router'); + } + + it('reads the request method for a route registered with an array of methods', () => { + const endedSpans = captureSpans(); + + expect( + invokeWrapped( + { + method: ['GET', 'POST'], + path: '/multi', + handler: () => 'ok', + }, + { method: 'post' }, + ), + ).toBe('ok'); + + expect(routerSpan(endedSpans)).toMatchObject({ + name: 'POST /multi', + attributes: { + [HTTP_ROUTE]: '/multi', + [HTTP_REQUEST_METHOD]: 'POST', + 'hapi.type': 'router', + }, + }); + }); + + it('reads the request method and keeps the path span name when span streaming is enabled', () => { + const endedSpans = captureSpans('stream'); + + invokeWrapped( + { + method: ['GET', 'POST'], + path: '/multi', + handler: () => 'ok', + }, + { method: 'get' }, + ); + + expect(routerSpan(endedSpans)).toMatchObject({ + name: '/multi', + attributes: { + [HTTP_ROUTE]: '/multi', + [HTTP_REQUEST_METHOD]: 'GET', + }, + }); + }); + + it('keeps the registered method for a string route', () => { + const endedSpans = captureSpans(); + + invokeWrapped( + { + method: 'GET', + path: '/single', + handler: () => 'ok', + }, + { method: 'get' }, + ); + + expect(routerSpan(endedSpans)).toMatchObject({ + name: 'GET /single', + attributes: { + [HTTP_REQUEST_METHOD]: 'GET', + }, + }); + }); + + it('still runs the route handler when building span metadata cannot resolve a method', () => { + const endedSpans = captureSpans(); + + expect( + invokeWrapped( + { + method: ['GET', 'POST'], + path: '/multi', + handler: () => 'ok', + }, + { method: '' }, + ), + ).toBe('ok'); + + expect(routerSpan(endedSpans)).toMatchObject({ + name: '/multi', + attributes: { + [HTTP_ROUTE]: '/multi', + }, + }); + expect(routerSpan(endedSpans)?.attributes[HTTP_REQUEST_METHOD]).toBeUndefined(); + }); + + it('propagates errors thrown by the route handler', () => { + captureSpans(); + + expect(() => + invokeWrapped( + { + method: ['GET', 'POST'], + path: '/multi', + handler: () => { + throw new Error('user'); + }, + }, + { method: 'post' }, + ), + ).toThrow('user'); + }); }); describe('getExtMetadata', () => {