refactor: removed "getLogger" usage #1556

kirrg001 · 2025-02-11T23:23:27Z

refs https://jsw.ibm.com/browse/INSTA-13498

Reasons why we have to move away from getLogger('tracing/xxx'):

Overhead / Performance. We create and store (!) more then 70 logger child instances. See registry logic in logger.js.
We do not follow dependency injection, which makes it very hard to maintain the codebase. We always request a new instance on top of each file using getLogger('tracing/blubb') - without knowing what the actual logging instance is. We should only access config.logger in the init function of each module. This gives us control over the codeflow.
Logs disappear in the beginning of the application if you call getLogger too early e.g. logs from normalizeConfig did not appear from serverless correctly.
I have never used this feature. If we receive logs, I just copy the error message and search for it in the codebase.

I have not found a good reason to keep the child logger logic. We now create only one child logger: the instana internal logger if you provide a custom logger. Otherwise we only have one logger instance.

If we want to re-add log prefixes, we can easily do that by adding e.g. logger = config.logger.prefix('xxx'). This helper fn will wrap the log calls and automatically adds a prefix such as tracing/kafka: error msg.

measure bootstrap time (AWS + Locally)
fallback for let logger to prevent bugs
extract some changes in smaller PR's if possible

packages/azure-container-services/src/activate.js

kirrg001 · 2025-02-17T10:20:58Z

packages/collector/src/agentConnection.js

-  } else {
-    logger.debug(`Sending data to ${path}, ${agentOpts}`);
-  }
+  logger.debug(`Sending data to ${path}, ${agentOpts}`);


Note to me: revert

packages/collector/src/announceCycle/agentready.js

kirrg001 · 2025-02-17T11:23:52Z

packages/collector/src/agentConnection.js

+  cmdline.init(config);
+  pidStore = _pidStore;
+
+  cpuSetFileContent = getCpuSetFileContent();


NOTE: Moved from root initialization to init fn. We should never initialize components on root.

kirrg001 · 2025-02-17T11:26:21Z

packages/collector/src/index.js

-  // NOTE: By default we set our instana internal logger
-  config.logger = logger;
-
+  pidStore.init(config);


NOTE: We define & init the pidstore in the parent module (collector) and pass it on to all modules, who use the pidstore. Dependency injection.

kirrg001 · 2025-02-17T11:43:05Z

packages/collector/src/logger.js

