From 1a8011d66164ef274c8063cfee35b74bd840d1a1 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 13 Sep 2023 13:37:04 -0700 Subject: [PATCH] Remove unused telemetry While this implementation is useful, we are not currently doing anything with said telemetry and therefore should not be sending it. We can bring it back by reference this commit. --- .../Handlers/ConfigurationHandler.cs | 62 ------------------- .../LanguageServerProtocolMessageTests.cs | 61 +++++++----------- 2 files changed, 21 insertions(+), 102 deletions(-) diff --git a/src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs b/src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs index 230a82d58..a7f36aab1 100644 --- a/src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs +++ b/src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs @@ -7,13 +7,9 @@ using System.Threading.Tasks; using MediatR; using Microsoft.Extensions.Logging; -using Microsoft.PowerShell.EditorServices.Logging; using Microsoft.PowerShell.EditorServices.Services; using Microsoft.PowerShell.EditorServices.Services.Configuration; -using Newtonsoft.Json.Linq; using OmniSharp.Extensions.LanguageServer.Protocol.Models; -using OmniSharp.Extensions.LanguageServer.Protocol.Server; -using OmniSharp.Extensions.LanguageServer.Protocol.Window; using OmniSharp.Extensions.LanguageServer.Protocol.Workspace; namespace Microsoft.PowerShell.EditorServices.Handlers @@ -23,19 +19,16 @@ internal class PsesConfigurationHandler : DidChangeConfigurationHandlerBase private readonly ILogger _logger; private readonly WorkspaceService _workspaceService; private readonly ConfigurationService _configurationService; - private readonly ILanguageServerFacade _languageServer; public PsesConfigurationHandler( ILoggerFactory factory, WorkspaceService workspaceService, AnalysisService analysisService, ConfigurationService configurationService, - ILanguageServerFacade languageServer, SymbolsService symbolsService) { _logger = factory.CreateLogger(); _workspaceService = workspaceService; _configurationService = configurationService; - _languageServer = languageServer; ConfigurationUpdated += analysisService.OnConfigurationUpdated; ConfigurationUpdated += symbolsService.OnConfigurationUpdated; @@ -51,8 +44,6 @@ public override async Task Handle(DidChangeConfigurationParams request, Ca return await Unit.Task.ConfigureAwait(false); } - SendFeatureChangesTelemetry(incomingSettings); - bool oldScriptAnalysisEnabled = _configurationService.CurrentSettings.ScriptAnalysis.Enable; string oldScriptAnalysisSettingsPath = _configurationService.CurrentSettings.ScriptAnalysis?.SettingsPath; @@ -109,59 +100,6 @@ public override async Task Handle(DidChangeConfigurationParams request, Ca return await Unit.Task.ConfigureAwait(false); } - private void SendFeatureChangesTelemetry(LanguageServerSettingsWrapper incomingSettings) - { - if (incomingSettings is null) - { - _logger.LogTrace("Incoming settings were null"); - return; - } - - Dictionary configChanges = new(); - // Send telemetry if the user opted-out of ScriptAnalysis - if (!incomingSettings.Powershell.ScriptAnalysis.Enable && - _configurationService.CurrentSettings.ScriptAnalysis.Enable != incomingSettings.Powershell.ScriptAnalysis.Enable) - { - configChanges["ScriptAnalysis"] = incomingSettings.Powershell.ScriptAnalysis.Enable; - } - - // Send telemetry if the user opted-out of CodeFolding - if (!incomingSettings.Powershell.CodeFolding.Enable && - _configurationService.CurrentSettings.CodeFolding.Enable != incomingSettings.Powershell.CodeFolding.Enable) - { - configChanges["CodeFolding"] = incomingSettings.Powershell.CodeFolding.Enable; - } - - // Send telemetry if the user opted-out of Profile loading - if (!incomingSettings.Powershell.EnableProfileLoading && - _configurationService.CurrentSettings.EnableProfileLoading != incomingSettings.Powershell.EnableProfileLoading) - { - configChanges["ProfileLoading"] = incomingSettings.Powershell.EnableProfileLoading; - } - - // Send telemetry if the user opted-in to Pester 5+ CodeLens - if (!incomingSettings.Powershell.Pester.UseLegacyCodeLens && - _configurationService.CurrentSettings.Pester.UseLegacyCodeLens != incomingSettings.Powershell.Pester.UseLegacyCodeLens) - { - // From our perspective we want to see how many people are opting in to this so we flip the value - configChanges["Pester5CodeLens"] = !incomingSettings.Powershell.Pester.UseLegacyCodeLens; - } - - // No need to send any telemetry since nothing changed - if (configChanges.Count == 0) - { - return; - } - - _languageServer.Window.SendTelemetryEvent(new TelemetryEventParams - { - ExtensionData = new PsesTelemetryEvent - { - EventName = "NonDefaultPsesFeatureConfiguration", - Data = JObject.FromObject(configChanges) - } - }); - } public event EventHandler ConfigurationUpdated; } diff --git a/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs b/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs index fd4854f04..1a227278d 100644 --- a/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs +++ b/test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs @@ -12,7 +12,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.PowerShell.EditorServices.Handlers; -using Microsoft.PowerShell.EditorServices.Logging; using Microsoft.PowerShell.EditorServices.Services.Configuration; using Microsoft.PowerShell.EditorServices.Services.PowerShell; using Microsoft.PowerShell.EditorServices.Services.Template; @@ -40,7 +39,6 @@ public class LanguageServerProtocolMessageTests : IClassFixture private readonly ILanguageClient PsesLanguageClient; private readonly List Messages; private readonly List Diagnostics; - private readonly List TelemetryEvents; private readonly string PwshExe; public LanguageServerProtocolMessageTests(ITestOutputHelper output, LSPTestsFixture data) @@ -51,15 +49,12 @@ public LanguageServerProtocolMessageTests(ITestOutputHelper output, LSPTestsFixt Messages.Clear(); Diagnostics = data.Diagnostics; Diagnostics.Clear(); - TelemetryEvents = data.TelemetryEvents; - TelemetryEvents.Clear(); PwshExe = PsesStdioProcess.PwshExe; } public void Dispose() { Diagnostics.Clear(); - TelemetryEvents.Clear(); GC.SuppressFinalize(this); } @@ -91,26 +86,12 @@ private async Task WaitForDiagnosticsAsync() // Wait for PSSA to finish. for (int i = 0; Diagnostics.Count == 0; i++) { - if (i >= 10) + if (i >= 30) { throw new InvalidDataException("No diagnostics showed up after 20s."); } - await Task.Delay(2000).ConfigureAwait(true); - } - } - - private async Task WaitForTelemetryEventsAsync() - { - // Wait for PSSA to finish. - for (int i = 0; TelemetryEvents.Count == 0; i++) - { - if (i >= 10) - { - throw new InvalidDataException("No telemetry events showed up after 20s."); - } - - await Task.Delay(2000).ConfigureAwait(true); + await Task.Delay(1000).ConfigureAwait(true); } } @@ -233,12 +214,6 @@ public async Task CanReceiveDiagnosticsFromConfigurationChangeAsync() Skip.If(PsesStdioProcess.RunningInConstrainedLanguageMode && PsesStdioProcess.IsWindowsPowerShell, "Windows PowerShell doesn't trust PSScriptAnalyzer by default so it won't load."); - NewTestFile("gci | % { $_ }"); - await WaitForDiagnosticsAsync().ConfigureAwait(true); - - // NewTestFile doesn't clear diagnostic notifications so we need to do that for this test. - Diagnostics.Clear(); - PsesLanguageClient.SendNotification("workspace/didChangeConfiguration", new DidChangeConfigurationParams { @@ -256,18 +231,12 @@ public async Task CanReceiveDiagnosticsFromConfigurationChangeAsync() }) }); - await WaitForTelemetryEventsAsync().ConfigureAwait(true); - PsesTelemetryEvent telemetryEvent = Assert.Single(TelemetryEvents); - Assert.Equal("NonDefaultPsesFeatureConfiguration", telemetryEvent.EventName); - Assert.False((bool)telemetryEvent.Data.GetValue("ScriptAnalysis")); + string filePath = NewTestFile("$a = 4"); - // We also shouldn't get any Diagnostics because ScriptAnalysis is disabled. + // Wait a bit to make sure no diagnostics came through + await Task.Delay(2000).ConfigureAwait(true); Assert.Empty(Diagnostics); - // Clear telemetry events so we can test to make sure telemetry doesn't - // come through with default settings. - TelemetryEvents.Clear(); - // Restore default configuration PsesLanguageClient.SendNotification("workspace/didChangeConfiguration", new DidChangeConfigurationParams @@ -280,10 +249,22 @@ public async Task CanReceiveDiagnosticsFromConfigurationChangeAsync() }) }); - // Wait a bit to make sure no telemetry events came through - await Task.Delay(2000).ConfigureAwait(true); - // Since we have default settings we should not get any telemetry events about - Assert.Empty(TelemetryEvents.Where(e => e.EventName == "NonDefaultPsesFeatureConfiguration")); + // That notification does not trigger re-analyzing open files. For that we have to send + // a textDocument/didChange notification. + PsesLanguageClient.SendNotification("textDocument/didChange", new DidChangeTextDocumentParams + { + ContentChanges = new Container(), + TextDocument = new OptionalVersionedTextDocumentIdentifier + { + Version = 4, + Uri = new Uri(filePath) + } + }); + + await WaitForDiagnosticsAsync().ConfigureAwait(true); + + Diagnostic diagnostic = Assert.Single(Diagnostics); + Assert.Equal("PSUseDeclaredVarsMoreThanAssignments", diagnostic.Code); } [Fact]