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.

Sign up for GitHub

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

Jump to bottom

Add metrics for datacollector.exe - provides information about profilers #2705

Merged
merged 12 commits into from Jan 25, 2021
Merged

Add metrics for datacollector.exe - provides information about profilers #2705

merged 12 commits into from Jan 25, 2021

Conversation

jakubch1
Copy link
Member

Description

PR adds logic to add data collectors metrics with information which profiler was used

@jakubch1 jakubch1 changed the title Initial version of metrics for DC profiling Add metrics for datacollector.exe - provides information about profilers Jan 20, 2021
Haplois
Haplois approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@Haplois Haplois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I have one concern. Since we're adding new types, is this version of TP working without problems on older versions of Visual Studio when used thru Test Explorer?

@jakubch1
Copy link
Member Author

LGTM, but I have one concern. Since we're adding new types, is this version of TP working without problems on older versions of Visual Studio when used thru Test Explorer?

I've changed communication between vstest.console.exe and datacollector.exe. This components are always installed together so I don't expect any issues with that.

AbhitejJohn
AbhitejJohn reviewed Jan 21, 2021
View reviewed changes
Copy link
Contributor

@AbhitejJohn AbhitejJohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check with @cvpoienaru for end-to-end scenarios that need validation with this change.

src/Microsoft.TestPlatform.Common/DataCollection/AfterTestRunEndResult.cs
/// Payload object that is used to exchange data between datacollector process and runner process.
/// </summary>
[DataContract]
public class AfterTestRunEndResult
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we require this to be public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. When tried internal than I got:
Severity Code Description Project File Line Suppression State
Error CS0050 Inconsistent accessibility: return type 'AfterTestRunEndResult' is less accessible than method 'DataCollectionRequestSender.SendAfterTestRunEndAndGetResult(ITestMessageEventHandler, bool)' Microsoft.TestPlatform.CommunicationUtilities (net451), Microsoft.TestPlatform.CommunicationUtilities (netstandard1.3), Microsoft.TestPlatform.CommunicationUtilities (netstandard2.0) D:\vstest\src\Microsoft.TestPlatform.CommunicationUtilities\DataCollectionRequestSender.cs 153 Active

SendAfterTestRunEndAndGetResult is part of interface so it cannot be done internal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that is sad. Adding @cvpoienaru to ensure that we aren't making/breaking any compat promises with this. From what I know, I think we are good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already discussed it with @cvpoienaru

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Outdated Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Outdated
requestData.MetricsCollection.Add(string.Format(telemetryTemplateName, dataCollectorInformation.DataCollectorConfig?.TypeUri?.ToString()), GetProfilerName(value));
}

