Telemetry #381
Telemetry #381
Conversation
…f some datapoints. This provides infrastructure for for adding concrete implementations for telemetry logger which will implement ITelemetryFactory.
…do some cleanup e.g. flushing data can do it. Using $(VSTarget) token instead of hardcoded version numbers in telemetry project file
- renamed DefaultTelemetryLogger and factory to DummyTelemetryLogger ... - some namespace sorting and tab/space alignment fixes - made classes for telemetry constants static
- Removed all references to vslogger - Added some overrides in Dapapointcollection to add items more conviniently - some minor alignment/typo fixes - Made Telemetry project anycpu and removed unnecessary items from it. set comvisible false for it - Extracted code to set some common telemetry properties and logging of packageloaded event to a more common place so that it can be called from any package - removed logging of project open/create as it is already covered by vs telemetry
using new approach to copy and reference private telemetry dlls directly. Updating nuget.exe to latest as that is required by latest stable app insight nugets
|
Hi @nareshjo, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| @@ -613,6 +615,8 @@ | |||
| <Compile Include="SmartIndent.cs" /> | |||
| <Compile Include="SmartIndentProvider.cs" /> | |||
| <Compile Include="SourceMapping\SourceMap.cs" /> | |||
| <Compile Include="Telemetry\TelemetryEvents.cs" /> | |||
| <Compile Include="Telemetry\TelemetryProperties.cs" /> | |||
There was a problem hiding this comment.
If these files are going to be shared across projects, they should be included alongside the Telemetry dlls in Nodejs/Common, and then linked to from the individual projects.
|
Left some comments and lgtm after that |
Moved Telemetry events and properties classes to shared project and marked them internal Including app insights dlls in Dev11 and 12 only Updated telemetry dlls Fixed indentation some other minor fixes
| internal static class TelemetryEvents { | ||
| public const string ProjectImported = "ProjectImported"; | ||
| } | ||
| } |
There was a problem hiding this comment.
I meant to move this to Nodejs/Common, not Common/* alongside the rest of the dlls.
Again, Common/* are shared components with Python Tools, and pretty much anyone who's forked us.
Nodejs/Common/* are shared components within the Nodejs product that we can reference from different NTVS projects.
|
lgtm after the above comment about location is addressed. |
|
👍 |
|
Hey, I spent some time debugging the signed binary issue... The problem is that the new telemetry binary was not getting copied to <Copy SourceFiles="@(OutputBinariesToSign -> '$(OutputPath)%(filename)%(extension)')"
DestinationFolder="$(CopyOutputsToPath)UnsignedBinaries"
Condition="'@(OutputBinaries)' != ''"/>Then in the project file, you can define the following item collection to ensure the dll is copied to the right location for signing. <OutputBinariesToSign Include="Microsoft.NodejsTools.Telemetry.$(VSTarget).dll;SomeOtherAssembly.dll" />cc @sanyamc-msft since this'll also apply to your PR. |
|
Thanks! I added the above copy steps to my PR and pushed the changes for review. Need to test it again on build machine. |
…s folder during build
…ith a parallel PR and avoid conflicts later
|
👍 Does everything appear to be functional on the build machine now? If so, this is ready to be merged in. |
|
Yes everything is working as expected in my tests using signed installer. Mergining in |
|
It seems I don't have permission to merge. Could you please do that. |
First set of telemetry infrastructure changes
Open piece of NTVS telemetry which consumes private telemetry loggers