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

prevent require() patching #16

Closed
bpasero opened this issue Apr 10, 2018 · 9 comments
Closed

prevent require() patching #16

bpasero opened this issue Apr 10, 2018 · 9 comments
Assignees

Comments

@bpasero
Copy link
Member

bpasero commented Apr 10, 2018

By using APPLICATION_INSIGHTS_NO_PATCH_MODULES

via #12 (comment)

@ramya-rao-a
Copy link
Contributor

@bpasero APPLICATION_INSIGHTS_NO_PATCH_MODULES is only used if APPLICATION_INSIGHTS_NO_DIAGNOSTIC_CHANNEL is not used.

We set APPLICATION_INSIGHTS_NO_DIAGNOSTIC_CHANNEL to true, so using APPLICATION_INSIGHTS_NO_PATCH_MODULES will not help

See https://github.com/Microsoft/ApplicationInsights-node.js/blob/develop/AutoCollection/diagnostic-channel/initialization.ts

@bpasero
Copy link
Member Author

bpasero commented Apr 11, 2018

@ramya-rao-a we should consider an alternative solution that does not patch our require function if we cannot disable this questionable feature...

@ramya-rao-a
Copy link
Contributor

Logged upstream issue microsoft/ApplicationInsights-node.js#425

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Sep 17, 2018

@bpasero Turns out there is a workaround we can apply that will prevent the monkey patching of the require module. The workaround is to update one of the global properties before calling appinsights. See microsoft/ApplicationInsights-node.js#425 (comment)

But, there are third party extensions that use the ApplicationInsights module directly and so will end up monkey patching the require module anyway. And so, I don't see a way to escape the patching for the extension host.

https://github.com/Microsoft/node-diagnostic-channel/blob/master/src/diagnostic-channel/src/channel.ts#L128-L137 is the exact place where the monkey patching takes place for require

When we upgrade app insights module for VS Code core, we will have to use the same workaround.

@bpasero
Copy link
Member Author

bpasero commented Sep 20, 2018

@ramya-rao-a not sure I follow, if we do microsoft/ApplicationInsights-node.js#425 (comment) how can "third party extensions use the ApplicationInsights module directly" and get the old behaviour?

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Sep 20, 2018

@bpasero

Oops, my mistake. I somehow assumed the workaround had to be taken by every extension that uses the application insights module (with or without vscode-extension-telemetry). That of course is not the case.

I can add the workaround to vscode-extension-telemetry, (without resetting global.diagnosticsSource back to its original value as discussed in the upstream issue). The built-in extensions get activated first and will execute the workaround.

The only catch is that if there are 3rd party extensions using the diagnostic-channel, then that feature would break.

@bpasero
Copy link
Member Author

bpasero commented Sep 20, 2018

@ramya-rao-a yeah I think thats fine. If you set this very early in the module I think we are good. Thanks.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Sep 20, 2018

On second thoughts, this might cause such 3rd party extensions to crash/misbehave or best case scenario fail gracefully

Diagnostic channel will look enabled to code that knows how to use it and will crash when access is attempted

@ramya-rao-a
Copy link
Contributor

The latest version of vscode-extension-telemtetry module (0.0.22) uses the latest version of applicationinsights module (1.0.5) which has the fix to prevent unnecessary patching of the require and other 3rd party modules.

This was referenced Oct 2, 2018
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