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(auto-instrumentations-node): shutdown the SDK when the process exits normally #2394

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Aug 21, 2024

Which problem is this PR solving?

Fixes #2383

Short description of the changes

Adds a one-shot beforeExit hook to call NodeSDK.shutdown(). In the event of SIGTERM and the process gracefully exiting, shutdown may be called twice. I believe the additional calls to shutdown() should no-op so that is not a concern but may make for spammy logs.

I updated the tests to capture the expected behavior

  • Shutdown on SIGTERM
  • Shutdown when exiting normally

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.64%. Comparing base (dfb2dff) to head (3c06297).
Report is 222 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2394      +/-   ##
==========================================
- Coverage   90.97%   90.64%   -0.34%     
==========================================
  Files         146      156      +10     
  Lines        7492     7612     +120     
  Branches     1502     1571      +69     
==========================================
+ Hits         6816     6900      +84     
- Misses        676      712      +36     

see 76 files with indirect coverage changes

@aabmass aabmass marked this pull request as ready for review August 21, 2024 15:20
@aabmass aabmass requested a review from a team August 21, 2024 15:20
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks!

@trentm
Copy link
Contributor

trentm commented Aug 21, 2024

I'll leave some time for others to review as well.

@pichlermarc pichlermarc merged commit 0f90b3d into open-telemetry:main Aug 27, 2024
19 checks passed
@dyladan dyladan mentioned this pull request Aug 27, 2024
@aabmass aabmass deleted the auto-beforeExit branch August 28, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zero code instrumentation doesn't flush telemetry on normal exit
6 participants