Provide alternative to using __dirname for bundling with Webpack

[chrmarti]

Context is using __dirname to read the package.json of the containing module and of the AI module itself. That doesn't work well when using Webpack for bundling where the folder structure is flat and included node modules' (e.g., AI's) package.json are not bundled at all.

1. Here the packageJsonPath isn't passed through: https://github.com/Microsoft/ApplicationInsights-node.js/blob/de8679a319d09d98d86cc0f831c0acf3f3e5621d/Library/Context.ts#L28

2. Here the version could be inlined at build time to avoid the dependency on the package.json: https://github.com/Microsoft/ApplicationInsights-node.js/blob/de8679a319d09d98d86cc0f831c0acf3f3e5621d/Library/Context.ts#L58

/cc @jrieken

[jrieken]

cc @OsvaldoRosado we somehow missed that when we started webpack'ing appinsights. Biggest question is what current consequences this has?

[chrmarti]

I just discovered that the second case can be fixed by using require('../../package.json'). That works in the unpacked case and Webpack will simply pack the entire file.

The first case would be helped by passing the mentioned parameter along.

(I should add: I do see the events in Kusto now and we seem to add enough of our own metadata to now who sent it and in which version. So we might be good for now.)

[OsvaldoRosado]

@chrmarti Good catch with packageJsonPath not being passed through! As for inlining the version, we'll need more infrastructure than our current build process (literally only tsc) to get that working. Nothing that can't be done but it's non-trivial to do without being hacky.

@jrieken The current consequences aren't super critical. If the package.json for the host application can't be found, we simply won't report the version of the host application. Same goes for if the package.json of the SDK itself can't be found. The SDK shouldn't stop working or anything like that.

If necessary, you can easily set these variables yourselves in code the same as any other context tag (eg. client.context.tags[client.context.keys.applicationVersion] = "myversion") which may be a helpful workaround in scenarios where the SDK doesn't automatically populate the field.

[chrmarti]

@OsvaldoRosado Instead of inlining yourself, you could use require with a string literal, that makes it work for the normal and the webpacked case: require('../../package.json')

[OsvaldoRosado]

@chrmarti I don't think we're able to use require here. We used to but we switched to JSON.parse because of the potential for unexpected code execution found while threat modeling. Since the path is relative, we don't really know for sure what will be existing there after the two backreferences.

[chrmarti]

FYI, when requiring a .json file Node.js sends it through JSON.parse also when it contains JavaScript code:

> node testrequirejson.js                                                                                                                                                                  8.9.4
module.js:665
    throw err;
    ^

SyntaxError: /Users/chrmarti/Development/repos/test/testrequirejson.json: Unexpected token c in JSON at position 0
    at JSON.parse (<anonymous>)
    at Object.Module._extensions..json (module.js:662:27)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Module.require (module.js:587:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/chrmarti/Development/repos/test/testrequirejson.js:1:63)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)

[OsvaldoRosado]

@chrmarti This is true, but if that file isn't found it will attempt to read a .json.js file of the same name - making it arguably unsafe to use require in unknown directories. For example:

[chrmarti]

I see, you could use fs.existsSync first. Fixing this is not urgent for us though, since we still get the events.

[Huachao]

@chrmarti @OsvaldoRosado any update on this issue?

[OsvaldoRosado]

@Huachao The current state is still to follow the advice above. I've quoted it below for convenience.

You can easily set these variables yourselves in code the same as any other context tag (eg. client.context.tags[client.context.keys.applicationVersion] = "myversion") which may be a helpful workaround in scenarios where the SDK doesn't automatically populate the field.

Would love to know if there are scenarios where this advice doesn't work!

[Huachao]

@OsvaldoRosado I think actually if we want to be consistent with the non-bundling behavior, we may need to add more tags like internalSdkVersion. Currently, my workaround is to add the applicationinsights as an external in the webpack config file. And do you have any plan to solve this in the future?

[OsvaldoRosado]

@Huachao there isn't an immediate plan to make a workaround within the SDK. Ideas (or PRs) are very welcome! The current best suggestion (fs.existsSync + require) seems somewhat hacky with a very very short race condition still allowing unexpected code execution. Is there perhaps another way this SDK can indicate to webpack that a file should be included other than using require?

IIRC, the application version and SDK version are the only variables populated by fs.readFileSync-based data - so these should be the only manual populations necessary if that's helpful.