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): make SIGTERM shutdown test more reliable #2667

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

garysassano
Copy link
Contributor

Which problem is this PR solving?

The test shuts down the NodeSDK when SIGTERM is received is flaky due to making HTTP requests to example.com:

image

Short description of the changes

The key improvements are:

  1. Controlled Environment: Instead of depending on example.com (which could be slow/unreachable), we use a local server that responds immediately.

  2. Better Event Handling: Changed from using req.on('close') to res.on('end') which is more reliable for confirming the request completed.

  3. Proper Cleanup: Added server.close() to the SIGTERM handler to ensure clean shutdown.

  4. Fixed Timing: The local server responds immediately, reducing the chance of hitting the test's 1500ms timeout.

The main issue was likely that requests to example.com were taking too long or failing, causing the test to hit its timeout before seeing the Finished request message. By using a local server, we make the test much more deterministic and faster.

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.84%. Comparing base (3e95995) to head (ca846b1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2667   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files         172      172           
  Lines        8103     8103           
  Branches     1646     1646           
=======================================
  Hits         7361     7361           
  Misses        742      742           

@pichlermarc pichlermarc linked an issue Jan 20, 2025 that may be closed by this pull request
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.

thanks, much appreciated 🙂

@pichlermarc pichlermarc merged commit 94414a1 into open-telemetry:main Jan 20, 2025
10 checks passed
@dyladan dyladan mentioned this pull request Jan 20, 2025
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.

[auto-instrumentations-node] flaky NodeSDK shutdown test
5 participants