use our own app insights client to record test results #9715
Conversation
amunger
force-pushed
the
dev/aamunger/appInsightsClient
branch
from
1f3bc90 to
1d752a8
Compare
| * An isolated wrapper for an Application Insights client that is specifically for sending telemetry during CI jobs. | ||
| * This won't run on a users machine, so there is no need to check for opt-in status. | ||
| */ | ||
| export class CiTelemetryReporter extends BaseTelemetryReporter { |
There was a problem hiding this comment.
Instead of pulling in copies of the code vscode-extension-telemetry, could you just extend the TelemetryReporter in the extension-telemetry npm module?
There was a problem hiding this comment.
not the way it's currently written - getTelemetryLevel is a utility function which can't be overridden, and is used as a guard on multiple levels
There was a problem hiding this comment.
Not sure I'm following. Sorry.
getTelemetryLevel isn't overridden here?
Did you change it in the BaseTelemetryReporter?
There was a problem hiding this comment.
I removed getTelemetryLevel entirely since this will only collect telemetry in CI, but in the npm package, it's a global utility function that checks the vscode setting.
There was a problem hiding this comment.
Could you monkey patch it in the npm code? Just remove it at run time?
I guess I'm not too sure about making a copy of the extension-telemetry code (with modifications).
There was a problem hiding this comment.
Monkey patching in the test only code sounds safer to me than copying the code over.
|
This is a surprising amount of copied code. In theory since it's just perf objects you wouldn't need most of the code and could just call |
I believe the solution (as discussed yesterday) is to enable telemetry on our CI server and just send the telemetry events specific to the tests on CI servers. |
vscode internally disables any telemetry during testing, so we can't just set a configuration to enable it. Monkey patching the function that checks if it's enabled might work, but that also might send additional telemetry from vscode as well |
|
I will look into a module level solution for this and report back with my findings. |
|
still waiting on Logan's thoughts, I just wanted to push up the work that I had done earlier |
Codecov Report
@@ Coverage Diff @@
## main #9715 +/- ##
=====================================
Coverage 63% 63%
=====================================
Files 204 204
Lines 9840 9854 +14
Branches 1556 1558 +2
=====================================
+ Hits 6242 6261 +19
+ Misses 3335 3092 -243
- Partials 263 501 +238
|
|
Just an update that we have added this native functionality to the module so it should be much easier to implement this and involve no code copying. |
Fixes #9218
Pulled in a lot of the npm package from https://github.com/microsoft/vscode-extension-telemetry to be able to send telemetry during CI tests.
I did not pull in the web-based version, since I'm not sure where/how that would be used at this point.
I had to add a
sleepcall to let the reporter flush, even after awaiting the dispose which is supposed to do that. If I ran a single test without thesleepcall, the event would never show up in Kusto.