fix(http-plugin): ensure exceptions are handled

[OlivierAlbertini]

Which problem is this PR solving?

Ensure exceptions are properly handled.

• closes Ensure exceptions are properly handled and propagated #222

Short description of the changes

• refactor(test): remove unnecessary code
• add tests

[markwolff]

LGTM. Side question: I think this is out of the scope of this PR but do we also need to handle 'error' and 'aborted' events for this plugin's event listener? I see only 'response' is listened to currently, or am I missing something?

[OlivierAlbertini]

LGTM. Side question: I think this is out of the scope of this PR but do we also need to handle 'error' and 'aborted' events for this plugin's event listener? I see only 'response' is listened to currently, or am I missing something?

we use error as well on resp and req.

opentelemetry-js/packages/opentelemetry-plugin-http/src/http.ts

Line 219 in 82b4110

Utils.setSpanOnError(span, response);

Also you can see related tests.

aborted is not used because of #225
Perhaps we could discuss about this in the next SIG meeting ? /cc @mayurkale22 (I have no expertise on how we could do it without using events... If someone can refer me to an implementation... I would be curious)

[danielkhan]

I pinged @Flarna to have a look as well

[codecov-io]

Codecov Report

Merging #273 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   98.86%   98.87%   +0.01%     
==========================================
  Files          69       70       +1     
  Lines        2732     2856     +124     
  Branches      193      194       +1     
==========================================
+ Hits         2701     2824     +123     
- Misses         31       32       +1

Impacted Files	Coverage Δ
...pentelemetry-node-sdk/src/instrumentation/utils.ts	96% <0%> (-4%)	⬇️
...node-sdk/test/instrumentation/PluginLoader.test.ts	100% <0%> (ø)	⬆️
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-node-sdk/src/config.ts	100% <0%> (ø)

[Flarna]

I think a try/catch around const request: ClientRequest = original.apply(this, [options, ...args]); is needed to end the span created just before in case the original throws.

Similar const returned = response.end.apply(this, arguments as any); in wrapped response.end.

[OlivierAlbertini]

const request: ClientRequest = original.apply(this, [options, ...args]);

Do you have a use case / exemple ? Right now, I don't see how I could reproduce the behaviour you mention. We check options and tests show that behaviour is respected. base on NodeJS spec, if options is ok, it will fire events. If I missed something let me know.

Thanks in advance.

[Flarna]

Do you have a use case / exemple ?

http.request({protocol: "telnet"}).

The plugin does not verify all data finally used by node http, it just checks the part needed by the plugin and the type of the object.
And I don't think the plugin should add more validation here as this would just mean we duplicate code from node and there will be always some gap left.

[OlivierAlbertini]

Do you have a use case / exemple ?

http.request({protocol: "telnet"}).

The plugin does not verify all data finally used by node http, it just checks the part needed by the plugin and the type of the object.
And I don't think the plugin should add more validation here as this would just mean we duplicate code from node and there will be always some gap left.

If you have other use cases please let me know.