Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(plugin-http): add plugin hooks before processing req and res #963

Merged
merged 8 commits into from
May 13, 2020
+101 −1
Merged

feat(plugin-http): add plugin hooks before processing req and res #963

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b348ec7
feat(plugin-http): add plugin hooks before processing req and res
blumamir Apr 13, 2020
8e8d753
fix(http-plugin): response hook bind to right plugin instance
blumamir Apr 14, 2020
d49c563
perf(plugin-http): remove function calls when no hooks are configured
blumamir Apr 21, 2020
6fdc120
Merge branch 'master' into req-res-hooks
blumamir May 3, 2020
51151df
style(plugin-http): lint fix
blumamir May 3, 2020
64e9def
Merge branch 'master' into req-res-hooks
dyladan May 6, 2020
3841e1c
Merge branch 'master' into req-res-hooks
mayurkale22 May 13, 2020
4219db8
Merge branch 'master' into req-res-hooks
dyladan May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/opentelemetry-plugin-http/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ Http plugin has few options available to choose from. You can set the following:
| Options | Type | Description |
| ------- | ---- | ----------- |
| [`applyCustomAttributesOnSpan`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L52) | `HttpCustomAttributeFunction` | Function for adding custom attributes |
| [`requestHook`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L60) | `HttpRequestCustomAttributeFunction` | Function for adding custom attributes before request is handled |
| [`responseHook`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L67) | `HttpResponseCustomAttributeFunction` | Function for adding custom attributes before response is handled |
| [`ignoreIncomingPaths`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all incoming requests that match paths |
| [`ignoreOutgoingUrls`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all outgoing requests that match urls |
| [`serverName`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `string` | The primary server name of the matched virtual host. |
Expand Down
28 changes: 28 additions & 0 deletions packages/opentelemetry-plugin-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ export class HttpPlugin extends BasePlugin<Http> {
hostname,
});
span.setAttributes(attributes);
this._onNewRequest(span, request);

request.on(
'response',
Expand All @@ -200,6 +201,7 @@ export class HttpPlugin extends BasePlugin<Http> {
{ hostname }
);
span.setAttributes(attributes);
this._onNewResponse(span, response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that probably it would be better if you modify this whole concept a bit because of performance issues.

First Approach
During a plugin initialisation check if user defined a hooks in config if that is the case then add simply a different event listener (request.on) with your modifications plugin._onNewRequest etc. otherwise use the one without your modifications.
This will have some code duplication but in fact 0 performance issue if user doesn't define any hooks.

Second Approach
Also during initialisation check if user defined a hooks in config but this time add separate event listener and then call plugin._onNewRequest.
This could be achieved by adding extra parameter to _traceClientRequest.
This would be basically a callback for setting up the desired events, but if user doesn't define the hooks this callback will be simply something like NoopHookCallback which will look like function () {} and can be called before returning the request inside function _traceClientRequest

WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review,

Regarding the performance impact, when these function are not set, the only performance issue is function call to _onNewRequest \ _onNewResponse, and executing an if: if (this._config.requestHook). Is it something to worry about?

On plugin initialisation I still have no request`responseobjects on which I can add events. I call the function to register events as soon as therequest` object is available (in incoming or outgoing functions).

What I can do is to register on the http.Server request Event, but then I'll get a callback without the relevant span, and it will be called every-time, also when the request is ignored in open telemetry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the performance impact, when these function are not set, the only performance issue is function call to _onNewRequest \ _onNewResponse, and executing an if: if (this._config.requestHook). Is it something to worry about?

Yes. When this is done on every single request and response on both client and server it is something to worry about. Function call overhead is non-negligible. Right now you have a function which is called every time and an if-check that happens inside the function. Moving the if-check outside of the function and only calling it if there is something configured would be drastically more performant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dyladan
I wasn't aware of the impact of the function call. Will perform the if-check before calling the function and push a new commit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change you did will have exactly the same number of checks then previously

request.on('some event', a);

function a() {
  b();
  console.log('bar');
}

function b() {
  if (config.c) {
    console.log('foo');
  }
}

is exactly the same as

request.on('some event', a);

function a() {
  if (config.c) {
    b();
  }
  console.log('bar');
}

function b() {
  console.log('foo');
}

What I tried to say is to refactor into something like this

if (config.c) {
  request.on('some event', a1);
} else {
  request.on('some event', a2);
}

function a1() {
  b();
  console.log('bar');
}

function a2() {
  console.log('bar');
}

function b() {
  console.log('foo');
}

this way you will have some duplication in code, but when it is about performance, you will check only once for config.c rather then every time on some event

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, missed the comment.
I like the idea of registering to the EventEmitter interface.
In this case, the hook is exposing the request object (which we get as the patched function parameter) so the user is able to do function calls like request.on('some event', a1);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more precise, the above code still execute the if (config.c) per request which is the same as what current implementation is doing


this._tracer.bind(response);
this._logger.debug('outgoingRequest on response()');
Expand Down Expand Up @@ -304,6 +306,9 @@ export class HttpPlugin extends BasePlugin<Http> {
context.bind(request);
context.bind(response);

plugin._onNewRequest(span, request);
plugin._onNewResponse(span, response);

// Wraps end (inspired by:
// https://github.com/GoogleCloudPlatform/cloud-trace-nodejs/blob/master/src/plugins/plugin-connect.ts#L75)
const originalEnd = response.end;
Expand Down Expand Up @@ -433,6 +438,29 @@ export class HttpPlugin extends BasePlugin<Http> {
this._spanNotEnded.delete(span);
}

private _onNewResponse(
span: Span,
response: IncomingMessage | ServerResponse
) {
if (this._config.responseHook) {
this._safeExecute(
span,
() => this._config.responseHook!(span, response),
false
);
}
}

private _onNewRequest(span: Span, request: ClientRequest | IncomingMessage) {
if (this._config.requestHook) {
this._safeExecute(
span,
() => this._config.requestHook!(span, request),
false
);
}
}

private _safeExecute<
T extends (...args: unknown[]) => ReturnType<T>,
K extends boolean
Expand Down
14 changes: 13 additions & 1 deletion packages/opentelemetry-plugin-http/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ export interface HttpCustomAttributeFunction {
): void;
}

export interface HttpRequestCustomAttributeFunction {
(span: Span, request: ClientRequest | IncomingMessage): void;
}

export interface HttpResponseCustomAttributeFunction {
(span: Span, response: IncomingMessage | ServerResponse): void;
}

/**
* Options available for the HTTP Plugin (see [documentation](https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-plugin-http#http-plugin-options))
*/
Expand All @@ -65,8 +73,12 @@ export interface HttpPluginConfig extends PluginConfig {
ignoreIncomingPaths?: IgnoreMatcher[];
/** Not trace all outgoing requests that match urls */
ignoreOutgoingUrls?: IgnoreMatcher[];
/** Function for adding custom attributes */
/** Function for adding custom attributes after response is handled */
applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
/** Function for adding custom attributes before request is handled */
requestHook?: HttpRequestCustomAttributeFunction;
/** Function for adding custom attributes before response is handled */
responseHook?: HttpResponseCustomAttributeFunction;
/** The primary server name of the matched virtual host. */
serverName?: string;
}
Expand Down
51 changes: 51 additions & 0 deletions packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import { DummyPropagation } from '../utils/DummyPropagation';
import { httpRequest } from '../utils/httpRequest';
import { ContextManager } from '@opentelemetry/context-base';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
import { ClientRequest, IncomingMessage, ServerResponse } from 'http';

const applyCustomAttributesOnSpanErrorMessage =
'bad applyCustomAttributesOnSpan function';
Expand Down Expand Up @@ -76,6 +77,20 @@ export const customAttributeFunction = (span: ISpan): void => {
span.setAttribute('span kind', SpanKind.CLIENT);
};

export const requestHookFunction = (
span: ISpan,
request: ClientRequest | IncomingMessage
): void => {
span.setAttribute('custom request hook attribute', 'request');
};

export const responseHookFunction = (
span: ISpan,
response: IncomingMessage | ServerResponse
): void => {
span.setAttribute('custom response hook attribute', 'response');
};

describe('HttpPlugin', () => {
let contextManager: ContextManager;

Expand Down Expand Up @@ -203,6 +218,8 @@ describe('HttpPlugin', () => {
(url: string) => url.endsWith(`/ignored/function`),
],
applyCustomAttributesOnSpan: customAttributeFunction,
requestHook: requestHookFunction,
responseHook: responseHookFunction,
serverName,
};
plugin.enable(http, provider, provider.logger, config);
Expand Down Expand Up @@ -695,6 +712,40 @@ describe('HttpPlugin', () => {
});
req.end();
});

it('custom attributes should show up on client and server spans', async () => {
await httpRequest.get(
`${protocol}://${hostname}:${serverPort}${pathname}`
);
const spans = memoryExporter.getFinishedSpans();
const [incomingSpan, outgoingSpan] = spans;

assert.strictEqual(
incomingSpan.attributes['custom request hook attribute'],
'request'
);
assert.strictEqual(
incomingSpan.attributes['custom response hook attribute'],
'response'
);
assert.strictEqual(
incomingSpan.attributes['span kind'],
SpanKind.CLIENT
);

assert.strictEqual(
outgoingSpan.attributes['custom request hook attribute'],
'request'
);
assert.strictEqual(
outgoingSpan.attributes['custom response hook attribute'],
'response'
);
assert.strictEqual(
outgoingSpan.attributes['span kind'],
SpanKind.CLIENT
);
});
});
});
});