From c66dbf9a38f19a2addfb50dcae0bfbf9b725a633 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 29 Nov 2022 10:32:03 +0100 Subject: [PATCH 1/9] feat(restify): add requestHook support The `requestHook` config option allows custom span handling per request layer. --- .../README.md | 21 ++++++ .../src/index.ts | 1 + .../src/instrumentation.ts | 28 +++++++- .../src/types.ts | 39 +++++++++++ .../test/restify.test.ts | 69 +++++++++++++++++++ 5 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 plugins/node/opentelemetry-instrumentation-restify/src/types.ts diff --git a/plugins/node/opentelemetry-instrumentation-restify/README.md b/plugins/node/opentelemetry-instrumentation-restify/README.md index 9ef35ea4da..719a042ab7 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/README.md +++ b/plugins/node/opentelemetry-instrumentation-restify/README.md @@ -41,6 +41,27 @@ registerInstrumentations({ See [examples/restify](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/restify) for a short example. +## Restify Instrumentation Options + +| Options | Type | Example | Description | +| `requestHook` | `RestifyCustomAttributeFunction` | `(span, requestInfo) => {}` | Function for adding custom attributes to restify requests. Receives params: `Span, RestifyRequestInfo`. | + +### Using `requestHook` + +Instrumentation configuration accepts a custom "hook" function which will be called for every instrumented restify request. Custom attributes can be set on the span or run any custom logic per request. + +```javascript +import { RestifyInstrumentation } from "@opentelemetry/instrumentation-restify" +const restifyInstrumentation = new RestifyInstrumentation({ + requestHook: function (span: Span, info: RestifyRequestInfo) { + span.setAttribute( + 'http.method', + info.request.method, + ) + } +}); +``` + ## Useful links - For more information on OpenTelemetry, visit: diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/index.ts b/plugins/node/opentelemetry-instrumentation-restify/src/index.ts index dae24360d1..331bf99930 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/index.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/index.ts @@ -20,3 +20,4 @@ export * from './instrumentation'; export default RestifyInstrumentation; export * from './enums/AttributeNames'; +export * from './types'; diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index 76fb7b95f9..6fa1166204 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -25,25 +25,34 @@ import { VERSION } from './version'; import * as constants from './constants'; import { InstrumentationBase, - InstrumentationConfig, InstrumentationNodeModuleDefinition, InstrumentationNodeModuleFile, isWrapped, + safeExecuteInTheMiddle, } from '@opentelemetry/instrumentation'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { isPromise, isAsyncFunction } from './utils'; import { getRPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core'; +import type { RestifyInstrumentationConfig } from './types'; const { diag } = api; export class RestifyInstrumentation extends InstrumentationBase { - constructor(config: InstrumentationConfig = {}) { + constructor(config: RestifyInstrumentationConfig = {}) { super(`@opentelemetry/instrumentation-${constants.MODULE_NAME}`, VERSION); } private _moduleVersion?: string; private _isDisabled = false; + override setConfig(config: RestifyInstrumentationConfig = {}) { + this._config = Object.assign({}, config); + } + + override getConfig(): RestifyInstrumentationConfig { + return this._config as RestifyInstrumentationConfig; + } + init() { const module = new InstrumentationNodeModuleDefinition( constants.MODULE_NAME, @@ -185,6 +194,21 @@ export class RestifyInstrumentation extends InstrumentationBase { }, api.context.active() ); + + const instrumentation = this; + const requestHook = instrumentation.getConfig().requestHook + if (requestHook) { + safeExecuteInTheMiddle( + () => requestHook!(span, { request: req }), + (e) => { + if (e) { + instrumentation._diag.error('request hook failed', e); + } + }, + true + ); + } + const patchedNext = (err?: any) => { span.end(); next(err); diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts new file mode 100644 index 0000000000..1b2ca2b49b --- /dev/null +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -0,0 +1,39 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Span } from '@opentelemetry/api'; +import { InstrumentationConfig } from '@opentelemetry/instrumentation'; + +export interface RestifyRequestInfo { + request: any; // RestifyRequest object from restify package +} + +/** + * Function that can be used to add custom attributes to the current span + * @param span - The restify handler span. + * @param info - The restify request info object. + */ +export interface RestifyCustomAttributeFunction { + (span: Span, info: RestifyRequestInfo): void; +} + +/** + * Options available for the restify Instrumentation + */ +export interface RestifyInstrumentationConfig extends InstrumentationConfig { + /** Function for adding custom attributes to each handler span */ + requestHook?: RestifyCustomAttributeFunction; +} diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index 33ce317188..70e23a2d3c 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -15,6 +15,7 @@ */ import { context, trace } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; @@ -487,6 +488,74 @@ describe('Restify Instrumentation', () => { assert.strictEqual(memoryExporter.getFinishedSpans().length, 3); assert.strictEqual(res, '{"route":"bar"}'); }); + + describe('using requestHook in config', () => { + it('calls requestHook provided function when set in config', async () => { + const requestHook = (span: Span, info: RestifyRequestInfo) => { + span.setAttribute( + SemanticAttributes.HTTP_METHOD, + info.request.method + ); + }; + + plugin.setConfig({ + ...plugin.getConfig(), + requestHook, + }); + + const rootSpan = tracer.startSpan('clientSpan'); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}/route/foo`); + rootSpan.end(); + assert.strictEqual(memoryExporter.getFinishedSpans().length, 4); + + { + // span from get + const span = memoryExporter.getFinishedSpans()[2]; + assert.notStrictEqual(span, undefined); + assert.strictEqual(span.attributes[SemanticAttributes.HTTP_METHOD], 'GET'); + } + } + ); + }); + + it('does not propagate an error from a requestHook that throws exception', async () => { + const requestHook = (span: Span, info: RestifyRequestInfo) => { + span.setAttribute( + SemanticAttributes.HTTP_METHOD, + info.request.method + ); + + throw Error('error thrown in requestHook'); + }; + + plugin.setConfig({ + ...plugin.getConfig(), + requestHook, + }); + + const rootSpan = tracer.startSpan('clientSpan'); + + await context.with( + trace.setSpan(context.active(), rootSpan), + async () => { + await httpRequest.get(`http://localhost:${port}/route/foo`); + rootSpan.end(); + assert.strictEqual(memoryExporter.getFinishedSpans().length, 4); + + { + // span from get + const span = memoryExporter.getFinishedSpans()[2]; + assert.notStrictEqual(span, undefined); + assert.strictEqual(span.attributes[SemanticAttributes.HTTP_METHOD], 'GET'); + } + } + ); + }); + }); }); describe('Disabling restify instrumentation', () => { From 7475274f4a631bbea070c7e7b38975d6f965013a Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 09:41:54 +0100 Subject: [PATCH 2/9] fix(restify): pass config to super class As mentioned in the review, pass the instrumentation config to the parent class. That way the config is also stored when given to the initializer, rather only when using the `setConfig` function. --- .../src/instrumentation.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index 6fa1166204..27f7d221f2 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -39,7 +39,11 @@ const { diag } = api; export class RestifyInstrumentation extends InstrumentationBase { constructor(config: RestifyInstrumentationConfig = {}) { - super(`@opentelemetry/instrumentation-${constants.MODULE_NAME}`, VERSION); + super( + `@opentelemetry/instrumentation-${constants.MODULE_NAME}`, + VERSION, + Object.assign({}, config) + ); } private _moduleVersion?: string; From b0419107637eeb22e14d797cab4bd80f29a43f24 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 09:47:18 +0100 Subject: [PATCH 3/9] fix(restify): fix comment referencing restify type Update comment to reference to correct type from the `@types/restify` package. --- plugins/node/opentelemetry-instrumentation-restify/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 1b2ca2b49b..3534493163 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -18,7 +18,7 @@ import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; export interface RestifyRequestInfo { - request: any; // RestifyRequest object from restify package + request: any; // Restify type from restify types package } /** From ba12a4a94fb13fe68b97e722ece0e1eec1a3f0ed Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 09:59:44 +0100 Subject: [PATCH 4/9] fix(restify): import missing Span type Add the missing import reported by the linter. --- .../opentelemetry-instrumentation-restify/test/restify.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index 70e23a2d3c..93b9b3079d 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { context, trace } from '@opentelemetry/api'; +import { context, trace, Span } from '@opentelemetry/api'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { RPCType, setRPCMetadata } from '@opentelemetry/core'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; From 9d233b28f663e7c05ca4b904d7bc3c6d63d10fda Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 10:00:13 +0100 Subject: [PATCH 5/9] fix(restify): fix issues reported by linter --- .../src/instrumentation.ts | 4 ++-- .../test/restify.test.ts | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index 27f7d221f2..71a95c1eb5 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -200,11 +200,11 @@ export class RestifyInstrumentation extends InstrumentationBase { ); const instrumentation = this; - const requestHook = instrumentation.getConfig().requestHook + const requestHook = instrumentation.getConfig().requestHook; if (requestHook) { safeExecuteInTheMiddle( () => requestHook!(span, { request: req }), - (e) => { + e => { if (e) { instrumentation._diag.error('request hook failed', e); } diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index 93b9b3079d..0257f8c18b 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -516,7 +516,10 @@ describe('Restify Instrumentation', () => { // span from get const span = memoryExporter.getFinishedSpans()[2]; assert.notStrictEqual(span, undefined); - assert.strictEqual(span.attributes[SemanticAttributes.HTTP_METHOD], 'GET'); + assert.strictEqual( + span.attributes[SemanticAttributes.HTTP_METHOD], + 'GET' + ); } } ); @@ -550,7 +553,10 @@ describe('Restify Instrumentation', () => { // span from get const span = memoryExporter.getFinishedSpans()[2]; assert.notStrictEqual(span, undefined); - assert.strictEqual(span.attributes[SemanticAttributes.HTTP_METHOD], 'GET'); + assert.strictEqual( + span.attributes[SemanticAttributes.HTTP_METHOD], + 'GET' + ); } } ); From eda13b168aa92517a8aee3e0c135294ba46a0976 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 10:01:20 +0100 Subject: [PATCH 6/9] fix(restify): fix comment referencing restify type Mention the package name exactly. --- plugins/node/opentelemetry-instrumentation-restify/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 3534493163..249049b2ac 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -18,7 +18,7 @@ import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; export interface RestifyRequestInfo { - request: any; // Restify type from restify types package + request: any; // Restify type from @types/restify package } /** From 9174428a11c2c4f599f53318bb8ea9c85f4b60e0 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 10:07:51 +0100 Subject: [PATCH 7/9] fix(restify): fix comment referencing restify type Co-authored-by: Amir Blum --- plugins/node/opentelemetry-instrumentation-restify/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 249049b2ac..6e93761b74 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -18,7 +18,7 @@ import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; export interface RestifyRequestInfo { - request: any; // Restify type from @types/restify package + request: any; // Request type from @types/restify package } /** From de7b5fe11fdf69fca1036b8db9067772bf2358c5 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 10:26:22 +0100 Subject: [PATCH 8/9] fix(restify): add missing import in restify test --- .../opentelemetry-instrumentation-restify/test/restify.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index 0257f8c18b..64d51b9503 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -26,6 +26,7 @@ import { import RestifyInstrumentation from '../src'; import * as types from '../src/internal-types'; +import { RestifyRequestInfo } from '../src/types'; const plugin = new RestifyInstrumentation(); import * as semver from 'semver'; From 55a0d4bf9f81e7488564571bbf0a39dff3e701c7 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 30 Nov 2022 13:59:36 +0100 Subject: [PATCH 9/9] feat(restify): add layer argument to requestHook Add the layerType argument to the requestHook function. This is like the following PR but for restify: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1226 Move the LayerType from internal-types to types, because it's now used in a function used by users of the instrumentation package. --- .../src/instrumentation.ts | 9 +++++++-- .../src/internal-types.ts | 6 +----- .../opentelemetry-instrumentation-restify/src/types.ts | 6 ++++++ .../test/restify.test.ts | 5 +++++ 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts index 71a95c1eb5..a856359d96 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts @@ -19,7 +19,7 @@ import type * as restify from 'restify'; import * as api from '@opentelemetry/api'; import type { Server } from 'restify'; -import { LayerType } from './internal-types'; +import { LayerType } from './types'; import * as AttributeNames from './enums/AttributeNames'; import { VERSION } from './version'; import * as constants from './constants'; @@ -203,7 +203,12 @@ export class RestifyInstrumentation extends InstrumentationBase { const requestHook = instrumentation.getConfig().requestHook; if (requestHook) { safeExecuteInTheMiddle( - () => requestHook!(span, { request: req }), + () => { + return requestHook!(span, { + request: req, + layerType: metadata.type, + }); + }, e => { if (e) { instrumentation._diag.error('request hook failed', e); diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/internal-types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/internal-types.ts index d6280a6bda..deb0ea3e98 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/internal-types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/internal-types.ts @@ -15,11 +15,7 @@ */ import { Span } from '@opentelemetry/api'; import type * as restify from 'restify'; - -export enum LayerType { - MIDDLEWARE = 'middleware', - REQUEST_HANDLER = 'request_handler', -} +import { LayerType } from './types'; declare interface RequestWithRoute extends restify.Request { route: { path: string }; diff --git a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts index 6e93761b74..238f247a81 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/src/types.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/src/types.ts @@ -17,8 +17,14 @@ import { Span } from '@opentelemetry/api'; import { InstrumentationConfig } from '@opentelemetry/instrumentation'; +export enum LayerType { + MIDDLEWARE = 'middleware', + REQUEST_HANDLER = 'request_handler', +} + export interface RestifyRequestInfo { request: any; // Request type from @types/restify package + layerType: LayerType; } /** diff --git a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts index 64d51b9503..0e381bf969 100644 --- a/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts +++ b/plugins/node/opentelemetry-instrumentation-restify/test/restify.test.ts @@ -497,6 +497,7 @@ describe('Restify Instrumentation', () => { SemanticAttributes.HTTP_METHOD, info.request.method ); + span.setAttribute('restify.layer', info.layerType); }; plugin.setConfig({ @@ -521,6 +522,10 @@ describe('Restify Instrumentation', () => { span.attributes[SemanticAttributes.HTTP_METHOD], 'GET' ); + assert.strictEqual( + span.attributes['restify.layer'], + 'request_handler' + ); } } );