microsoft / vscode-extension-telemetry

Node module to help VS Code extensions send telemetry using application insights
https://www.npmjs.com/package/@vscode/extension-telemetry
Other
124 stars 46 forks source link

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

Open aeschli opened 1 year ago

aeschli commented 1 year ago

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)

aeschli commented 1 year ago

FYI @lramos15

lramos15 commented 1 year ago

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?

aeschli commented 1 year ago

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 commented 1 year ago

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