feat: net module instrumentation

[seemk]

Which problem is this PR solving?

net module instrumentation (fixes #291)

Short description of the changes

• Instrumentation for Socket.connect (thus also providing instrumentation for net.connect and net.createConnection)
• In case of successful TCP connections, the local socket attributes (host, port, ip) are added.
• timeout event is not handled on purpose as this event is not related to connect. When connect fails with a timeout, it will emit an error instead (e.g. EATIMEOUT).

[codecov]

Codecov Report

Merging #389 (095fad9) into main (9244a08) will not change coverage.
The diff coverage is n/a.

❗ Current head 095fad9 differs from pull request most recent head 1b2b3a0. Consider uploading reports for the commit 1b2b3a0 to get more accurate results

@@           Coverage Diff           @@
##             main     #389   +/-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files          10       10           
  Lines         407      407           
  Branches       44       44           
=======================================
  Hits          384      384           
  Misses         23       23           

[vmarchaud]

overall looks good, few nitpicks

[johnbley]

Looks really good; one small question about error handling which I don't consider a blocker.

[obecny]

thx for all changes, I'm missing two final things:

• a README.md with example usage
• LICENSE - please copy it from any other instrumentation

Extra:
An image showing the span that is being added by this plugin would be a nice addition but it is not a blocker.

[obecny]

btw I tried to assign you the issue you worked on but it seems you have to comment on that before I can do it

[seemk]

Added a LICENSE and README.md (along with references to the package even though it is not published yet).

For an example how this looks like in Zipkin when using an express application which is doing an outbound request:

Do note that the connect span is attached to the wrong parent, but this is an issue with the http instrumentation where it does not activate the correct context. It did once, but it was removed with open-telemetry/opentelemetry-js#1479 (PR open-telemetry/opentelemetry-js#1546) I think it was wrong to remove it and will create PR with a fix for http instrumentation.

[obecny]

thx a lot for your contribution, this looks great!

[johanneswuerbach]

This looks great, would you be open to accept a contribute to also add a span for the lookup event https://nodejs.org/api/net.html#net_event_lookup, which is generally useful to troubleshoot connection performance as especially k8s was prone to DNS timeouts?

[vmarchaud]

@johanneswuerbach shouldn't this be handled by the dns instrumentation ?