-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add telemetry logger support for API-based MSBuild usage with BannedApiAnalyzer protection #51068
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
base: release/10.0.2xx
Are you sure you want to change the base?
Changes from 1 commit
58aeab6
0b8fb6f
c37c61a
1dc7f54
c201734
6d409d6
51f1399
9343a69
65f60f0
102371a
3cd34a8
3093c60
31e17b7
6eb6064
243ad7f
151ef1a
bb53470
ef5751b
5825147
6de6502
ea3fc88
ea27e91
4540b6a
d3b9d3d
d2a96eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Co-authored-by: baronfel <[email protected]>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Globalization; | ||
|
|
@@ -119,8 +119,6 @@ public void Initialize(IEventSource eventSource) | |
|
|
||
| eventSource.BuildFinished += OnBuildFinished; | ||
| } | ||
|
|
||
| eventSource.BuildFinished += OnBuildFinished; | ||
|
Comment on lines
122
to
123
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot why was this event registration removed? It's necessary for the task telemetry reporting. in fact, revert all changes to this file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in commit 28602f4. The |
||
| } | ||
| catch (Exception) | ||
| { | ||
|
|
@@ -135,18 +133,17 @@ private void OnBuildFinished(object sender, BuildFinishedEventArgs e) | |
|
|
||
| internal void SendAggregatedEventsOnBuildFinished(ITelemetry? telemetry) | ||
| { | ||
| if (telemetry is null) return; | ||
| if (_aggregatedEvents.TryGetValue(TaskFactoryTelemetryAggregatedEventName, out var taskFactoryData)) | ||
| { | ||
| Dictionary<string, string?> taskFactoryProperties = ConvertToStringDictionary(taskFactoryData); | ||
| var taskFactoryProperties = ConvertToStringDictionary(taskFactoryData); | ||
|
|
||
| TrackEvent(telemetry, $"msbuild/{TaskFactoryTelemetryAggregatedEventName}", taskFactoryProperties, toBeHashed: [], toBeMeasured: []); | ||
| _aggregatedEvents.Remove(TaskFactoryTelemetryAggregatedEventName); | ||
| } | ||
|
|
||
| if (_aggregatedEvents.TryGetValue(TasksTelemetryAggregatedEventName, out var tasksData)) | ||
| { | ||
| Dictionary<string, string?> tasksProperties = ConvertToStringDictionary(tasksData); | ||
| var tasksProperties = ConvertToStringDictionary(tasksData); | ||
|
|
||
| TrackEvent(telemetry, $"msbuild/{TasksTelemetryAggregatedEventName}", tasksProperties, toBeHashed: [], toBeMeasured: []); | ||
| _aggregatedEvents.Remove(TasksTelemetryAggregatedEventName); | ||
|
|
@@ -166,10 +163,14 @@ internal void SendAggregatedEventsOnBuildFinished(ITelemetry? telemetry) | |
|
|
||
| internal void AggregateEvent(TelemetryEventArgs args) | ||
| { | ||
| if (args.EventName is null) return; | ||
| if (!_aggregatedEvents.TryGetValue(args.EventName, out Dictionary<string, int>? eventData) || eventData is null) | ||
| if (args.EventName is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!_aggregatedEvents.TryGetValue(args.EventName, out var eventData)) | ||
| { | ||
| eventData = new Dictionary<string, int>(); | ||
| eventData = []; | ||
| _aggregatedEvents[args.EventName] = eventData; | ||
| } | ||
|
|
||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: stray spaces in this file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.CommandLine; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Runtime.Versioning; | ||
| using System.Text.RegularExpressions; | ||
|
|
@@ -41,39 +40,19 @@ public static int Run(ParseResult parseResult) | |
| VSTestTrace.SafeWriteTrace(() => $"Argument list: '{commandLineParameters}'"); | ||
| } | ||
|
|
||
| (args, string[] settings) = SeparateSettingsFromArgs(args); | ||
|
|
||
| // Fix for https://github.com/Microsoft/vstest/issues/1453 | ||
| // Run dll/exe directly using the VSTestForwardingApp | ||
| // Note: ContainsBuiltTestSources need to know how many settings are there, to skip those from unmatched tokens | ||
| // When we don't have settings, we pass 0. | ||
|
Comment on lines
43
to
49
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot why was this removed? this arg parsing is necessary for the TestCommand. |
||
| // When we have settings, we want to exclude the '--' as it doesn't end up in unmatched tokens, so we pass settings.Length - 1 | ||
| if (ContainsBuiltTestSources(parseResult, GetSettingsCount(settings))) | ||
| { | ||
| return ForwardToVSTestConsole(parseResult, args, settings, testSessionCorrelationId); | ||
| } | ||
|
|
||
| return ForwardToMsbuild(parseResult, settings, testSessionCorrelationId); | ||
| } | ||
|
|
||
| internal /*internal for testing*/ static (string[] Args, string[] Settings) SeparateSettingsFromArgs(string[] args) | ||
| { | ||
| // settings parameters are after -- (including --), these should not be considered by the parser | ||
| string[] settings = [.. args.SkipWhile(a => a != "--")]; | ||
| // all parameters before -- | ||
| args = [.. args.TakeWhile(a => a != "--")]; | ||
| return (args, settings); | ||
| } | ||
|
|
||
| internal /*internal for testing*/ static int GetSettingsCount(string[] settings) | ||
| { | ||
| if (settings.Length == 0) | ||
| // Fix for https://github.com/Microsoft/vstest/issues/1453 | ||
| // Run dll/exe directly using the VSTestForwardingApp | ||
| if (ContainsBuiltTestSources(args)) | ||
| { | ||
| return 0; | ||
| return ForwardToVSTestConsole(parseResult, args, settings, testSessionCorrelationId); | ||
| } | ||
|
|
||
| Debug.Assert(settings[0] == "--", "Settings should start with --"); | ||
| return settings.Length - 1; | ||
| return ForwardToMsbuild(parseResult, settings, testSessionCorrelationId); | ||
| } | ||
|
|
||
| private static int ForwardToMsbuild(ParseResult parseResult, string[] settings, string testSessionCorrelationId) | ||
|
|
@@ -175,7 +154,7 @@ private static int ForwardToVSTestConsole(ParseResult parseResult, string[] args | |
|
|
||
| public static TestCommand FromArgs(string[] args, string? testSessionCorrelationId = null, string? msbuildPath = null) | ||
| { | ||
| var parseResult = Parser.Parse(["dotnet", "test", .. args]); | ||
| var parseResult = Parser.Parse(["dotnet", "test", ..args]); | ||
|
|
||
| // settings parameters are after -- (including --), these should not be considered by the parser | ||
| string[] settings = [.. args.SkipWhile(a => a != "--")]; | ||
|
|
@@ -258,10 +237,9 @@ private static TestCommand FromParseResult(ParseResult result, string[] settings | |
| } | ||
| } | ||
|
|
||
|
|
||
| Dictionary<string, string> variables = VSTestForwardingApp.GetVSTestRootVariables(); | ||
| foreach (var (rootVariableName, rootValue) in variables) | ||
| { | ||
| foreach (var (rootVariableName, rootValue) in variables) { | ||
| testCommand.EnvironmentVariable(rootVariableName, rootValue); | ||
| VSTestTrace.SafeWriteTrace(() => $"Root variable set {rootVariableName}:{rootValue}"); | ||
| } | ||
|
|
@@ -315,14 +293,18 @@ internal static int RunArtifactPostProcessingIfNeeded(string testSessionCorrelat | |
| } | ||
| } | ||
|
|
||
| internal /*internal for testing*/ static bool ContainsBuiltTestSources(ParseResult parseResult, int settingsLength) | ||
| private static bool ContainsBuiltTestSources(string[] args) | ||
| { | ||
| for (int i = 0; i < parseResult.UnmatchedTokens.Count - settingsLength; i++) | ||
| for (int i = 0; i < args.Length; i++) | ||
| { | ||
| string arg = parseResult.UnmatchedTokens[i]; | ||
| if (!arg.StartsWith("-") && | ||
| (arg.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) || arg.EndsWith(".exe", StringComparison.OrdinalIgnoreCase))) | ||
| string arg = args[i]; | ||
| if (arg.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) || arg.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var previousArg = i > 0 ? args[i - 1] : null; | ||
| if (previousArg != null && CommonOptions.PropertiesOption.Aliases.Contains(previousArg)) | ||
| { | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.