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

fix(http-intrumentation): prevent request socket null from throwing uncaught error #3858

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

aodysseos
Copy link
Contributor

@aodysseos aodysseos commented Jun 4, 2023

Which problem is this PR solving?

When a socket is undefined, the http-instrumentation package throws an error that is not caught. This is causing the application to restart.

Fixes #3560

Short description of the changes

This PR is modifying the utils.ts where the issue occurs. It checks if the socket is defined before accessing its contents and sets the relevant attributes only when the socket is defined.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Followed the style guidelines of this project

…ncaught error

Signed-off-by: Andreas Odysseos <aodysseos91@gmail.com>
@aodysseos aodysseos requested a review from a team June 4, 2023 22:06
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 4, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@pichlermarc
Copy link
Member

Hi, thanks for opening this PR. 🙂 Before we can continue with reviewing this PR, the CLA needs to be signed (see comment here).

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #3858 (d5a18a5) into main (b3d57bb) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head d5a18a5 differs from pull request most recent head 5259fd8. Consider uploading reports for the commit 5259fd8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3858      +/-   ##
==========================================
- Coverage   92.98%   92.98%   -0.01%     
==========================================
  Files         297      297              
  Lines        8839     8849      +10     
  Branches     1815     1817       +2     
==========================================
+ Hits         8219     8228       +9     
- Misses        620      621       +1     
Impacted Files Coverage Δ
...es/opentelemetry-instrumentation-http/src/utils.ts 99.21% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

@dyladan
Copy link
Member

dyladan commented Jun 21, 2023

@aodysseos can you please sign the CLA? If not, I can reopen the PR under my own account with the CLA signed.

@aodysseos
Copy link
Contributor Author

@aodysseos can you please sign the CLA? If not, I can reopen the PR under my own account with the CLA signed.

Hey @dyladan I haven't been able to pick this up and probably won't be till next Monday. Feel free to reopen the PR.

@smnbbrv
Copy link

smnbbrv commented Jun 28, 2023

I've hit this issue today. It happens for me if the server is behind a @fastify/http-proxy . Can I help somehow with resolving this?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks great but the PR still needs an entry in experimental/CHANGELOG.md and some lint fixes 🙂

@pichlermarc
Copy link
Member

FYI took the liberty to add the entry in experimental/CHANGELOG.md and run lint:fix to make the checks pass and save ourselves a cycle. (88e380c, 5259fd8). 🙂

Thank you for your contribution! 🙂

@pichlermarc pichlermarc merged commit bb8a4f7 into open-telemetry:main Jun 29, 2023
@smnbbrv
Copy link

smnbbrv commented Jun 29, 2023

Thank you @pichlermarc !

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

Successfully merging this pull request may close these issues.

Application crash if request.socket is null
4 participants