feat: restify instrumentation #416
style: fix linting errors
- Loading branch information
commit cbbdea00938fc81de601b04cc94cc3ebbd3a0879
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,103 +39,141 @@ export class RestifyInstrumentation extends InstrumentationBase< | |
| typeof restify | ||
| > { | ||
| constructor() { | ||
| super(`@opentelemetry/instrumentation-${MODULE_NAME}`, VERSION); | ||
| } | ||
|
|
||
| private _moduleVersion: string | undefined; | ||
| private _isDisabled = false; | ||
|
|
||
| init() { | ||
| const module = new InstrumentationNodeModuleDefinition<typeof restify>( | ||
| MODULE_NAME, | ||
| SUPPORTED_VERSIONS, | ||
| (moduleExports, moduleVersion) => { | ||
| this._moduleVersion = moduleVersion; | ||
| return moduleExports; | ||
| } | ||
| ); | ||
|
|
||
| module.files.push( | ||
| new InstrumentationNodeModuleFile<typeof restify>( | ||
| 'restify/lib/server.js', | ||
| SUPPORTED_VERSIONS, | ||
| (moduleExports, moduleVersion) => { | ||
| diag.debug(`Applying patch for ${MODULE_NAME}@${moduleVersion}`); | ||
| this._isDisabled = false; | ||
| const Server: any = moduleExports; | ||
| for (const name of RESTIFY_METHODS) { | ||
| if (isWrapped(Server.prototype[name])) { | ||
| this._unwrap(Server.prototype, name); | ||
| } | ||
| this._wrap( | ||
| Server.prototype, | ||
| name as keyof Server, | ||
| this._methodPatcher.bind(this) | ||
| ); | ||
| } | ||
| for (const name of RESTIFY_MW_METHODS) { | ||
| if (isWrapped(Server.prototype[name])) { | ||
| this._unwrap(Server.prototype, name); | ||
| } | ||
| this._wrap( | ||
| Server.prototype, | ||
| name as keyof Server, | ||
| this._middlewarePatcher.bind(this) | ||
| ); | ||
| } | ||
| return moduleExports; | ||
| }, | ||
| (moduleExports, moduleVersion) => { | ||
| diag.debug(`Removing patch for ${MODULE_NAME}@${moduleVersion}`); | ||
| this._isDisabled = true; | ||
| if (moduleExports) { | ||
| const Server: any = moduleExports; | ||
| for (const name of RESTIFY_METHODS) { | ||
| this._unwrap(Server.prototype, name as keyof Server); | ||
| } | ||
| for (const name of RESTIFY_MW_METHODS) { | ||
| this._unwrap(Server.prototype, name as keyof Server); | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| ); | ||
|
|
||
| return module; | ||
| } | ||
|
|
||
| private _middlewarePatcher(original: Function, methodName?: string) { | ||
| const instrumentation = this; | ||
| return function (this: Server, ...handler: types.NestedRequestHandlers) { | ||
| return original.call( | ||
| this, | ||
| instrumentation._handlerPatcher( | ||
| { type: types.LayerType.MIDDLEWARE, methodName }, | ||
| handler | ||
| ) | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| private _methodPatcher(original: Function, methodName?: string) { | ||
| const instrumentation = this; | ||
| return function ( | ||
| this: Server, | ||
| path: any, | ||
| ...handler: types.NestedRequestHandlers | ||
| ) { | ||
| return original.call( | ||
| this, | ||
| path, | ||
| ...instrumentation._handlerPatcher( | ||
|
Member
There was a problem hiding this comment. I like how cleanly you've kept the reuse between methodPatcher and middlewarePatcher - not too complex but still keeping the essential bits parameterized instead of copied+pasted. |
||
| { type: types.LayerType.REQUEST_HANDLER, path, methodName }, | ||
| handler | ||
| ) | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| // will return the same type as `handler`, but all functions recusively patched | ||
| private _handlerPatcher( | ||
| metadata: types.Metadata, | ||
| handler: restify.RequestHandler | types.NestedRequestHandlers | ||
| ): any { | ||
| if (Array.isArray(handler)) { | ||
| return handler.map(handler => this._handlerPatcher(metadata, handler)); | ||
| } | ||
| if (typeof handler === 'function') { | ||
| return ( | ||
| req: types.Request, | ||
| res: restify.Response, | ||
| next: restify.Next | ||
| ) => { | ||
| if (this._isDisabled) { | ||
| return handler(req, res, next); | ||
| } | ||
| // const parentSpan = api.getSpan(api?.context?.active()); | ||
|
||
| const route = | ||
| typeof req.getRoute === 'function' | ||
| ? req.getRoute()?.path | ||
| : req.route?.path; | ||
| const spanName = route || metadata.methodName || metadata.type; | ||
| const attributes = { | ||
| [types.CustomAttributeNames.VERSION]: this._moduleVersion || 'n/a', | ||
| [types.CustomAttributeNames.TYPE]: metadata.type, | ||
| [types.CustomAttributeNames.METHOD]: metadata.methodName, | ||
| [HttpAttribute.HTTP_ROUTE]: route, | ||
| }; | ||
| const span = this.tracer.startSpan( | ||
| spanName, | ||
| { | ||
| attributes, | ||
| }, | ||
| api.context.active() | ||
| ); // TODO: <- with this I intend to find and attach all consecutive handlers to HTTP span | ||
| // .. but instead all spans are attached to the previous handler's span. | ||
| const endSpan = once(span.end.bind(span)); | ||
vmarchaud marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const patchedNext = (err?: any) => { | ||
| endSpan(); | ||
| next(err); | ||
| }; | ||
|
|
@@ -157,7 +195,7 @@ export class RestifyInstrumentation extends InstrumentationBase< | |
| req, | ||
| res, | ||
| patchedNext | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think middleware spans should have middleware name as the span name (not sure if possibly with restify), e.g. the operation tree looks like this currently, where each middleware operation name is the route name:
However the express instrumentation contains middleware names:
Also I think middleware spans should have the incoming request span as its parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's possible. Express takes the name of the function it seems, which can be undefined at times, but still better than nothing. Thanks!
I generally agree but don't want to change that because there might not be a request span if HTTP instrumentation is not enabled, so the structure would be different between HTTP instrumentation enabled and not.