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

[blumamir]

Enable users of the http plugin to register hooks on http request and response. The hooks are called before the stream begins to handle data, allowing the plugin user to register to events.

Which problem is this PR solving?

• Access to the request and response stream, which enable to capture the http body in the plugin.
  Add custom span attributes to the span ASAP.
• In case of error or exception in the request handling, the existing applyCustomAttributesOnSpan function is never called, and it is impossible to add custom attributes to span.

Short description of the changes

• Add two new options to the HttpPluginConfig: requestHook, responseHook, similar to the existing applyCustomAttributesOnSpan option. If set, these callbacks are called by the http plugin, allowing the plugin user to add custom span attributes and execute logic right after the stream creation and before data is sent on it.

[codecov-io]

Codecov Report

Merging #963 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #963      +/-   ##
==========================================
+ Coverage   95.10%   95.12%   +0.01%     
==========================================
  Files         213      213              
  Lines        8914     8944      +30     
  Branches      800      804       +4     
==========================================
+ Hits         8478     8508      +30     
  Misses        436      436              

Impacted Files	Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts	97.87% <100.00%> (+0.17%)	⬆️
...y-plugin-http/test/functionals/http-enable.test.ts	97.02% <100.00%> (+0.12%)	⬆️

[vmarchaud]

Overall it seems fine but i don't see the point of keeping applyCustomAttributesOnSpan if we have this system. Should we just drop it and use those hooks ? Paging @OlivierAlbertini since if i remember correctly you had implement that because you needed it

[blumamir]

Overall it seems fine but i don't see the point of keeping applyCustomAttributesOnSpan if we have this system. Should we just drop it and use those hooks ? Paging @OlivierAlbertini since if i remember correctly you had implement that because you needed it

Thanks for the comment. The applyCustomAttributesOnSpan callback is still needed since it is called after the response ended and all the data is available on the request and response objects (like the response http headers for example).

[dyladan]

In general, I'm worried about the performance impact of another function which is called on every request and response on both the client and the server side, and I'm not sure I see the value.

[blumamir]

In general, I'm worried about the performance impact of another function which is called on every request and response on both the client and the server side, and I'm not sure I see the value.

Thanks for taking a look
Same as the current applyCustomAttributesOnSpan, these hooks are called only if configured in the HttpPluginConfig. If not set, no extra function will be called.
Regarding the value:

• I need to sometimes also capture the request and response body, which requires to register to the stream events: on('data', ...), and to patch the write function of the ClientRequest before it's called.
• The existing function applyCustomAttributesOnSpan is called only if the request ended successfully. In case of error \ exception, the plugin user has no option to add custom attributes with the current hook.

[obecny]

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 ?

[blumamir]

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.

[dyladan]

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.

[blumamir]

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

[obecny]

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

[blumamir]

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);

[blumamir]

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

[markwolff]

LGTM after latest changes

[dyladan]

LGTM please fix the build

[mayurkale22]

@OlivierAlbertini would be nice if you can review this one.

[dyladan]

@blumamir are you going to make the changes suggested by @obecny ?

[blumamir]

@blumamir are you going to make the changes suggested by @obecny ?

I like the concept of using the event emitter, but specifically in this case the request and response objects are received via patched function parameters:

        return function incomingRequest(this: {}, event: string, ...args: unknown[]): boolean {
            ...
            const request = args[0] as IncomingMessage;
            const response = args[1] as ServerResponse & { socket: Socket };

Or return value from patched function

                const request: ClientRequest = plugin._safeExecute(
                    span,
                    () => original.apply(this, [optionsParsed, ...args]),
                    true
                );

So there is no place where I can register once for an event and get the request and response objects when they are are created.
I think the current implementation (adding two extra if's per request to check the config), is the right way to go here.

[dyladan]

@obecny I was going to merge this but would like your go/no-go.

[obecny]

@dyladan I don't want to block it anymore, we both wanted this to be done in a way I described - split this into 2 events instead of one, but if that won't happen then we can have it as it is :), thx

[dyladan]

I'm going to merge it, we can address performance concerns if they come up.