Skip to content

refactor: Refactor child_process instrumentation to use tracing channel instead of Shim - #4212

Open
amychisholm03 wants to merge 6 commits into
newrelic:mainfrom
amychisholm03:core-subscribers
Open

refactor: Refactor child_process instrumentation to use tracing channel instead of Shim#4212
amychisholm03 wants to merge 6 commits into
newrelic:mainfrom
amychisholm03:core-subscribers

Conversation

@amychisholm03

@amychisholm03 amychisholm03 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Description

This PR converts the child_process exec/execFile instrumentation from Shim-based monkeypatching to utilize Node's native tracing channel API. It creates one tracingChannel per method and monkey patches each exec/execFile function once per process. It keeps the nested Callback: <name> segment if a callback was provided.

How to Test

npm run unit
npm run integration

Related Issues

Closes #4223

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.40260% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.44%. Comparing base (6bf954b) to head (c853544).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
lib/subscribers/tc-base.js 96.99% 4 Missing ⚠️
lib/subscribers/child_process/exec.js 97.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4212      +/-   ##
==========================================
- Coverage   97.52%   97.44%   -0.08%     
==========================================
  Files         522      525       +3     
  Lines       62959    63139     +180     
  Branches        1        1              
==========================================
+ Hits        61400    61526     +126     
- Misses       1559     1613      +54     
Flag Coverage Δ
integration-tests-cjs-22.x 73.69% <97.40%> (+0.08%) ⬆️
integration-tests-cjs-24.x 74.28% <97.40%> (+0.08%) ⬆️
integration-tests-cjs-26.x 74.28% <97.40%> (+0.08%) ⬆️
integration-tests-esm-22.x 54.66% <82.25%> (+0.09%) ⬆️
integration-tests-esm-24.x 55.95% <82.25%> (+0.08%) ⬆️
integration-tests-esm-26.x 55.95% <82.25%> (+0.08%) ⬆️
unit-tests-22.x 89.09% <83.11%> (-0.03%) ⬇️
unit-tests-24.x 89.04% <83.11%> (-0.03%) ⬇️
unit-tests-26.x 89.04% <83.11%> (-0.03%) ⬇️
versioned-tests-22.x 97.44% <97.40%> (-0.08%) ⬇️
versioned-tests-24.x 97.44% <97.40%> (-0.08%) ⬇️
versioned-tests-26.x 97.44% <97.40%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amychisholm03
amychisholm03 marked this pull request as ready for review August 10, 2026 19:30
jsumners-nr
jsumners-nr previously approved these changes Aug 10, 2026

@jsumners-nr jsumners-nr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, but I think we can improve a few things.

Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated

@bizob2828 bizob2828 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't studied this but we should keep the instrumentation separate from the subscribers of instrumentation. I know they are in separate classes but the subscribers should live in lib/subscribers. I know this is the first but we should be creating abstractions like the orchestrion and native diagnostics channel based instrumentation so it can be reused for future core node library subscribers

@bizob2828 bizob2828 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a few more comments. this is a good start but we probably want to study the abstracted design more as we add more core node.js subscribers and instrumentation that runs in tracing channels

Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Comment thread lib/instrumentation/core/child_process.js Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs PR Review

Development

Successfully merging this pull request may close these issues.

Migrate child_process to emit events over tracing channel

4 participants