-const registry = {};
+let instanaLogger;
+
+class InstanaLogger {


NOTE: The class instance makes it possible to update the logger without having to reinitialize all components.

It makes the old wrong logging design obsolete.

e.g.

if (isReInit) { Object.keys(registry).forEach(loggerName => { const reInitFn = registry[loggerName]; reInitFn(exports.getLogger(loggerName)); }); // cascade re-init to @instana/core logger.init(config); }

e.g.

exports.setLogger = function setLogger(_logger) { logger = _logger; config.logger = logger; instanaCore.logger.init(config); };

e.g.

exports.setLogger = function setLogger(logger) { metricsModules.forEach(metricModule => { if (typeof metricModule.setLogger === 'function') { metricModule.setLogger(logger); } }); };

Look at these complicated code constructs.

kirrg001 · 2025-02-17T14:52:34Z

packages/collector/src/logger.js

@@ -45,7 +104,8 @@ exports.init = function init(config, isReInit) {
    // we create later on.
    parentLogger = uninstrumentedLogger({
      name: '@instana/collector',
-      level: 'info'
+      level: 'info',
+      base: { threadId }


TODO: this removes pid & hostname

kirrg001 · 2025-02-17T15:30:28Z

packages/collector/src/metrics/index.js


-coreMetrics.registerAdditionalMetrics(sharedMetrics.allMetrics);


NOTE: Same here. No initialization on the root file.

kirrg001 · 2025-02-17T15:35:04Z

packages/collector/src/tracing/instrumentation/process/selfPath.js

 // @ts-ignore - Cannot redeclare exported variable
 exports.immediate = path.join(__dirname, '..', '..', '..', 'immediate.js');
-
-if (!fs.existsSync(exports.immediate)) {


We print the warning on the caller component.

https://github.com/instana/nodejs/blob/v4.6.1/packages/collector/src/tracing/instrumentation/process/edgemicro.js#L36

https://github.com/instana/nodejs/blob/v4.6.1/packages/collector/src/tracing/instrumentation/process/childProcess.js#L49

It was anyway wrong that we log on root file.

kirrg001 · 2025-02-17T15:39:46Z

packages/collector/test/immediate/app.js

@@ -7,7 +7,7 @@

 // NOTE: c8 bug https://github.com/bcoe/c8/issues/166
 process.on('SIGTERM', () => {
-  process.disconnect();
+  process.disconnect && process.disconnect();


kirrg001 · 2025-02-17T15:51:03Z

packages/core/src/metrics/index.js

@@ -47,6 +46,7 @@ exports.findAndRequire = function findAndRequire(baseDir) {
      .filter(
        moduleName =>
          moduleName.indexOf('.js') === moduleName.length - 3 &&
+          // TODO: move the extra metrics into a separate folder


Just noticed while coding. Bad code quality. Ignore this change if you want.

kirrg001 · 2025-02-17T16:44:12Z

packages/serverless/src/logger.js

@@ -0,0 +1,163 @@
+/*


Putting it here. Same concept as our collector logger. Just with a little bit less logic.

kirrg001 · 2025-02-17T16:47:14Z

packages/serverless/src/backend_connector.js

@@ -36,32 +36,36 @@ let warningsHaveBeenLogged = false;
 const disableCaCheckEnvVar = 'INSTANA_DISABLE_CA_CHECK';
 const disableCaCheck = process.env[disableCaCheckEnvVar] === 'true';

-if (process.env[proxyEnvVar] && !environmentUtil.sendUnencrypted) {


Just moved into init fn.

kirrg001 · 2025-02-17T16:53:03Z

packages/core/src/util/index.js

+ * @return {(...args: *) => *} A wrapped function which will forward the first call to `cb`
+ *   and log any successive calls.
+ */
+exports.atMostOnce = function atMostOnce(name, cb) {


Move the content of the file "atMostOnce" into this file for now, because I could not find a nice way to add an init fn to this little helper.

Extracted from #1556

refs https://jsw.ibm.com/browse/INSTA-13498 Extracted from #1556 With this refactoring, we can easily forward the config object now to the aws v3 pkg. Each class is instantiated with the config object, which contains the logger.

Extracted from #1556

kirrg001 · 2025-02-19T11:34:40Z

packages/collector/src/agentConnection.js

@@ -6,24 +6,17 @@
 'use strict';

 const { util, uninstrumentedHttp, uninstrumentedFs: fs } = require('@instana/core');
-const http = uninstrumentedHttp.http;


Sorted requires and variables. Just moved down

kirrg001 · 2025-02-19T11:36:28Z

packages/collector/src/agentConnection.js

-
-const circularReferenceRemover = require('./util/removeCircular');
-const agentOpts = require('./agent/opts');
-const pidStore = require('./pidStore');


pidStore is injected. See later comments.

kirrg001 · 2025-02-19T11:37:33Z

packages/collector/src/announceCycle/agentready.js

 const agentConnection = require('../agentConnection');
 const agentOpts = require('../agent/opts');
 const initializedTooLate = require('../util/initializedTooLate');
 const metrics = require('../metrics');
-const pidStore = require('../pidStore');


Same here. Dependency injection. pidStore is defined in the upper unit.

kirrg001 · 2025-02-19T11:39:22Z

packages/collector/src/index.js

+    log.init(_config);
+  }
+
+  // TODO: The idea of having a config per parent module probably makes, sense


Not solved as part of this PR.

kirrg001 force-pushed the refactor-remove-getlogger branch 2 times, most recently from 61df358 to 1a95836 Compare February 12, 2025 13:08

kirrg001 commented Feb 17, 2025

View reviewed changes

packages/azure-container-services/src/activate.js Outdated Show resolved Hide resolved

kirrg001 commented Feb 17, 2025

View reviewed changes

packages/collector/src/announceCycle/agentready.js Outdated Show resolved Hide resolved

kirrg001 commented Feb 17, 2025

View reviewed changes

kirrg001 force-pushed the refactor-remove-getlogger branch from e7eae88 to 7fceedb Compare February 17, 2025 14:54

kirrg001 mentioned this pull request Feb 17, 2025

fix: ensure the logger always applies the correct log level #1565

Merged

kirrg001 commented Feb 17, 2025

View reviewed changes

This was referenced Feb 17, 2025

refactor: optimized aws v3 initialization #1571

Merged

refactor: core utils usage #1573

Merged

kirrg001 added a commit that referenced this pull request Feb 18, 2025

refactor: optimized core utils usage (#1573)

cb387d6

Extracted from #1556

kirrg001 force-pushed the refactor-remove-getlogger branch 3 times, most recently from 1848a47 to 32268f6 Compare February 18, 2025 07:57

kirrg001 force-pushed the refactor-remove-getlogger branch from 32268f6 to 3731ce9 Compare February 18, 2025 10:44

kirrg001 added a commit that referenced this pull request Feb 19, 2025

fix: ensure the logger always applies the correct log level (#1565)

90af990

Extracted from #1556

kirrg001 force-pushed the refactor-remove-getlogger branch 2 times, most recently from 96425d4 to cbc832d Compare February 19, 2025 10:14

refactor: rewrite logger usage

66cbb55

kirrg001 added 20 commits February 19, 2025 12:09

chore: fixes

d67f773

chore: fixes

1f90119

chore: fixes

03864c8

chore: lint

04c03e4

chore: fixes

da6bbbc

chore: fixes

0deaaf2

chore: fix

7925b38

chore: fix

33ad7f7

chore: clarify comment

49108be

chore: reverts

9421b39

chore: cleanup

8d3b6f5

chore: naming convention

f01838f

chore: comments

864d6c9

chore: comments + fixes

8df1926

chore: lint

f1f2c7c

chore: comment

b17dc5e

chore: corrected comment

f17f208

chore: fix otel

81afe00

chore: cleanup

e93b487

chore: readability

d042d81

kirrg001 force-pushed the refactor-remove-getlogger branch from cbc832d to d042d81 Compare February 19, 2025 11:10

kirrg001 commented Feb 19, 2025

View reviewed changes

kirrg001 added 4 commits February 19, 2025 12:43

chore: cleanup

34cdde4

chore: fixed test

7b026fa

chore: fixes

bb55bed

chore: ts

8376e97

kirrg001 mentioned this pull request Feb 19, 2025

refactor(metrics-util): improved readability #1581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: removed "getLogger" usage #1556

refactor: removed "getLogger" usage #1556

kirrg001 commented Feb 11, 2025 •

edited

Loading

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025 •

edited

Loading

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025 •

edited

Loading

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025 •

edited

Loading

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025

kirrg001 Feb 17, 2025

kirrg001 Feb 19, 2025

kirrg001 Feb 19, 2025

kirrg001 Feb 19, 2025

kirrg001 Feb 19, 2025


		coreMetrics.registerAdditionalMetrics(sharedMetrics.allMetrics);

refactor: removed "getLogger" usage #1556

Are you sure you want to change the base?

refactor: removed "getLogger" usage #1556

Conversation

kirrg001 commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirrg001 commented Feb 11, 2025 •

edited

Loading

kirrg001 Feb 17, 2025 •

edited

Loading

kirrg001 Feb 17, 2025 •

edited

Loading

kirrg001 Feb 17, 2025 •

edited

Loading