private void AddProfilerMetricForConflictedVariable(string profilerVariable, string telemetryTemplateName, DataCollectorInformation dataCollectorInformation, string name, string existingValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: Could we merge with the above and make this a for-loop over the environment variables of interest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In such case we need to copy logic which we already have to detect conflicts and so on. I would keep it like it is now. I want to keep this class making only simple metrics, nothing more

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it a little bit simpler. We have 2 methods: RecordEnvVariableAddition and RecordEnvVariableConflict.

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Show resolved Hide resolved
src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionTelemetryManager.cs Outdated Show resolved Hide resolved
@jakubch1 jakubch1 merged commit bb39536 into microsoft:master Jan 25, 2021
@jakubch1 jakubch1 deleted the dev/jachocho/telemetry branch January 25, 2021 16:22
jakubch1 added a commit that referenced this pull request Feb 5, 2021
@jakubch1 @dotnet-maestro
@fhnaseer @adamralph @Haplois @vritant24 @jflepp @cvpoienaru @cristianosuzuki77 @pavelhorak @Sanan07 @nohwnd @tmds
Update rel/16.9 to latest commit pushed to VS (#2734)
73113fe 
* Update dependencies from https://github.com/dotnet/arcade build 20201130.3 (#2659)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild
 From Version 1.0.0-beta.20570.10 -> To Version 1.0.0-beta.20580.3

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Updating Microsoft.VisualStudio.TraceDataCollector source (#2663)

* Getting Microsoft.VisualStudio.TraceDataCollector from CodeCoverageExternals  instead of TestPlatformExternals,

* Signing TraceDataCollector from the right path,

* Removing already signed dlls,

* Not signing corelib.net,

* Removing files from the list as they are not present,

Co-authored-by: faisal <faisalhafeez@microsoft.com>

* Fixed "issue" pluralization in write-release-notes.ps1 (#2665)

* Fixed "issue" pluralization in write-release-notes.ps1

Co-authored-by: Medeni Baykal <433724+Haplois@users.noreply.github.com>

* Bumping Fakes TestRunnerHarness version (#2661)

* Implement Workitem support in TRX logger (#2666)

* Implemented Workitem support in TRX logger

* Renamed Workitem to WorkItem

Co-authored-by: Flepp Jann <JFlepp@Hamilton.ch>

* Do not merge logs from code coverage (#2671)

* Early testhost startup performance work (#2584)

* Removed TypesToLoadAttribute from ObjectModel. (#2674)

* Removed TypesToLoadAttribute from ObjectModel, and moved the functionallity into adapters themselves.

* Attribute refactoring (#2676)

* Getting TraceDataCollector from nuget (#2678)

* Removing TraceDataCollector project,

* Getting tracedatacollector from nuget,

* Removing signing of codecoverage libs as they are already signed,
updating codecoverage version,

* fixing path to tracedatacollector,

* Fixing acceptance tests,

Co-authored-by: faisal <faisalhafeez@microsoft.com>

* Adding environment variable used during build process, (#2679)

* Removing TraceDataCollector project,

* Getting tracedatacollector from nuget,

* Removing signing of codecoverage libs as they are already signed,
updating codecoverage version,

* fixing path to tracedatacollector,

* Fixing acceptance tests,

* Setting TraceDataCollectorPackagesDir vairable which is used in pipeline,

* Fixing environment variable,

* Fixing path, once again,

Co-authored-by: faisal <faisalhafeez@microsoft.com>

* Update dependencies from https://github.com/dotnet/arcade build 20201221.2 (#2680)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild
 From Version 1.0.0-beta.20580.3 -> To Version 1.0.0-beta.20621.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Upgrade CC and CLR IE versions (#2681)

Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>

* Fixed assembly names of TestHost executables (#2682)

* Upgrade fakes version (#2683)

Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>

* Upgrade CC to 16.9.0-beta.20630.1 (#2684)

Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>

* Loc Update (#2685)

Co-authored-by: Cristiano Suzuki <crsuzuki@microsoft.com>

* Create AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Delete AzureDevOps.yml

* Fix nuget feed (#2697)

* Removing deprecated mygets and adding new nuget sources

* Upgrade CC components (#2698)

* Upgrade CC components

* Revert nuget changes

* Refactor it

Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>

* Update dependencies from https://github.com/dotnet/arcade build 20210113.4 (#2696)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild
 From Version 1.0.0-beta.20621.2 -> To Version 1.0.0-beta.21063.4

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Create AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Update AzureDevOps.yml

changed to my repo

* Update AzureDevOps.yml

* Add integration tests for dotnet test (#2689)

* Add integration tests for dotnet test

* Use relative paths and correct proj

* Update to latest .NET 6

* Install 5.0.1 runtime

* Add parametrized project

* Updated build status badge. (#2709)

* Updated build status badge.

* Update dependencies from https://github.com/dotnet/arcade build 20210121.4 (#2712)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild
 From Version 1.0.0-beta.21063.4 -> To Version 1.0.0-beta.21071.4

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Update dependencies from https://github.com/dotnet/arcade build 20210122.7 (#2713)

Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SignTool , Microsoft.DotNet.SwaggerGenerator.MSBuild
 From Version 1.0.0-beta.21063.4 -> To Version 1.0.0-beta.21072.7

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Medeni Baykal <433724+Haplois@users.noreply.github.com>

* Update azure-pipelines.yml for Azure Pipelines (#2715)

* Add metrics for datacollector.exe - provides information about profilers (#2705)

* Initial version of metrics for DC profiling

* Tests

* Push fixes to make linux build

* provide telemetry opted in flag

* Small refactoring

* Change name

* Upgrade CC comp

* Move to guids

* Revert lang change

* Fixing descriptions

* Last changes

Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>

* Added git branch and commit NuGet packages (#2716)

* vstest.console: CommandLineOptions: preserve stacktrace on re-throw (CA2200) (#2606)

* vstest.console: CommandLineOptions: preserve stacktrace on re-throw (CA2200)

* Update AzureDevOps.yml

* Update AzureDevOps.yml

* Move FQN related code into a separate NuGet package (#2714)

* Removed ManagedNameUtilities from ObjectModel in order to restore compatibility with older versions.
* Added `Microsoft.TestPlatform.AdapterUtilities`.
* Added `net20` and `net35` support to the code.

* Including `Microsoft.TestPlatform.AdapterUtilities` in signing (#2719)

* Included missing assemblies in signing.

* Updating nuget Pdb2Pdb package (#2720)

* Adding new Pdb2Pdb package and change to licence tag because of warning
* Fixing license
* Formating fixes

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: fhnaseer <fhnaseer@live.com>
Co-authored-by: faisal <faisalhafeez@microsoft.com>
Co-authored-by: Adam Ralph <adam@adamralph.com>
Co-authored-by: Medeni Baykal <433724+Haplois@users.noreply.github.com>
Co-authored-by: Vritant Bhardwaj <vrbhardw@microsoft.com>
Co-authored-by: jflepp <jflepp@users.noreply.github.com>
Co-authored-by: Flepp Jann <JFlepp@Hamilton.ch>
Co-authored-by: Codrin-Victor Poienaru <cvpoienaru@gmail.com>
Co-authored-by: Jakub Chocholowicz <jachocho@microsoft.com>
Co-authored-by: Cristiano Suzuki <cristianosuzuki77@gmail.com>
Co-authored-by: Cristiano Suzuki <crsuzuki@microsoft.com>
Co-authored-by: Pavel Horak <22235234+pavelhorak@users.noreply.github.com>
Co-authored-by: Sanan Yuzbashiyev <Sanan07@users.noreply.github.com>
Co-authored-by: Jakub Jareš <me@jakubjares.com>
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhitejJohn AbhitejJohn AbhitejJohn approved these changes

@Haplois Haplois Haplois approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jakubch1 @Haplois @AbhitejJohn