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.

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: net module instrumentation #389

Merged
merged 17 commits into from
Mar 19, 2021

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Mar 17, 2021

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

@seemk seemk requested a review from a team March 17, 2021 09:03
@seemk seemk changed the title net module instrumentation feat: net module instrumentation Mar 17, 2021
@codecov
Copy link

codecov bot commented Mar 17, 2021

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           

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

overall looks good, few nitpicks

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

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

plugins/node/opentelemetry-instrumentation-net/src/net.ts Outdated Show resolved Hide resolved
@obecny
Copy link
Member

obecny commented Mar 18, 2021

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
Copy link
Member

obecny commented Mar 18, 2021

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

@obecny obecny added the enhancement New feature or request label Mar 18, 2021
@seemk
Copy link
Contributor Author

seemk commented Mar 19, 2021

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:
image

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.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

thx a lot for your contribution, this looks great!

@vmarchaud vmarchaud merged commit 557d9a4 into open-telemetry:main Mar 19, 2021
@johanneswuerbach
Copy link
Contributor

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NodeJS Net library
5 participants