Webpack `vscode/extension-telemetry` to avoid that clients need to know exclusion rules
aeschli opened this issue · 4 comments
When an extension has a dependency to vscode/extension-telemetry
and wants to webpack itself, it struggles with vscode/extension-telemetry as not all dependences can be resolved
for @vscode/extension-telemetry@0.7.5
the following is necessary:
externals: {
'applicationinsights-native-metrics': 'commonjs applicationinsights-native-metrics', // ignored because we don't ship native module
'@opentelemetry/tracing': 'commonjs @opentelemetry/tracing', // ignored because we don't ship this module
'@opentelemetry/instrumentation': 'commonjs @opentelemetry/instrumentation', // ignored because we don't ship this module
'@azure/opentelemetry-instrumentation-azure-sdk': 'commonjs @azure/opentelemetry-instrumentation-azure-sdk', // ignored because we don't ship this module
},
for @vscode/extension-telemetry@0.7.7
this is not enough:
WARNING in ./node_modules/applicationinsights/out/AutoCollection/AzureFunctionsHook.js 51:40-72
Module not found: Error: Can't resolve '@azure/functions-core' in '/home/martin/workspaces/vscode-remote-wsl/node_modules/applicationinsights/out/AutoCollection'
It would be better if vscode/extension-telemetry
uses a packager itself (webpack or esbuild)
- it can be easily consumed without the knowledge above.
- all it's
dependencies
go away (become devDependencies). Which reduces the number of node modules that need to be installed when depending onvscode/extension-telemetry
. - This allows
vscode/extension-telemetry
to further reduce its size by defining stubs for even more functionality that isn't used.
We used to bundle, but stopped with #119 as suggested by a bunch of people on the team as bundlers didn't play nice with my minified pre-bundled output.
Do we know why we need so many externals and these aren't included when we NPM install and then properly tree shaked by the bundler? I find the telemetry package very difficult and I'm not sure why. What is the best practice for shipping an npm package?
IMO, in general, packaging a utility node module is not recommended. But applicationinsights
is a beast as it bundles so/too much functionality that is enabled/disabled at runtime.
Ideally applicationinsights
would be broken up into pieces so that clients can pick what they need and get the minimal number of dependencies.
More dependencies mean more compliance work, more disk footprint, in the worst case even bigger product size.
Given that vscode-extension-telemetry
is the one that chooses which features it wants, I think it's in the best position to tame the beast.
So unless we can use something leaner than applicationinsights
, I still suggest packaging.
I would recommend to package it to ESM in non-minified form to make it friendly to re-packaging.
As a side note, it looks like we disable all functionality but setUseDiskRetryCaching
and setAutoCollectIncomingRequestAzureFunctions
. I guess the last one we should also disable.
See microsoft/ApplicationInsights-node.js#1102 for the @azure/functions-core
issue.