fix: exposed installation method in debug mode #1268

abhilash-sivan · 2024-08-12T07:14:42Z

test in local with --experimental-loader flag: node --experimental-loader ./node_modules/@instana/collector/esm-loader.mjs ./index-esm.mjs
test in local with --import flag: node --import ./node_modules/@instana/collector/esm-register.mjs ./index-esm.mjs
test in local with --require flag: node --require ./node_modules/@instana/collector/src/immediate ./index.js
test in local with no flags: node ./index.js

abhilash-sivan · 2024-08-16T07:39:15Z

commit messge to main:

feat: exposed installation method in debug mode
refs INSTA-778

abhilash-sivan · 2024-08-16T07:45:58Z

For the normal commonJS instrumentation using require('@instana/collector')(); in the code level currently considered as default and the debug log will print like

The app isnstrumented instana using: no additional flags

aryamohanan · 2024-08-16T08:10:31Z

commit messge to main:

feat: exposed installation method in debug mode
refs INSTA-778

For me, this isn't a feature but rather a fix, which is useful for troubleshooting. It doesn’t add any new functionality for the customer.

aryamohanan · 2024-08-16T08:13:53Z

packages/core/src/util/esm.js

@@ -66,3 +66,31 @@ exports.isESMApp = function isESMApp() {

  return isESM;
 };
