Upgrade appinsights #58999
Upgrade appinsights #58999
Conversation
ramya-rao-a
force-pushed
the
ramyar/appinsights
branch
from
c5277da
to
05a1228
Compare
| // Set the below before requiring applicationinsights in the shared process | ||
| process.env['APPLICATION_INSIGHTS_NO_DIAGNOSTIC_CHANNEL'] = 'true'; // Skip monkey patching of 3rd party modules by appinsights | ||
| global['diagnosticsSource'] = {}; // Prevents diagnostic channel (which patches "require") from initializing entirely | ||
|
|
There was a problem hiding this comment.
This is the workaround to avoid the monkey patching that gets done by the newer version of application insights. It needs to be placed before application insights module gets imported. Based on the way we load things, this is definitely the safest place for the workaround, but maybe not the best.
Any ideas on any other place thats right for this?
There was a problem hiding this comment.
@bpasero Maybe the bootstrap files?
There was a problem hiding this comment.
@ramya-rao-a you can add a helper method to bootstrap.js and then call it from where we need it, looks like:
src/cli.jssrc/vs/code/electron-browser/sharedProcess/sharedProcess.js
There was a problem hiding this comment.
@bpasero Pushed a commit to make the change as you suggested.
It works as expected for the shared process, but not for the cli.
I added some console logs in the modules that do the patching and below is what I see when running the cli
The application insights module gets loaded even before https://github.com/Microsoft/vscode/blob/master/scripts/code-cli.bat#L24
Thoughts?
cc @joaomoreno @jrieken
|
@bpasero Pushed a commit to make the change as you suggested. It works as expected for the shared process, but not for the cli.
The application insights module gets loaded even before https://github.com/Microsoft/vscode/blob/master/scripts/code-cli.bat#L24 Thoughts? |
Thanks lgtm |
This PR upgrades the version of appinsights we use. Fixes #44052 & #55417
I have tested the telemetry gets logged as expected.
What's pending in this review is the right place to add the workaround we need to ensure the newer version of application insights does not patch any modules. See microsoft/ApplicationInsights-node.js#425