refactor: Added ability to prevent creation of segments in subscriber based instrumentation when parent is marked as internal and of the same package (newrelic#3322)

Author: bizob2828

lib/subscribers/base.js

@@ -8,10 +8,40 @@
 const { tracingChannel } = require('node:diagnostics_channel')
 
 /**
- * Base class for defining a subscriber.
- * events property is an array with the following possible event names:
- *  `start`, `end`, `asyncStart`, `asyncEnd`, `error`
- *  @link https://nodejs.org/api/diagnostics_channel.html#class-tracingchannel
+ * The baseline parameters available to all subscribers.
+ *
+ * @typedef {object} SubscriberParams
+ * @property {object} agent A New Relic Node.js agent instance.
+ * @property {object} logger An agent logger instance.
+ * @property {string} packageName The npm installable name for the package
+ * being instrumented. This is what a developer would provide to the `require`
+ * function.
+ * @property {string} channelName A unique name for the diagnostics channel
+ * that will be created and monitored.
+ */
+
+/**
+ * @property {object} agent A New Relic Node.js agent instance.
+ * @property {object} logger An agent logger instance.
+ * @property {object} config The agent configuration object.
+ * @property {string} packageName The name of the module being instrumented.
+ * This is the same string one would pass to the `require` function.
+ * @property {string} channelName A unique name for the diagnostics channel
+ * that will be registered.
+ * @property {string[]} [events=[]] Set of tracing channel event names to
+ * register handlers for. For any name in the set, a corresponding method
+ * must exist on the subscriber instance. The method will be passed the
+ * event object. Possible event names are `start`, `end`, `asyncStart`,
+ * `asyncEnd`, and `error`. @link https://nodejs.org/api/diagnostics_channel.html#class-tracingchannel
+ * @property {boolean} [opaque=false] If true, any children segments will not be created
+ * @property {boolean} [internal=false] If true, any children segments from the same library will not be created
+ * @property {string} [prefix='orchestrion:'] String to prepend to diagnostics
+ * channel event names. This provides a namespace for the events we are
+ * injecting into a module.
+ * @property {boolean} [requireActiveTx=true] If true, the subscriber will only handle events when there is an active transaction.
+ * @property {string} id A unique identifier for the subscriber, combining the prefix, package name, and channel name.
+ * @property {TracingChannel} channel The tracing channel instance this subscriber will be monitoring.
+ * @property {AsyncLocalStorage} store The async local storage instance used for context management.
  */
 class Subscriber {
   constructor({ agent, logger, packageName, channelName }) {
@@ -22,13 +52,20 @@ class Subscriber {
     this.channelName = channelName
     this.events = []
     this.opaque = false
+    this.internal = false
     this.prefix = 'orchestrion:'
     this.requireActiveTx = true
     this.id = `${this.prefix}${this.packageName}:${this.channelName}`
     this.channel = tracingChannel(this.id)
     this.store = agent.tracer._contextManager._asyncLocalStorage
   }
 
+  shouldCreateSegment(parent) {
+    return !(parent?.opaque ||
+    (this.internal && this.packageName === parent?.shimId)
+    )
+  }
+
   /**
    * Creates a segment with a name, parent, transaction and optional recorder.
    * If the segment is successfully created, it will be started and added to the context.
@@ -39,15 +76,23 @@ class Subscriber {
    * @returns {Context} - The updated context with the new segment or existing context if segment creation fails
    */
   createSegment({ name, recorder, ctx }) {
+    const parent = ctx?.segment
+
+    if (this.shouldCreateSegment(parent) === false) {
+      this.logger.trace('Skipping segment creation for %s, %s(parent) is of the same package: %s and incoming segment is marked as internal', name, parent?.name, this.packageName)
+      return ctx
+    }
+
     const segment = this.agent.tracer.createSegment({
       name,
-      parent: ctx?.segment,
+      parent,
       recorder,
       transaction: ctx?.transaction,
     })
 
     if (segment) {
       segment.opaque = this.opaque
+      segment.shimId = this.packageName
       segment.start()
       this.logger.trace('Created segment %s', name)
       this.addAttributes(segment)

test/unit/lib/subscribers/base.test.js

@@ -95,6 +95,37 @@ test('should not create segment if no active tx', (t) => {
   assert.ok(!newCtx.segment)
 })
 
+test('should not create segment if parent is opaque', async (t) => {
+  const { agent, subscriber } = t.nr
+  await helper.runInTransaction(agent, async () => {
+    const ctx = agent.tracer.getContext()
+    ctx.segment.opaque = true
+    const newCtx = subscriber.createSegment({
+      name: 'test-segment',
+      ctx,
+    })
+
+    assert.deepEqual(newCtx, ctx)
+    assert.equal(newCtx.segment.name, ctx.segment.name)
+  })
+})
+
+test('should not create segment if parent is of same package and subscriber is internal', async (t) => {
+  const { agent, subscriber } = t.nr
+  await helper.runInTransaction(agent, async () => {
+    const ctx = agent.tracer.getContext()
+    ctx.segment.shimId = 'test-package'
+    subscriber.internal = true
+    const newCtx = subscriber.createSegment({
+      name: 'test-segment',
+      ctx,
+    })
+
+    assert.deepEqual(newCtx, ctx)
+    assert.equal(newCtx.segment.name, ctx.segment.name)
+  })
+})
+
 test('should touch segment when asyncEnd is called', (t, end) => {
   const { agent, subscriber } = t.nr
   helper.runInTransaction(agent, () => {