+
+exports.tracerInstrumentationInfo = function tracerInstrumentationInfo() {


This is only applicable for the collector package? what about the serverless packages?

Right now considered only the collector package.
The serverless whole is another scope.

If we need the serverless information from args and env, then it can also be added I guess.

Instead of

includes('@instana/collector')

can check for

esm-register.mjs

If we need the serverless information from args and env, then it can also be added I guess.

Instead of

includes('@instana/collector')

can check for

esm-register.mjs

I don't understand.

The serverless whole is another scope.

Ok, then we can do it later if needed.

Instead of

includes('@instana/collector')

can check for

esm-register.mjs

this won't work btw

The serverless whole is another scope.

Yeah, I agree.

I believe we need to check the flag for all our packages. Checking @instana/collector would only apply to the collector package. We already have a similar check in place for detecting ESM, as seen here. I'm thinking of either extending or referencing this function . This approach would help us avoid maintaining flags in multiple functions. What do you think?

That would be a good idea. Initially, I designed the function like this but then later decided to check for a single package for checking serverless and collector specifically.
Re-using the existing function will be a good idea, even I thought of this. But since the outputs and checks differ in the function isESMApp() and function tracerInstrumentationInfo(), need to decide how can we extend this. I will get back to you after that. Please share if you have any better solutions in mind

Implemented a new logic!

aryamohanan · 2024-08-16T08:40:01Z

packages/core/src/util/esm.js

@@ -66,3 +66,31 @@ exports.isESMApp = function isESMApp() {

  return isESM;
 };
+
+exports.tracerInstrumentationInfo = function tracerInstrumentationInfo() {
+  const instrumentationMap = {


This logic should not be here, this file specific to esm.

Okay will update.

aryamohanan · 2024-08-16T08:51:54Z

packages/core/src/tracing/index.js

@@ -197,6 +198,10 @@ exports.init = function init(_config, downstreamConnection, _processIdentityProv
  automaticTracingEnabled = config.tracing.automaticTracingEnabled;

  if (tracingEnabled) {
+    const loader = tracerInstrumentationInfo();


IMO: This should be called only in debug mode. Could you explore the options to do that?

If there is an issue reported and we want to check the logs, then IMO it might be helpful to know the instrumentation details.
But for that, we can do console.info instead of console.debug!
Whats your take on that?

IMO this should only be logged if debug mode is enabled.

aryamohanan · 2024-08-19T07:46:37Z

packages/core/src/util/clientInstrumentationCheck.js

+      noFlags: 'no additional flags'
+    };
+
+    const usingExperimentalLoaderFlag =


Qs: Why isn't there a check for @instana/collector included here? I noticed that you added checks for other flags.

Thanks for catching. That was not added in the checklist.
I will update this and add to checklist

aryamohanan · 2024-08-19T07:52:15Z

I'm done with the review. Added one more comment.

aryamohanan

LGTM

kirrg001 · 2024-08-22T09:49:16Z

packages/core/src/tracing/index.js

+    const method = getPackageInstallation();
+
+    // eslint-disable-next-line no-console
+    console.debug(`The App has instrumented instana using: ${method}`);


issue: I am getting

The App has instrumented instana using: no additional flags

When doing

$ INSTANA_DEBUG=true node --require ~/dev/instana/nodejs-2/packages/collector/src/immediate.js app.js

I am getting correctly!! but why🤔

The path is different. Maybe we only cover specific use cases?

The reason is that you are referring to the packages directly.
The logic behind the comparison is checking for the @instana keyword in the arguments (as the clients are using our npm package).
So in a client app perspective it should work if they are including our package in the application start arguments

We could simplify the whole function by printing all values if --require, --import or --experimental-loader is used.

e.g. customer uses node --import X --import X app.js

Advantages:

less complicated logic, less checks

covers the use case that if a customer uses multiple import values to dig down potential problems -> conflicts

this is only debug information

covers more use cases (such as the case I showed)

The output would be:

console.debug(`The App is using the following preload flags: ${preloadFlags}`);

Yeah perfect 👍

kirrg001 · 2024-08-22T14:12:25Z

packages/core/src/tracing/index.js

@@ -179,6 +180,13 @@ exports.preInit = function preInit(preliminaryConfig) {
 * @param {CollectorPIDStore} _processIdentityProvider
 */
 exports.init = function init(_config, downstreamConnection, _processIdentityProvider) {
+  if (process.env.INSTANA_DEBUG || process.env.INSTANA_LOG_LEVEL === 'debug') {


I like this check a lot 👍

S: It would be cool if you could raise a second PR to add a utility for that. We use this check in many places e.g.

https://github.com/search?q=repo%3Ainstana%2Fnodejs%20%22if%20(process.env.INSTANA_DEBUG%20%7C%7C%20process.env.INSTANA_LOG_LEVEL%22&type=code

Yeah sure! 👍

kirrg001 · 2024-08-22T14:20:15Z

packages/core/src/tracing/index.js

+    const method = getPackageInstallation();
+
+    // eslint-disable-next-line no-console
+    console.debug(`The App has instrumented instana using: ${method}`);


We could simplify the whole function by printing all values if --require, --import or --experimental-loader is used.

e.g. customer uses node --import X --import X app.js

Advantages:

less complicated logic, less checks

covers the use case that if a customer uses multiple import values to dig down potential problems -> conflicts

this is only debug information

covers more use cases (such as the case I showed)

The output would be:

console.debug(`The App is using the following preload flags: ${preloadFlags}`);

abhilash-sivan · 2024-08-23T06:17:01Z

commit message:

fix: exposed preload flags in debug mode
refs INSTA-778

kirrg001 · 2024-08-23T08:01:53Z

packages/core/src/util/getPreloadFlags.js

+    process.env.NODE_OPTIONS?.includes('--require')
+    || process.env.NODE_OPTIONS?.includes('--import')
+    || process.env.NODE_OPTIONS?.includes('--experimental-loader')) {
+      return process.env.NODE_OPTIONS;


NODE_OPTIONS can contain multiple information..

Possible update:

const flags = ['--require', '--import', '--experimental-loader']; if (process.env.NODE_OPTIONS) { const relevantFlags = process.env.NODE_OPTIONS.split(' ').filter(option => flags.some(flag => option.includes(flag)) ); if (relevantFlags.length > 0) { return relevantFlags.join(' '); } } if (process.execArgv.length > 0) { const result = process.execArgv.find(arg => flags.some(flag => arg.includes(flag)) ); return result || 'noFlags'; } else { return 'noFlags'; }

NODE_OPTIONS can contain multiple information..

Thanks for sharing. I will update the same according to this suggestion 👍

since Array.prototype.some() function returns a boolean, we might have to make some changes in the returning string if the whole argument list is required in the debug logs.
But as you pointed out, the arguments and NODE_OPTIONS can contain more information that might not be relevant to us in the debug log.
Let me think of other options.

Any solution works :)

Instead of printing out the whole, now logging only the flags (--import, --require, etc)
Also added a check for @instana keyword which will be sufficient to verify that one/some of instana packages are passed in the argument list or as NODE_OPTIONS

packages/core/src/util/getPreloadFlags.js

abhilash-sivan · 2024-08-27T08:19:25Z

LGTM!

…ented instana/collector

…of host applications refs INSTA-778

…added ut

abhilash-sivan added the WIP label Aug 12, 2024

kirrg001 changed the title ~~feat: find out the method in which the client application has instrum…~~ feat: exposed installation method in debug mode Aug 13, 2024

abhilash-sivan marked this pull request as ready for review August 16, 2024 07:46

abhilash-sivan requested a review from a team as a code owner August 16, 2024 07:46

abhilash-sivan force-pushed the client-instrumentation-instana branch from 79e82dd to a754486 Compare August 16, 2024 07:46

aryamohanan reviewed Aug 16, 2024

View reviewed changes

kirrg001 removed the WIP label Aug 16, 2024

aryamohanan reviewed Aug 16, 2024

View reviewed changes

aryamohanan reviewed Aug 19, 2024

View reviewed changes

aryamohanan approved these changes Aug 19, 2024

View reviewed changes

aryamohanan requested a review from kirrg001 August 19, 2024 14:18

abhilash-sivan requested a review from aryamohanan August 21, 2024 05:18

kirrg001 requested changes Aug 22, 2024

View reviewed changes

kirrg001 reviewed Aug 22, 2024

View reviewed changes

abhilash-sivan force-pushed the client-instrumentation-instana branch from 6ea894a to b54a587 Compare August 23, 2024 06:15

kirrg001 reviewed Aug 23, 2024

View reviewed changes

kirrg001 requested changes Aug 24, 2024

View reviewed changes

packages/core/src/util/getPreloadFlags.js Show resolved Hide resolved

abhilash-sivan changed the title ~~feat: exposed installation method in debug mode~~ fix: exposed installation method in debug mode Aug 27, 2024

kirrg001 approved these changes Aug 27, 2024

View reviewed changes

abhilash-sivan added 4 commits August 28, 2024 09:46

feat: find out the method in which the client application has instrum…

80fb3cd

…ented instana/collector

chore: added debug information of @instana/collector instrumentation …

e612a5d

…of host applications refs INSTA-778

chore: fixed pr comments

11a5b60

chore: new logic using ESM check

2e18efd

abhilash-sivan added 8 commits August 28, 2024 09:46

chore: updated console message

34e5f75

chore: updated function name and logic

0e61c64

chore: refactor

2547bd7

chore: updated debug info, added preload flags

cd94c37

chore: updated the logic such that only the flag is printed

a55359b

chore: updated logic to printout value of required/imported file and …

6678c21

…added ut

chore: updated comments

7bd987f

chore: refactored code

f523ef0

abhilash-sivan force-pushed the client-instrumentation-instana branch from aa82d63 to f523ef0 Compare August 28, 2024 04:16

abhilash-sivan merged commit 775df15 into main Aug 28, 2024

abhilash-sivan deleted the client-instrumentation-instana branch August 28, 2024 04:17

fix: exposed installation method in debug mode #1268

fix: exposed installation method in debug mode #1268

Conversation

abhilash-sivan commented Aug 12, 2024 • edited Loading

abhilash-sivan commented Aug 16, 2024

abhilash-sivan commented Aug 16, 2024

aryamohanan commented Aug 16, 2024

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

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

aryamohanan commented Aug 19, 2024

aryamohanan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhilash-sivan commented Aug 23, 2024

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

abhilash-sivan commented Aug 27, 2024

abhilash-sivan commented Aug 12, 2024 •

edited

Loading