fix(pino): skip instrumentation for pino thread-stream workers

[aryamohanan]

When collector is loaded via:

NODE_OPTIONS="--require /path/to/@instana/collector/src/immediate"

it executes before the main application code. If a Pino thread-stream worker is spawned, this worker also inherits the NODE_OPTIONS environment variable and attempts to initialize Instana.

The Pino worker is not a real application; it only exists to handle log messages. No need to instrument this process

Error

node:events:496
      throw er; // Unhandled 'error' event
      ^

Error: this should not happen: undefined
    at Worker.onWorkerMessage (/test-app/node_modules/thread-stream/index.js:188:23)
    at Worker.emit (node:events:518:28)
    at MessagePort.<anonymous> (node:internal/worker:268:53)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)
    at MessagePort.<anonymous> (node:internal/per_context/messageport:23:28)
Emitted 'error' event on ThreadStream instance at:
    at Immediate.<anonymous> (/test-app/node_modules/thread-stream/index.js:369:12)
    at process.processImmediate (node:internal/timers:485:21)

refs https://jsw.ibm.com/browse/INSTA-63554
https://github.com/pinojs/pino/blob/main/docs/transports.md

[kirrg001]

So yeah.

I think the decision in #517 fixed the metrics bug (wrong process name).

But I think we have to reconsider how we trace worker threads.

IMO Worker Threads should only be traced if its part of the customer application. And that should happen automatically.

We should not trace workers who are created from dependencies (such as pino).
This is a general problem - a lot of libraries nowadays create a worker to outsource stuff.

IMO adding an exception for pino does not solve the underlying problem and its hard to manage that long-term. Do you wanne add an exception for all libraries now who span a worker?

I'd suggest:

1. Hook into worker threads as early as possible

const workerThreads = require('worker_threads');
class InstanaWorkerOverrideClass extends workerThreads.Worker {
  // @ts-ignore
  constructor(filename, options) {
    if (filename.includes('node_modules') {
      if (!options) {
        options = {};
      }

      if (options.workerData !== undefined) {
        options.workerData.instanaTracing = false;
      } else {
        options.workerData = { instanaTracing: false };
      }
    }

    super(filename, options);
  }
}

workerThreads.Worker = InstanaWorkerOverrideClass;

2. Checking the worker thread tracing:

const skipWorkerThreadTracing = !workerThreads.isMainThread && workerThreads.workerData?.instanaTracing === false;

If any customer wants to trace worker treads in node_modules, then we can also add an env.

---

Before doing so, we should raise a PR to move the worker in packages/collector/test/metrics/appWithWorkerThread/test.js to the app root.

[kirrg001]

A quick fix for the customer could look like this:

INSTANA_PARENT_THREAD_COMMUNICATION=false

      if (!process.env.INSTANA_PARENT_THREAD_COMMUNICATION || process.env.INSTANA_PARENT_THREAD_COMMUNICATION !== 'true') {
        parentPort.postMessage('instana.collector.initialized');
      }

The error

Error: this should not happen: undefined

Only occurs because thread-stream does not follow the postMessage format.
This error is actually a bug in this library. They expect a specific format - otherwise they throw an error.

Value can be anything - also a string.

https://github.com/pinojs/thread-stream/blob/82e011281b8895bf5e8f70008e0242927fcaf0cb/index.js#L163
https://developer.mozilla.org/en-US/docs/Web/API/Worker/postMessage

Still, this fix is more like a workaround - but its quick and easy to ship. But its not solving the root cause.

[kirrg001]

Correction:

INSTANA_PARENT_PROCESS_COMMUNICATION=false

if (process.env.INSTANA_PARENT_PROCESS_COMMUNICATION !== 'false') {
  parentPort.postMessage('instana.collector.initialized');
}

or alternatively, disable worker thread tracing completely. This is maybe a more useful env anyway. (personally preferred)

if (!isMainThread && disableWorkerThreadTracing) {
   return
}

[aryamohanan]

#2131

[aryamohanan]

created a separate card for this https://jsw.ibm.com/browse/INSTA-63554

[kirrg001]

Lets talk about https://github.com/instana/nodejs/pull/2130/files#r2503439416

[sonarqubecloud]

Quality Gate failed

Failed conditions
71.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud