logModulesLoadedBeforeTrace should log a warning not an error, and should have a flag to disable the warning

[bag-man]

The function logModulesLoadedBeforeTrace will currently log an error, which is not appropriate for a warning to a developer.

It should be changed to log a warning, so as to not trigger error reports when a service is restarted. Ideally the module should also have an option to disable this warning.

I'm happy to make a PR to do this if considered a valid change.

[kjin]

@bag-man I agree with this change, a PR would be great (ignore the labels/assignee). Thanks!

[bag-man]

@kjin Is there a branch that I should work off of?

Master doesn't build:

cloud-trace-nodejs (master) λ npm run compile

> @google-cloud/trace-agent@4.0.1 compile /home/owg1/Projects/cloud-trace-nodejs
> npm run script get-plugin-types compile-auto compile-auto-strict


> @google-cloud/trace-agent@4.0.1 script /home/owg1/Projects/cloud-trace-nodejs
> ts-node -P ./scripts/tsconfig.json ./scripts "get-plugin-types" "compile-auto" "compile-auto-strict"

> Running step: get-plugin-types
> Running step: compile-auto
> Running: `node_modules/typescript/lib/tsc -p ./tsconfig.full.json --target es2017`
test/plugins/test-trace-mongoose-async-await.ts:34:29 - error TS2559: Type '{ f1: string; f2: boolean; f3: number; }' has no properties in common with type 'Partial<Document>'.

34     const data = new Simple(doc);
                               ~~~


Found 1 error.

Error: Process 18783 exited with code 2.
    at ChildProcess.childProcess.on (/home/owg1/Projects/cloud-trace-nodejs/scripts/utils.ts:41:14)
    at ChildProcess.emit (events.js:198:13)
    at ChildProcess.EventEmitter.emit (domain.js:448:20)
    at maybeClose (internal/child_process.js:982:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)

[kjin]

Should be master, but seems like a type definition changed under our feet. Let me fix that!

[kjin]

@bag-man Sorry -- should have mentioned that I fixed the build issue in #1067.

Let me know if you want me to do this change instead.

[bag-man]

@kjin If you could make the fix, you could probably get it up quicker than I could, and it would be greatly appreciated!

[bag-man]

Nice one thanks! :D I might make a PR to add an option to disable the warning for production if that would be accepted?

[kjin]

Sure, let me know if you want me to add it in as well. A note though, if you are proposing using NODE_ENV to detect whether whether we are running in production, I'm not sure if this can be uniliterally accepted by all of our users, so I'd suggest maybe using a different environment variable (like GCLOUD_TRACE_DISABLE_UNTRACED_MODULES_WARNING -- it's a mouthful, but the shortest descriptive env var name I could think of).

[bag-man]

I wasn't thinking of using a env var, rather just another option in the configuration object, so those who want to disable it can

disableUntracedModulesWarning: true

[kjin]

Sure, that works!

[bag-man]

When would these changes get published?

[kjin]

Ah, I was waiting on your PR. If it's something you need now let me go ahead and make the config option change + release.

[bag-man]

I started on a PR today, but Google's setup had me a bit stumped unfortunately. If it's a quick change for you before release it would be super helpful and greatly appreciated!

[kjin]

Yup: #1070

[kjin]

Hi @bag-man, this has been released in 4.1.0.