-
Notifications
You must be signed in to change notification settings - Fork 422
refactor: Refactor child_process instrumentation to use tracing channel instead of Shim
#4212
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
Open
amychisholm03
wants to merge
6
commits into
newrelic:main
Choose a base branch
from
amychisholm03:core-subscribers
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5311183
Refactor `child_process` instrumentation to use tracing channel inste…
amychisholm03 2ef6c25
Add unit test if no `child_process`
amychisholm03 5b832b5
Address PR feedback: Refactor to `ChildProcessInstrumentation`; add d…
amychisholm03 ab569ba
Add proper teardown
amychisholm03 1330ce0
Refactor to use a new base subscriber, `tc-base`
amychisholm03 c853544
Add more tests
amychisholm03 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,37 +1,120 @@ | ||
| /* | ||
| * Copyright 2020 New Relic Corporation. All rights reserved. | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const { RecorderSpec } = require('../../../lib/shim/specs') | ||
| // eslint-disable-next-line n/no-unsupported-features/node-builtins | ||
| const { tracingChannel } = require('node:diagnostics_channel') | ||
| const defaultLogger = require('#agentlib/logger.js').child({ component: 'child_process' }) | ||
|
|
||
| module.exports = initialize | ||
| const channels = { | ||
| exec: tracingChannel('child_process.exec'), | ||
| execFile: tracingChannel('child_process.execFile') | ||
| } | ||
|
|
||
| function initialize(agent, childProcess, moduleName, shim) { | ||
| if (!childProcess) { | ||
| shim.logger.debug('Could not find child_process, not instrumenting') | ||
| return false | ||
| class ChildProcessInstrumentation { | ||
| constructor(agent) { | ||
| this.tracer = agent.tracer | ||
| this.store = this.tracer._contextManager._asyncLocalStorage | ||
| this.originals = {} | ||
| } | ||
|
|
||
| const methods = ['exec', 'execFile'] | ||
| patch(childProcess) { | ||
| this.childProcess = childProcess | ||
|
|
||
| shim.record(childProcess, methods, function recordExec(shim, fn, name) { | ||
| return new RecorderSpec({ name: 'child_process.' + name, callback: shim.LAST }) | ||
| }) | ||
| for (const [methodName, channel] of Object.entries(channels)) { | ||
| const original = childProcess[methodName] | ||
| this.originals[methodName] = original | ||
| const tracer = this.tracer | ||
|
|
||
| makePromisifyCompatible(shim, childProcess) | ||
| } | ||
| childProcess[methodName] = function wrappedMethod(...args) { | ||
| const ctx = tracer.getContext() | ||
| if (!ctx?.transaction?.isActive()) { | ||
| return original.apply(this, args) | ||
| } | ||
|
|
||
| const lastArg = args[args.length - 1] | ||
| const hasCallback = typeof lastArg === 'function' | ||
| const data = { methodName, callbackName: hasCallback ? lastArg.name || '<anonymous>' : null } | ||
|
|
||
| function makePromisifyCompatible(shim, childProcess) { | ||
| const originalExec = shim.getOriginal(childProcess.exec) | ||
| for (const symbol of Object.getOwnPropertySymbols(originalExec)) { | ||
| childProcess.exec[symbol] = originalExec[symbol] | ||
| return hasCallback | ||
|
amychisholm03 marked this conversation as resolved.
Outdated
|
||
| ? channel.traceCallback(original, -1, data, this, ...args) | ||
| : channel.traceSync(original, data, this, ...args) | ||
| } | ||
|
|
||
| for (const symbol of Object.getOwnPropertySymbols(original)) { | ||
| childProcess[methodName][symbol] = original[symbol] | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TODO: this can be extracted to be used by other core instrumentation | ||
| // as we refactor them to use tracing channel | ||
| createSegment(ctx, name) { | ||
| const segment = this.tracer.createSegment({ name, parent: ctx?.segment, transaction: ctx?.transaction }) | ||
| if (segment) { | ||
| segment.start() | ||
| } | ||
| return segment | ||
| } | ||
|
|
||
| const originalExecFile = shim.getOriginal(childProcess.execFile) | ||
| for (const symbol of Object.getOwnPropertySymbols(originalExecFile)) { | ||
| childProcess.execFile[symbol] = originalExecFile[symbol] | ||
| bindStore() { | ||
| const { tracer, store } = this | ||
|
|
||
| for (const channel of Object.values(channels)) { | ||
| channel.start.bindStore(store, (data) => { | ||
| const ctx = tracer.getContext() | ||
| const segment = this.createSegment(ctx, 'child_process.' + data.methodName) | ||
| data.ctx = segment ? ctx.enterSegment({ segment }) : ctx | ||
| return data.ctx | ||
| }) | ||
|
|
||
| channel.asyncStart.bindStore(store, (data) => { | ||
| const { ctx, callbackName } = data | ||
| ctx.segment.touch() | ||
|
|
||
| const segment = this.createSegment(ctx, 'Callback: ' + callbackName) | ||
| data.callbackSegment = segment | ||
| return ctx.enterSegment({ segment }) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| teardown() { | ||
| for (const [methodName, channel] of Object.entries(channels)) { | ||
| if (this.originals[methodName]) { | ||
| this.childProcess[methodName] = this.originals[methodName] | ||
| } | ||
| channel.start.unbindStore(this.store) | ||
|
amychisholm03 marked this conversation as resolved.
Outdated
|
||
| channel.asyncStart.unbindStore(this.store) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| module.exports = function initialize(agent, childProcess, { logger = defaultLogger } = {}) { | ||
| if (!childProcess) { | ||
| logger.debug('Could not find child_process, not instrumenting') | ||
| return false | ||
| } | ||
|
|
||
| const instrumentation = new ChildProcessInstrumentation(agent) | ||
|
amychisholm03 marked this conversation as resolved.
Outdated
|
||
| instrumentation.patch(childProcess) | ||
| instrumentation.bindStore() | ||
| return instrumentation | ||
| } | ||
|
|
||
| // `end`/`asyncEnd` are agent-agnostic (operate only on what's already | ||
| // stashed on `data` by the bindStore transforms above), so unlike | ||
| // `start`/`asyncStart` they only ever need to be subscribed once. | ||
| for (const channel of Object.values(channels)) { | ||
| channel.subscribe({ | ||
| end(data) { | ||
| data.ctx?.segment?.touch() | ||
|
amychisholm03 marked this conversation as resolved.
Outdated
|
||
| }, | ||
| asyncEnd(data) { | ||
| data.callbackSegment?.touch() | ||
| } | ||
| }) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const test = require('node:test') | ||
| const assert = require('node:assert') | ||
| const helper = require('#testlib/agent_helper.js') | ||
| const childProcessInstrumentation = require('#agentlib/instrumentation/core/child_process.js') | ||
|
|
||
| test('child_process instrumentation', async (t) => { | ||
| const agent = helper.loadMockedAgent() | ||
| t.after(() => { | ||
| helper.unloadAgent(agent) | ||
| }) | ||
|
|
||
| await t.test('should log and return false when child_process is not available', () => { | ||
| const debugCalls = [] | ||
| const stubLogger = { debug: (msg) => debugCalls.push(msg) } | ||
|
|
||
| const result = childProcessInstrumentation(agent, null, { logger: stubLogger }) | ||
|
|
||
| assert.equal(result, false) | ||
| assert.deepEqual(debugCalls, ['Could not find child_process, not instrumenting']) | ||
| }) | ||
|
|
||
| await t.test('teardown should restore the original exec/execFile', () => { | ||
| const originalExec = function exec() {} | ||
| const originalExecFile = function execFile() {} | ||
| const fakeChildProcess = { exec: originalExec, execFile: originalExecFile } | ||
|
|
||
| const instrumentation = childProcessInstrumentation(agent, fakeChildProcess) | ||
| assert.notEqual(fakeChildProcess.exec, originalExec, 'exec should be wrapped after patch') | ||
| assert.notEqual(fakeChildProcess.execFile, originalExecFile, 'execFile should be wrapped after patch') | ||
|
|
||
| instrumentation.teardown() | ||
|
|
||
| assert.equal(fakeChildProcess.exec, originalExec, 'exec should be restored after teardown') | ||
| assert.equal(fakeChildProcess.execFile, originalExecFile, 'execFile should be restored after teardown') | ||
| }) | ||
|
|
||
| await t.test('should not double-wrap after a teardown/re-patch cycle', () => { | ||
| const originalExec = function exec() {} | ||
| const fakeChildProcess = { exec: originalExec, execFile: function execFile() {} } | ||
|
|
||
| const first = childProcessInstrumentation(agent, fakeChildProcess) | ||
| first.teardown() | ||
|
|
||
| const second = childProcessInstrumentation(agent, fakeChildProcess) | ||
|
|
||
| assert.equal(second.originals.exec, originalExec, 'second patch should have wrapped the true original, not a stale wrapper') | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.