vscode-extension-telemetry icon indicating copy to clipboard operation
vscode-extension-telemetry copied to clipboard

Webpack `vscode/extension-telemetry` to avoid that clients need to know exclusion rules

Open aeschli opened this issue 1 year ago • 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/[email protected] 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/[email protected] 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 on vscode/extension-telemetry.
  • This allows vscode/extension-telemetry to further reduce its size by defining stubs for even more functionality that isn't used.

aeschli avatar Mar 20 '23 19:03 aeschli

FYI @lramos15

aeschli avatar Mar 20 '23 19:03 aeschli

We used to bundle, but stopped with https://github.com/microsoft/vscode-extension-telemetry/pull/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?

lramos15 avatar Mar 20 '23 20:03 lramos15

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.

aeschli avatar Mar 21 '23 09:03 aeschli

See https://github.com/microsoft/ApplicationInsights-node.js/issues/1102 for the @azure/functions-core issue.

aeschli avatar Mar 21 '23 09:03 aeschli