From 2549d4c146cd14657e42dbae2dacf7d7efe7782e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luismi=20Ram=C3=ADrez?= Date: Wed, 26 Oct 2022 14:49:27 +0200 Subject: [PATCH 1/3] feat(fastify): add requestHook support The `requestHook` config option allows custom span handling per request layer. --- .../README.md | 22 +++++++++++++ .../package.json | 4 ++- .../src/instrumentation.ts | 31 ++++++++++++++++-- .../src/types.ts | 19 +++++++++++ .../test/instrumentation.test.ts | 32 +++++++++++++++++++ 5 files changed, 104 insertions(+), 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-fastify/README.md b/plugins/node/opentelemetry-instrumentation-fastify/README.md index 89c4a6e1d0..85868b7712 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/README.md +++ b/plugins/node/opentelemetry-instrumentation-fastify/README.md @@ -45,6 +45,28 @@ registerInstrumentations({ See [examples/fastify](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/fastify) for a short example. +## Fastify Instrumentation Options + +| Options | Type | Example | Description | +| `requestHook` | `FastifyCustomAttributeFunction` | `(span, request) => {}` | Function for adding custom attributes to Fastify layers. Receives params: `Span, FastifyRequest`. | + +### Using `requestHook` + +Instrumentation configuration accepts a custom "hook" function which will be called for every instrumented Fastify layer involved in a request. Custom attributes can be set on the span or run any custom logic per layer. + +```javascript +import { FastifyInstrumentation } from "@opentelemetry/instrumentation-fastify" + +const fastifyInstrumentation = new FastifyInstrumentation({ + requestHook: function (span: Span, request: FastifyRequest) { + span.setAttribute( + 'http.method', + request.method, + ) + } +}); +``` + ## Useful links - For more information on OpenTelemetry, visit: diff --git a/plugins/node/opentelemetry-instrumentation-fastify/package.json b/plugins/node/opentelemetry-instrumentation-fastify/package.json index 5f73d37d80..567db08435 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/package.json +++ b/plugins/node/opentelemetry-instrumentation-fastify/package.json @@ -54,11 +54,13 @@ "@types/express": "4.17.13", "@types/mocha": "7.0.2", "@types/node": "16.11.21", - "gts": "3.1.0", + "@types/sinon": "10.0.13", "fastify": "^4.5.3", + "gts": "3.1.0", "mocha": "7.2.0", "nyc": "15.1.0", "rimraf": "3.0.2", + "sinon": "14.0.1", "ts-mocha": "10.0.0", "typescript": "4.3.5" }, diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts index b267f03336..44597b243a 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts @@ -16,6 +16,7 @@ import { context, + diag, SpanAttributes, SpanStatusCode, trace, @@ -23,7 +24,6 @@ import { import { getRPCMetadata, RPCType } from '@opentelemetry/core'; import { InstrumentationBase, - InstrumentationConfig, InstrumentationNodeModuleDefinition, safeExecuteInTheMiddle, } from '@opentelemetry/instrumentation'; @@ -38,7 +38,11 @@ import { FastifyNames, FastifyTypes, } from './enums/AttributeNames'; -import type { HandlerOriginal, PluginFastifyReply } from './types'; +import type { + HandlerOriginal, + PluginFastifyReply, + FastifyInstrumentationConfig, +} from './types'; import { endSpan, safeExecuteInTheMiddleMaybePromise, @@ -50,7 +54,7 @@ export const ANONYMOUS_NAME = 'anonymous'; /** Fastify instrumentation for OpenTelemetry */ export class FastifyInstrumentation extends InstrumentationBase { - constructor(config: InstrumentationConfig = {}) { + constructor(config: FastifyInstrumentationConfig = {}) { super( '@opentelemetry/instrumentation-fastify', VERSION, @@ -58,6 +62,14 @@ export class FastifyInstrumentation extends InstrumentationBase { ); } + override setConfig(config: FastifyInstrumentationConfig = {}) { + this._config = Object.assign({}, config); + } + + override getConfig(): FastifyInstrumentationConfig { + return this._config as FastifyInstrumentationConfig; + } + init() { return [ new InstrumentationNodeModuleDefinition( @@ -271,6 +283,19 @@ export class FastifyInstrumentation extends InstrumentationBase { spanName, spanAttributes ); + + if (instrumentation.getConfig().requestHook) { + safeExecuteInTheMiddle( + () => instrumentation.getConfig().requestHook!(span, request), + e => { + if (e) { + diag.error('fastify instrumentation: request hook failed', e); + } + }, + true + ); + } + return context.with(trace.setSpan(context.active(), span), () => { done(); }); diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts index 95521eed2f..055565bd72 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts @@ -17,9 +17,28 @@ import { Span } from '@opentelemetry/api'; import type { FastifyReply } from 'fastify'; import { spanRequestSymbol } from './constants'; +import { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import type { FastifyRequest } from 'fastify/types/request'; export type HandlerOriginal = (() => Promise) & (() => void); export type PluginFastifyReply = FastifyReply & { [spanRequestSymbol]?: Span[]; }; + +/** + * Function that can be used to add custom attributes to the current span + * @param span - The Fastify layer span. + * @param request - The Fastify request object. + */ +export interface FastifyCustomAttributeFunction { + (span: Span, request: FastifyRequest): void; +} + +/** + * Options available for the Fastify Instrumentation + */ +export interface FastifyInstrumentationConfig extends InstrumentationConfig { + /** Function for adding custom attributes to each layer span */ + requestHook?: FastifyCustomAttributeFunction; +} diff --git a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts index aa17f3b246..2e0c608c79 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts @@ -24,6 +24,7 @@ import { ReadableSpan, SimpleSpanProcessor, } from '@opentelemetry/sdk-trace-base'; +import { Span } from '@opentelemetry/api'; import { HookHandlerDoneFunction } from 'fastify/types/hooks'; import { FastifyReply } from 'fastify/types/reply'; import { FastifyRequest } from 'fastify/types/request'; @@ -31,6 +32,7 @@ import * as http from 'http'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; import { ANONYMOUS_NAME } from '../src/instrumentation'; import { AttributeNames, FastifyInstrumentation } from '../src'; +import * as sinon from 'sinon'; const URL = require('url').URL; @@ -426,5 +428,35 @@ describe('fastify', () => { await startServer(); }); }); + + describe('using requestHook in config', () => { + it('calls requestHook provided function when set in config', async () => { + const requestHook = sinon.spy((span: Span, request: FastifyRequest) => { + span.setAttribute('http.method', request.method); + }); + + instrumentation.setConfig({ + ...instrumentation.getConfig(), + requestHook, + }); + + app.get('/test', (req, res) => { + res.send('OK'); + }); + + await startServer(); + await httpRequest.get(`http://localhost:${PORT}/test`); + + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 5); + const span = spans[3]; + assert.deepStrictEqual(span.attributes, { + 'fastify.type': 'request_handler', + 'plugin.name': 'fastify -> @fastify/express', + [SemanticAttributes.HTTP_ROUTE]: '/test', + 'http.method': 'GET', + }); + }); + }); }); }); From 64f3ac66b2b53a58716ba59fbd60906e91d57d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luismi=20Ram=C3=ADrez?= Date: Thu, 3 Nov 2022 10:05:37 +0100 Subject: [PATCH 2/3] Address first requestHook proposal issues - Add a test for when an exception is thrown from inside the hook - Export the types - Don't use Fastify core types so the package doesn't need to be a dependency - Rephrase docs to be clearer about when the request hook is executed - Remove sinon - Use proper component to log hook errors --- .../README.md | 8 ++--- .../package.json | 2 -- .../src/index.ts | 1 + .../src/instrumentation.ts | 5 +-- .../src/types.ts | 13 ++++--- .../test/instrumentation.test.ts | 36 +++++++++++++++++-- 6 files changed, 49 insertions(+), 16 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-fastify/README.md b/plugins/node/opentelemetry-instrumentation-fastify/README.md index 85868b7712..2d50664008 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/README.md +++ b/plugins/node/opentelemetry-instrumentation-fastify/README.md @@ -48,20 +48,20 @@ See [examples/fastify](https://github.com/open-telemetry/opentelemetry-js-contri ## Fastify Instrumentation Options | Options | Type | Example | Description | -| `requestHook` | `FastifyCustomAttributeFunction` | `(span, request) => {}` | Function for adding custom attributes to Fastify layers. Receives params: `Span, FastifyRequest`. | +| `requestHook` | `FastifyCustomAttributeFunction` | `(span, request) => {}` | Function for adding custom attributes to Fastify requests. Receives params: `Span, FastifyRequest`. | ### Using `requestHook` -Instrumentation configuration accepts a custom "hook" function which will be called for every instrumented Fastify layer involved in a request. Custom attributes can be set on the span or run any custom logic per layer. +Instrumentation configuration accepts a custom "hook" function which will be called for every instrumented Fastify request. Custom attributes can be set on the span or run any custom logic per request. ```javascript import { FastifyInstrumentation } from "@opentelemetry/instrumentation-fastify" const fastifyInstrumentation = new FastifyInstrumentation({ - requestHook: function (span: Span, request: FastifyRequest) { + requestHook: function (span: Span, info: FastifyRequestInfo) { span.setAttribute( 'http.method', - request.method, + info.request.method, ) } }); diff --git a/plugins/node/opentelemetry-instrumentation-fastify/package.json b/plugins/node/opentelemetry-instrumentation-fastify/package.json index 567db08435..4068a60d0a 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/package.json +++ b/plugins/node/opentelemetry-instrumentation-fastify/package.json @@ -54,13 +54,11 @@ "@types/express": "4.17.13", "@types/mocha": "7.0.2", "@types/node": "16.11.21", - "@types/sinon": "10.0.13", "fastify": "^4.5.3", "gts": "3.1.0", "mocha": "7.2.0", "nyc": "15.1.0", "rimraf": "3.0.2", - "sinon": "14.0.1", "ts-mocha": "10.0.0", "typescript": "4.3.5" }, diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/index.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/index.ts index 34b600dd0c..958c8fef60 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/index.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/index.ts @@ -15,4 +15,5 @@ */ export * from './enums/AttributeNames'; +export * from './types'; export * from './instrumentation'; diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts index 44597b243a..3b30499410 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts @@ -286,10 +286,11 @@ export class FastifyInstrumentation extends InstrumentationBase { if (instrumentation.getConfig().requestHook) { safeExecuteInTheMiddle( - () => instrumentation.getConfig().requestHook!(span, request), + () => instrumentation.getConfig().requestHook!(span, { request }), e => { if (e) { - diag.error('fastify instrumentation: request hook failed', e); + diag.error('request hook failed', e); + instrumentation._diag.error('request hook failed', e); } }, true diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts index 055565bd72..2a419e3356 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts @@ -18,7 +18,6 @@ import { Span } from '@opentelemetry/api'; import type { FastifyReply } from 'fastify'; import { spanRequestSymbol } from './constants'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; -import type { FastifyRequest } from 'fastify/types/request'; export type HandlerOriginal = (() => Promise) & (() => void); @@ -26,19 +25,23 @@ export type PluginFastifyReply = FastifyReply & { [spanRequestSymbol]?: Span[]; }; +export type FastifyRequestInfo = { + request: any, // FastifyRequest object from fastify package +} + /** * Function that can be used to add custom attributes to the current span - * @param span - The Fastify layer span. - * @param request - The Fastify request object. + * @param span - The Fastify handler span. + * @param info - The Fastify request info object. */ export interface FastifyCustomAttributeFunction { - (span: Span, request: FastifyRequest): void; + (span: Span, info: FastifyRequestInfo): void; } /** * Options available for the Fastify Instrumentation */ export interface FastifyInstrumentationConfig extends InstrumentationConfig { - /** Function for adding custom attributes to each layer span */ + /** Function for adding custom attributes to each handler span */ requestHook?: FastifyCustomAttributeFunction; } diff --git a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts index 2e0c608c79..a018639d69 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts @@ -32,7 +32,7 @@ import * as http from 'http'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; import { ANONYMOUS_NAME } from '../src/instrumentation'; import { AttributeNames, FastifyInstrumentation } from '../src'; -import * as sinon from 'sinon'; +import { FastifyRequestInfo } from '../src/types'; const URL = require('url').URL; @@ -431,9 +431,39 @@ describe('fastify', () => { describe('using requestHook in config', () => { it('calls requestHook provided function when set in config', async () => { - const requestHook = sinon.spy((span: Span, request: FastifyRequest) => { - span.setAttribute('http.method', request.method); + const requestHook = (span: Span, info: FastifyRequestInfo) => { + span.setAttribute('http.method', info.request.method); + }; + + instrumentation.setConfig({ + ...instrumentation.getConfig(), + requestHook, + }); + + app.get('/test', (req, res) => { + res.send('OK'); + }); + + await startServer(); + await httpRequest.get(`http://localhost:${PORT}/test`); + + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 5); + const span = spans[3]; + assert.deepStrictEqual(span.attributes, { + 'fastify.type': 'request_handler', + 'plugin.name': 'fastify -> @fastify/express', + [SemanticAttributes.HTTP_ROUTE]: '/test', + 'http.method': 'GET', }); + }); + + it('does not propagate an error from a requestHook that throws exception', async () => { + const requestHook = (span: Span, info: FastifyRequestInfo) => { + span.setAttribute('http.method', info.request.method); + + throw Error('error thrown in requestHook'); + }; instrumentation.setConfig({ ...instrumentation.getConfig(), From 44b7c5ff477d0f657c923936d11b4701961b2d8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luismi=20Ram=C3=ADrez?= Date: Thu, 3 Nov 2022 11:18:22 +0100 Subject: [PATCH 3/3] Address nit comments for requestHook - Fix types in docs - Make FastifyRequestInfo an interface - Use SemanticAttributes enums instead of plain strings - Remove extra diag error call - Some lint fixes --- .../README.md | 2 +- .../src/instrumentation.ts | 2 -- .../src/types.ts | 4 ++-- .../test/instrumentation.test.ts | 14 ++++++++++---- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-fastify/README.md b/plugins/node/opentelemetry-instrumentation-fastify/README.md index 2d50664008..3e74f96b8b 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/README.md +++ b/plugins/node/opentelemetry-instrumentation-fastify/README.md @@ -48,7 +48,7 @@ See [examples/fastify](https://github.com/open-telemetry/opentelemetry-js-contri ## Fastify Instrumentation Options | Options | Type | Example | Description | -| `requestHook` | `FastifyCustomAttributeFunction` | `(span, request) => {}` | Function for adding custom attributes to Fastify requests. Receives params: `Span, FastifyRequest`. | +| `requestHook` | `FastifyCustomAttributeFunction` | `(span, requestInfo) => {}` | Function for adding custom attributes to Fastify requests. Receives params: `Span, FastifyRequestInfo`. | ### Using `requestHook` diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts index c63f8c9b06..2ede9b2713 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts @@ -16,7 +16,6 @@ import { context, - diag, SpanAttributes, SpanStatusCode, trace, @@ -286,7 +285,6 @@ export class FastifyInstrumentation extends InstrumentationBase { () => instrumentation.getConfig().requestHook!(span, { request }), e => { if (e) { - diag.error('request hook failed', e); instrumentation._diag.error('request hook failed', e); } }, diff --git a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts index ec575d710b..2deadc0598 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/src/types.ts @@ -17,8 +17,8 @@ import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; -export type FastifyRequestInfo = { - request: any, // FastifyRequest object from fastify package +export interface FastifyRequestInfo { + request: any; // FastifyRequest object from fastify package } /** diff --git a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts index a018639d69..d80595728a 100644 --- a/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts +++ b/plugins/node/opentelemetry-instrumentation-fastify/test/instrumentation.test.ts @@ -432,7 +432,10 @@ describe('fastify', () => { describe('using requestHook in config', () => { it('calls requestHook provided function when set in config', async () => { const requestHook = (span: Span, info: FastifyRequestInfo) => { - span.setAttribute('http.method', info.request.method); + span.setAttribute( + SemanticAttributes.HTTP_METHOD, + info.request.method + ); }; instrumentation.setConfig({ @@ -454,13 +457,16 @@ describe('fastify', () => { 'fastify.type': 'request_handler', 'plugin.name': 'fastify -> @fastify/express', [SemanticAttributes.HTTP_ROUTE]: '/test', - 'http.method': 'GET', + [SemanticAttributes.HTTP_METHOD]: 'GET', }); }); it('does not propagate an error from a requestHook that throws exception', async () => { const requestHook = (span: Span, info: FastifyRequestInfo) => { - span.setAttribute('http.method', info.request.method); + span.setAttribute( + SemanticAttributes.HTTP_METHOD, + info.request.method + ); throw Error('error thrown in requestHook'); }; @@ -484,7 +490,7 @@ describe('fastify', () => { 'fastify.type': 'request_handler', 'plugin.name': 'fastify -> @fastify/express', [SemanticAttributes.HTTP_ROUTE]: '/test', - 'http.method': 'GET', + [SemanticAttributes.HTTP_METHOD]: 'GET', }); }); });