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

[plugin-http] null-check socket before destructuring #1377

Closed
adelsz opened this issue Aug 3, 2020 · 5 comments · Fixed by #1939
Closed

[plugin-http] null-check socket before destructuring #1377

adelsz opened this issue Aug 3, 2020 · 5 comments · Fixed by #1939
Labels
bug Something isn't working up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@adelsz
Copy link

adelsz commented Aug 3, 2020

What version of OpenTelemetry are you using?

"@opentelemetry/api": "0.9.0",
"@opentelemetry/node": "0.9.0",
"@opentelemetry/plugin-http": "0.9.0",
"@opentelemetry/plugin-https": "0.9.0",
"@opentelemetry/tracing": "0.9.0",

What version of Node are you using?

Node 12

What did you do?

We use OpenTelemetry for tracing and for some requests we see destructuring errors inside plugin-http in our error logs.
This isn't consistently reproducible but it does happen sometimes. Error trace:

TypeError: Cannot destructure property 'localAddress' of 'socket' as it is null.
    at Object.exports.getIncomingRequestAttributesOnResponse (/app/node_modules/@opentelemetry/plugin-http/build/src/utils.js:363:13)
    at ServerResponse.response.end (/app/node_modules/@opentelemetry/plugin-http/build/src/http.js:215:50)
    at ServerResponse.send (/app/node_modules/express/lib/response.js:221:10)
    at ServerResponse.sendStatus (/app/node_modules/express/lib/response.js:359:15)
    at /app/dist/src/index.js:409:46

Looks like these occur because response.socket can be null:

const { localAddress, localPort, remoteAddress, remotePort } = socket;

Node docs do mention that:

After response.end(), the property (response.socket) is nulled

So maybe that is the source of the problem.

@adelsz adelsz added the bug Something isn't working label Aug 3, 2020
@dyladan
Copy link
Member

dyladan commented Aug 3, 2020

@OlivierAlbertini can you look into this? It looks like the typings say that socket should always be defined, which is why we didn't catch this before. My first instinct is to try to move utils.getIncomingRequestAttributesOnResponse before response.end is called. I tried that locally and the tests seem to pass, but I'm not sure if I'm missing some edge case there.

@dyladan dyladan self-assigned this Aug 5, 2020
@dyladan
Copy link
Member

dyladan commented Aug 5, 2020

I will look into this today

@vardani
Copy link

vardani commented Aug 26, 2020

@dyladan Hey, any updates on this?

@dyladan
Copy link
Member

dyladan commented Sep 2, 2020

@OlivierAlbertini Do you have any idea what might have happened here?

@kaareal
Copy link

kaareal commented Feb 9, 2021

also seen with node 14 with

     "@opentelemetry/api": "^0.14.0",
    "@opentelemetry/core": "^0.14.0",
    "@opentelemetry/exporter-zipkin": "^0.14.0",
    "@opentelemetry/koa-instrumentation": "^0.12.0",
    "@opentelemetry/node": "^0.14.0",
    "@opentelemetry/plugin-dns": "^0.12.0",
    "@opentelemetry/plugin-grpc-js": "^0.14.0",
    "@opentelemetry/plugin-http": "^0.14.0",
    "@opentelemetry/plugin-https": "^0.14.0",
    "@opentelemetry/plugin-mongodb": "^0.12.0",
    "@opentelemetry/tracing": "^0.14.0"

@dyladan dyladan changed the title Socket destructuing error in plugin-http [plugin-http] null-check socket before destructuring Feb 9, 2021
@dyladan dyladan added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Feb 9, 2021
@dyladan dyladan removed their assignment Feb 9, 2021
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
…-telemetry#1377)

* fix(instrumentation-dns): fix instrumentation of `dns/promises`

* chore(instrumentation-dns): fix ts error

* Revert "fix(instrumentation-dns): fix instrumentation of `dns/promises`"

This reverts commit a897248c5aac03573112cad093322cf5ab99ac17.

* fix(instrumentation-dns): fix instrumentation of `dns/promises`

* chore(instrumentation-dns): skip `dns/promises` test on node 14

* chore(instrumentation-dns): fix `dns/promises` test in ci

---------

Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants