From 14466058c2f2f56662a4a1b1fa2f68e9299c3c30 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Wed, 17 Jun 2026 13:16:24 +0200 Subject: [PATCH] ref(node): Streamline hapi instrumentation Move vendored hapi span creation onto Sentry's span API (startSpan), fold op/origin into creation, drop the spanStart hook + semconv-stability branching, and add unit tests plus plugin-route/server.ext integration coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../suites/tracing/hapi/scenario.mjs | 16 +++ .../suites/tracing/hapi/test.ts | 37 ++++++ .../src/integrations/tracing/hapi/index.ts | 34 +----- .../tracing/hapi/vendored/instrumentation.ts | 111 +++++++----------- .../tracing/hapi/vendored/utils.ts | 25 +--- .../test/integrations/tracing/hapi.test.ts | 90 ++++++++++++++ 6 files changed, 194 insertions(+), 119 deletions(-) create mode 100644 packages/node/test/integrations/tracing/hapi.test.ts 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 c1497d2c5e2f..1aa7e8846b9f 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs @@ -51,6 +51,22 @@ const run = async () => { }, }); + // Route registered via a plugin produces a `plugin.hapi` span. + await server.register({ + name: 'testPlugin', + version: '1.0.0', + register: async function (pluginServer) { + pluginServer.route({ + method: 'GET', + path: '/plugin-route', + handler: () => 'Hello from plugin!', + }); + }, + }); + + // Server extension produces a `server.ext.hapi` span. + server.ext('onPreResponse', (request, h) => h.continue); + await Sentry.setupHapiErrorHandler(server); await server.start(); 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 5693f55a6a8c..2510ecece88b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts @@ -43,6 +43,43 @@ describe('hapi auto-instrumentation', () => { await runner.completed(); }); + test('should instrument plugin routes and server extensions.', async () => { + const runner = createRunner() + .expect({ + transaction: { + transaction: 'GET /plugin-route', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'GET /plugin-route', + op: 'plugin.hapi', + origin: 'auto.http.otel.hapi', + data: expect.objectContaining({ + 'http.route': '/plugin-route', + 'hapi.type': 'plugin', + 'hapi.plugin.name': 'testPlugin', + 'sentry.op': 'plugin.hapi', + 'sentry.origin': 'auto.http.otel.hapi', + }), + }), + expect.objectContaining({ + description: 'ext - onPreResponse', + op: 'server.ext.hapi', + origin: 'auto.http.otel.hapi', + data: expect.objectContaining({ + 'hapi.type': 'server.ext', + 'server.ext.type': 'onPreResponse', + 'sentry.op': 'server.ext.hapi', + 'sentry.origin': 'auto.http.otel.hapi', + }), + }), + ]), + }, + }) + .start(); + runner.makeRequest('get', '/plugin-route'); + await runner.completed(); + }); + test('should handle returned plain errors in routes.', async () => { const runner = createRunner() .expect({ diff --git a/packages/node/src/integrations/tracing/hapi/index.ts b/packages/node/src/integrations/tracing/hapi/index.ts index ae3b870d5128..5b84fc4f94c5 100644 --- a/packages/node/src/integrations/tracing/hapi/index.ts +++ b/packages/node/src/integrations/tracing/hapi/index.ts @@ -1,16 +1,12 @@ import { HapiInstrumentation } from './vendored/instrumentation'; -import type { IntegrationFn, Span } from '@sentry/core'; +import type { IntegrationFn } from '@sentry/core'; import { captureException, debug, defineIntegration, - getClient, getDefaultIsolationScope, getIsolationScope, SDK_VERSION, - SEMANTIC_ATTRIBUTE_SENTRY_OP, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - spanToJSON, } from '@sentry/core'; import { ensureIsWrapped, generateInstrumentOnce } from '@sentry/node-core'; import { DEBUG_BUILD } from '../../../debug-build'; @@ -108,33 +104,5 @@ export const hapiErrorPlugin = { */ export async function setupHapiErrorHandler(server: Server): Promise { await server.register(hapiErrorPlugin); - - // Sadly, middleware spans do not go through `requestHook`, so we handle those here - // We register this hook in this method, because if we register it in the integration `setup`, - // it would always run even for users that are not even using hapi - const client = getClient(); - if (client) { - client.on('spanStart', span => { - addHapiSpanAttributes(span); - }); - } - ensureIsWrapped(server.register, 'hapi'); } - -function addHapiSpanAttributes(span: Span): void { - const attributes = spanToJSON(span).data; - - // this is one of: router, plugin, server.ext - const type = attributes['hapi.type']; - - // If this is already set, or we have no Hapi span, no need to process again... - if (attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !type) { - return; - } - - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.hapi', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.hapi`, - }); -} diff --git a/packages/node/src/integrations/tracing/hapi/vendored/instrumentation.ts b/packages/node/src/integrations/tracing/hapi/vendored/instrumentation.ts index 51f8475bc066..39fb11e516db 100644 --- a/packages/node/src/integrations/tracing/hapi/vendored/instrumentation.ts +++ b/packages/node/src/integrations/tracing/hapi/vendored/instrumentation.ts @@ -18,32 +18,27 @@ * - Upstream version: @opentelemetry/instrumentation-hapi@0.64.0 * - Types vendored from @hapi/hapi as simplified interfaces * - Minor TypeScript strictness adjustments for this repository's compiler settings + * - Span creation migrated to the @sentry/core API; op/origin folded into span creation */ -/* eslint-disable */ import * as api from '@opentelemetry/api'; import { setHttpServerSpanRouteAttribute } from '../../../../utils/setHttpServerSpanRouteAttribute'; -import { - InstrumentationBase, - InstrumentationConfig, - InstrumentationNodeModuleDefinition, - isWrapped, - SemconvStability, - semconvStabilityFromStr, -} from '@opentelemetry/instrumentation'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; import type * as Hapi from './hapi-types'; -import { SDK_VERSION } from '@sentry/core'; +import { SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan } from '@sentry/core'; +import { AttributeNames } from './enums/AttributeNames'; import { HapiComponentName, - HapiServerRouteInput, handlerPatched, - PatchableServerRoute, - HapiServerRouteInputMethod, - HapiPluginInput, - RegisterFunction, - PatchableExtMethod, - ServerExtDirectInput, + type HapiServerRouteInput, + type PatchableServerRoute, + type HapiServerRouteInputMethod, + type HapiPluginInput, + type RegisterFunction, + type PatchableExtMethod, + type ServerExtDirectInput, } from './internal-types'; import { getRouteMetadata, @@ -60,11 +55,8 @@ const PACKAGE_NAME = '@sentry/instrumentation-hapi'; /** Hapi instrumentation for OpenTelemetry */ export class HapiInstrumentation extends InstrumentationBase { - private _semconvStability: SemconvStability; - constructor(config: InstrumentationConfig = {}) { super(PACKAGE_NAME, SDK_VERSION, config); - this._semconvStability = semconvStabilityFromStr('http', process.env.OTEL_SEMCONV_STABILITY_OPT_IN); } protected init() { @@ -74,11 +66,11 @@ export class HapiInstrumentation extends InstrumentationBase { (module: any) => { const moduleExports: typeof Hapi = module[Symbol.toStringTag] === 'Module' ? module.default : module; if (!isWrapped(moduleExports.server)) { - this._wrap(moduleExports, 'server', this._getServerPatch.bind(this) as any); + this._wrap(moduleExports, 'server', this._getServerPatch.bind(this)); } if (!isWrapped(moduleExports.Server)) { - this._wrap(moduleExports, 'Server', this._getServerPatch.bind(this) as any); + this._wrap(moduleExports, 'Server', this._getServerPatch.bind(this)); } return moduleExports; }, @@ -115,7 +107,7 @@ export class HapiInstrumentation extends InstrumentationBase { // Casting as any is necessary here due to multiple overloads on the Hapi.Server.register // function, which requires supporting a variety of different types of Plugin inputs - self._wrap(newServer, 'register', instrumentation._getServerRegisterPatch.bind(instrumentation) as any); + self._wrap(newServer, 'register', instrumentation._getServerRegisterPatch.bind(instrumentation)); return newServer; }; } @@ -202,6 +194,7 @@ export class HapiInstrumentation extends InstrumentationBase { route[i] = newRoute; } } else { + // oxlint-disable-next-line no-param-reassign route = instrumentation._wrapRouteHandler.call(instrumentation, route, pluginName); } return original.apply(this, [route]); @@ -254,38 +247,29 @@ export class HapiInstrumentation extends InstrumentationBase { const instrumentation: HapiInstrumentation = this; if (method instanceof Array) { for (let i = 0; i < method.length; i++) { - method[i] = instrumentation._wrapExtMethods(method[i]!, extPoint) as PatchableExtMethod; + method[i] = instrumentation._wrapExtMethods(method[i]!, extPoint); } return method; } else if (isPatchableExtMethod(method)) { if (method[handlerPatched] === true) return method; method[handlerPatched] = true; - const newHandler: PatchableExtMethod = async function (this: any, ...params: Parameters) { + const newHandler: PatchableExtMethod = function (this: any, ...params: Parameters) { if (api.trace.getSpan(api.context.active()) === undefined) { - return await method.apply(this, params); + return method.apply(this, params); } const metadata = getExtMetadata(extPoint, pluginName, method.name); - const span = instrumentation.tracer.startSpan(metadata.name, { - attributes: metadata.attributes, - }); - try { - return await api.context.with, Hapi.Lifecycle.Method>( - api.trace.setSpan(api.context.active(), span), - method, - undefined, - ...params, - ); - } catch (err: any) { - span.recordException(err); - span.setStatus({ - code: api.SpanStatusCode.ERROR, - message: err.message, - }); - throw err; - } finally { - span.end(); - } + return startSpan( + { + name: metadata.name, + op: `${metadata.attributes[AttributeNames.HAPI_TYPE]}.hapi`, + attributes: { + ...metadata.attributes, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.hapi', + }, + }, + () => method.apply(undefined, params), + ); }; return newHandler as T; } @@ -300,34 +284,27 @@ export class HapiInstrumentation extends InstrumentationBase { * for adding this server route. Else, signifies that the route was added directly */ private _wrapRouteHandler(route: PatchableServerRoute, pluginName?: string): PatchableServerRoute { - const instrumentation: HapiInstrumentation = this; if (route[handlerPatched] === true) return route; route[handlerPatched] = true; const wrapHandler: (oldHandler: Hapi.Lifecycle.Method) => Hapi.Lifecycle.Method = oldHandler => { - return async function (this: any, ...params: Parameters) { + return function (this: any, ...params: Parameters) { if (api.trace.getSpan(api.context.active()) === undefined) { - return await oldHandler.call(this, ...params); + return oldHandler.call(this, ...params); } setHttpServerSpanRouteAttribute(route.path); - const metadata = getRouteMetadata(route, instrumentation._semconvStability, pluginName); - const span = instrumentation.tracer.startSpan(metadata.name, { - attributes: metadata.attributes, - }); - try { - return await api.context.with(api.trace.setSpan(api.context.active(), span), () => - oldHandler.call(this, ...params), - ); - } catch (err: any) { - span.recordException(err); - span.setStatus({ - code: api.SpanStatusCode.ERROR, - message: err.message, - }); - throw err; - } finally { - span.end(); - } + const metadata = getRouteMetadata(route, pluginName); + return startSpan( + { + name: metadata.name, + op: `${metadata.attributes[AttributeNames.HAPI_TYPE]}.hapi`, + attributes: { + ...metadata.attributes, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.hapi', + }, + }, + () => oldHandler.call(this, ...params), + ); }; }; diff --git a/packages/node/src/integrations/tracing/hapi/vendored/utils.ts b/packages/node/src/integrations/tracing/hapi/vendored/utils.ts index 94d7ed613cc9..f96a63b6ec1b 100644 --- a/packages/node/src/integrations/tracing/hapi/vendored/utils.ts +++ b/packages/node/src/integrations/tracing/hapi/vendored/utils.ts @@ -18,21 +18,19 @@ * - Upstream version: @opentelemetry/instrumentation-hapi@0.64.0 * - Types vendored from @hapi/hapi as simplified interfaces */ -/* eslint-disable */ -import { Attributes } from '@opentelemetry/api'; -import { ATTR_HTTP_ROUTE, ATTR_HTTP_REQUEST_METHOD } from '@opentelemetry/semantic-conventions'; +import type { Attributes } from '@opentelemetry/api'; +import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; import { ATTR_HTTP_METHOD } from './semconv'; import type * as Hapi from './hapi-types'; import { HapiLayerType, HapiLifecycleMethodNames, - HapiPluginObject, - PatchableExtMethod, - ServerExtDirectInput, + type HapiPluginObject, + type PatchableExtMethod, + type ServerExtDirectInput, } from './internal-types'; import { AttributeNames } from './enums/AttributeNames'; -import { SemconvStability } from '@opentelemetry/instrumentation'; export function getPluginName(plugin: Hapi.Plugin): string { if ((plugin as Hapi.PluginNameVersion).name) { @@ -70,7 +68,6 @@ export const isPatchableExtMethod = ( export const getRouteMetadata = ( route: Hapi.ServerRoute, - semconvStability: SemconvStability, pluginName?: string, ): { attributes: Attributes; @@ -78,18 +75,8 @@ export const getRouteMetadata = ( } => { const attributes: Attributes = { [ATTR_HTTP_ROUTE]: route.path, + [ATTR_HTTP_METHOD]: route.method, }; - if (semconvStability & SemconvStability.OLD) { - attributes[ATTR_HTTP_METHOD] = route.method; - } - if (semconvStability & SemconvStability.STABLE) { - // Note: This currently does *not* normalize the method name to uppercase - // and conditionally include `http.request.method.original` as described - // at https://opentelemetry.io/docs/specs/semconv/http/http-spans/ - // These attributes are for a *hapi* span, and not the parent HTTP span, - // so the HTTP span guidance doesn't strictly apply. - attributes[ATTR_HTTP_REQUEST_METHOD] = route.method; - } let name; if (pluginName) { diff --git a/packages/node/test/integrations/tracing/hapi.test.ts b/packages/node/test/integrations/tracing/hapi.test.ts new file mode 100644 index 000000000000..a11d93099d62 --- /dev/null +++ b/packages/node/test/integrations/tracing/hapi.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest'; +import { + getExtMetadata, + getPluginName, + getRouteMetadata, + isDirectExtInput, + isLifecycleExtType, + isPatchableExtMethod, +} from '../../../src/integrations/tracing/hapi/vendored/utils'; + +describe('getRouteMetadata', () => { + const route = { path: '/users/{id}', method: 'get' } as any; + + it('describes a directly-registered route as a router layer', () => { + expect(getRouteMetadata(route)).toEqual({ + name: 'route - /users/{id}', + attributes: { + 'http.route': '/users/{id}', + 'http.method': 'get', + 'hapi.type': 'router', + }, + }); + }); + + it('describes a plugin-registered route as a plugin layer', () => { + expect(getRouteMetadata(route, 'my-plugin')).toEqual({ + name: 'my-plugin: route - /users/{id}', + attributes: { + 'http.route': '/users/{id}', + 'http.method': 'get', + 'hapi.type': 'plugin', + 'hapi.plugin.name': 'my-plugin', + }, + }); + }); +}); + +describe('getExtMetadata', () => { + it('names an extension by its point', () => { + expect(getExtMetadata('onPreHandler')).toEqual({ + name: 'ext - onPreHandler', + attributes: { 'server.ext.type': 'onPreHandler', 'hapi.type': 'server.ext' }, + }); + }); + + it('includes the method name when it is not the default `method`', () => { + expect(getExtMetadata('onPreHandler', undefined, 'myHandler').name).toBe('ext - onPreHandler - myHandler'); + expect(getExtMetadata('onPreHandler', undefined, 'method').name).toBe('ext - onPreHandler'); + }); + + it('includes the plugin name and prefixes the span name', () => { + expect(getExtMetadata('onPreHandler', 'my-plugin')).toEqual({ + name: 'my-plugin: ext - onPreHandler', + attributes: { + 'server.ext.type': 'onPreHandler', + 'hapi.type': 'server.ext', + 'hapi.plugin.name': 'my-plugin', + }, + }); + }); +}); + +describe('getPluginName', () => { + it('reads the name property when present', () => { + expect(getPluginName({ name: 'direct-name' } as any)).toBe('direct-name'); + }); + + it('falls back to the package name', () => { + expect(getPluginName({ pkg: { name: 'pkg-name' } } as any)).toBe('pkg-name'); + }); +}); + +describe('ext type guards', () => { + it('isLifecycleExtType recognizes lifecycle points', () => { + expect(isLifecycleExtType('onPreHandler')).toBe(true); + expect(isLifecycleExtType('onPreStart')).toBe(false); + expect(isLifecycleExtType(undefined)).toBe(false); + }); + + it('isDirectExtInput recognizes the [type, method] tuple form', () => { + expect(isDirectExtInput(['onPreHandler', () => {}])).toBe(true); + expect(isDirectExtInput(['onPreHandler', 'not-a-fn'])).toBe(false); + expect(isDirectExtInput({ type: 'onPreHandler' })).toBe(false); + }); + + it('isPatchableExtMethod is false for arrays', () => { + expect(isPatchableExtMethod((() => {}) as any)).toBe(true); + expect(isPatchableExtMethod([() => {}] as any)).toBe(false); + }); +});