chore: adapt pr comments

instana · Dec 24, 2024 · 5e80c7b · 5e80c7b
1 parent e7fe9bb
commit 5e80c7b
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 65 deletions.
diff --git a/packages/collector/src/logger.js b/packages/collector/src/logger.js
@@ -33,26 +33,20 @@ const registry = {};
  * @param {boolean} [isReInit]
  */
 exports.init = function init(config, isReInit) {
-  const consoleStream = pino.destination(1);
-
   if (config.logger && typeof config.logger.child === 'function') {
-    // A pino logger has been provided via config; create a child logger directly under it.
+    // A bunyan or pino logger has been provided via config. In either case we create a child logger directly under the
+    // given logger which serves as the parent for all loggers we create later on.
     parentLogger = config.logger.child({
       module: 'instana-nodejs-logger-parent'
     });
   } else if (config.logger && hasLoggingFunctions(config.logger)) {
-    // A custom logger has been provided; use it as is.
+    // A custom non-bunyan/non-pino logger has been provided via config. We use it as is.
     parentLogger = config.logger;
   } else {
-    // No custom logger has been provided; create a new pino logger as the parent logger for all loggers
-    // @ts-ignore
-    parentLogger = pino({
-      name: '@instana/collector',
-      thread: threadId
-    });
-  }
+    // This consoleStream creates a destination stream for the Pino logger that writes log data to the standard output.
+    // Since we are using multistream here, this needs to be specified explicitly
+    const consoleStream = pino.destination(process.stdout);
 
-  if (isPino(parentLogger)) {
     const multiStream = {
       /**
        * Custom write method to send logs to multiple destinations
@@ -66,12 +60,13 @@ exports.init = function init(config, isReInit) {
       }
     };
 
-    // @ts-ignore
+    // No custom logger has been provided via config, we create a new pino logger as the parent logger for all loggers
+    // we create later on.
     parentLogger = pino(
       {
-        ...parentLogger.levels,
-        level: parentLogger.level || 'info',
-        base: parentLogger.bindings()
+        name: '@instana/collector',
+        thread: threadId,
+        level: 'info'
       },
       multiStream
     );
@@ -126,20 +121,6 @@ exports.getLogger = function getLogger(loggerName, reInitFn) {
   return /** @type {import('@instana/core/src/core').GenericLogger} */ (_logger);
 };
 
-/**
- * @param {*} _logger
- * @returns {boolean}
- */
-function isPino(_logger) {
-  return (
-    _logger &&
-    typeof _logger === 'object' &&
-    typeof _logger.child === 'function' &&
-    typeof _logger.level === 'string' &&
-    typeof _logger[pino.symbols.streamSym] !== 'undefined'
-  );
-}
-
 /**
  * @param {import('@instana/core/src/core').GenericLogger | *} _logger
  * @returns {boolean}

diff --git a/packages/collector/test/tracing/logger/pino/test.js b/packages/collector/test/tracing/logger/pino/test.js
@@ -57,7 +57,7 @@ describe('tracing/logger/pino', function () {
       await controls.clearIpcMessages();
     });
 
-    it(`must not trace info${suffix}`, () => {
+    it(`must not trace info${suffix}`, () =>
       trigger('info', useExpressPino, controls).then(() =>
         testUtils.retry(() =>
           agentControls.getSpans().then(spans => {
@@ -71,8 +71,7 @@ describe('tracing/logger/pino', function () {
             expect(pinoSpans).to.be.empty;
           })
         )
-      );
-    });
+      ));
 
     it('[suppressed] should not trace', async function () {
       await controls.sendRequest({
@@ -91,51 +90,48 @@ describe('tracing/logger/pino', function () {
         });
     });
 
-    it(`must trace warn${suffix}`, () => {
-      runTest('warn', useExpressPino, false, 'Warn message - should be traced.', controls);
-    });
+    it(`must trace warn${suffix}`, () =>
+      runTest('warn', useExpressPino, false, 'Warn message - should be traced.', controls));
 
-    it(`must trace error${suffix}`, () => {
-      runTest('error', useExpressPino, true, 'Error message - should be traced.', controls);
-    });
+    it(`must trace error${suffix}`, () =>
+      runTest('error', useExpressPino, true, 'Error message - should be traced.', controls));
 
-    it(`must trace fatal${suffix}`, () => {
-      runTest('fatal', useExpressPino, true, 'Fatal message - should be traced.', controls);
-    });
+    it(`must trace fatal${suffix}`, () =>
+      runTest('fatal', useExpressPino, true, 'Fatal message - should be traced.', controls));
 
     // prettier-ignore
-    it(`must trace error object without message${suffix}`, () => {
-      runTest('error-object-only', useExpressPino, true, 'This is an error.', controls);
-  });
+    it(`must trace error object without message${suffix}`, () =>
+      runTest('error-object-only', useExpressPino, true, 'This is an error.', controls)
+  );
 
     // prettier-ignore
-    it(`should serialize random objects one level deep${suffix}`, () => {
+    it(`should serialize random objects one level deep${suffix}`, () =>
       runTest(
         'error-random-object-only',
         useExpressPino,
         true,
         ['{ payload: ', 'statusCode: 404', "error: 'Not Found'", 'very: [Object'],
         controls
-      );
-  });
+      )
+  );
 
     // prettier-ignore
-    it(`must trace error object and string${suffix}`, () => {
+    it(`must trace error object and string${suffix}`, () =>
       runTest(
         'error-object-and-string',
         useExpressPino,
         true,
         'This is an error. -- Error message - should be traced.',
         controls
-      );
-});
+      )
+);
 
     // prettier-ignore
-    it(`must trace random object and string${suffix}`, () => {
-      runTest('error-random-object-and-string', useExpressPino, true, 'Error message - should be traced.', controls);
-  });
+    it(`must trace random object and string${suffix}`, () =>
+      runTest('error-random-object-and-string', useExpressPino, true, 'Error message - should be traced.', controls)
+  );
 
-    it(`must not trace custom info${suffix}`, () => {
+    it(`must not trace custom info${suffix}`, () =>
       trigger('custom-info', useExpressPino, controls).then(() =>
         testUtils.retry(() =>
           agentControls.getSpans().then(spans => {
@@ -149,18 +145,16 @@ describe('tracing/logger/pino', function () {
             expect(pinoSpans).to.be.empty;
           })
         )
-      );
-    });
+      ));
 
-    it(`must trace custom error${suffix}`, () => {
-      runTest('custom-error', useExpressPino, true, 'Custom error level message - should be traced.', controls);
-    });
+    it(`must trace custom error${suffix}`, () =>
+      runTest('custom-error', useExpressPino, true, 'Custom error level message - should be traced.', controls));
 
     it(`must trace child logger error${suffix}`, () => {
-      // if (useExpressPino) {
-      // return
-      // }
-      // return runTest('child-error', false, true, 'Child logger error message - should be traced.', controls);
+      if (useExpressPino) {
+        return;
+      }
+      return runTest('child-error', false, true, 'Child logger error message - should be traced.', controls);
     });
   }
 

diff --git a/packages/core/src/uninstrumentedLogger.js b/packages/core/src/uninstrumentedLogger.js
@@ -4,7 +4,6 @@
 
 'use strict';
 
-// instana/no-unsafe-require
 // eslint-disable-next-line import/no-extraneous-dependencies, instana/no-unsafe-require
 const logger = require('pino');