Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aeschli opened this issue Mar 20, 2023 · 4 comments
Assignees

Comments

@aeschli
Copy link

aeschli commented Mar 20, 2023

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
Copy link
Author

aeschli commented Mar 20, 2023

FYI @lramos15

@lramos15
Copy link
Member

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?

@aeschli
Copy link
Author

aeschli commented Mar 21, 2023

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
Copy link
Author

aeschli commented Mar 21, 2023

See microsoft/ApplicationInsights-node.js#1102 for the @azure/functions-core issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants