Skip to content

Commit

Permalink
Remove unused telemetry
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@andyleejordan
andyleejordan committed Sep 13, 2023
1 parent 0fcb641 commit 515deab
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 102 deletions.
62 changes: 0 additions & 62 deletions src/PowerShellEditorServices/Services/Workspace/Handlers/ConfigurationHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<PsesConfigurationHandler>();
_workspaceService = workspaceService;
_configurationService = configurationService;
_languageServer = languageServer;

ConfigurationUpdated += analysisService.OnConfigurationUpdated;
ConfigurationUpdated += symbolsService.OnConfigurationUpdated;
Expand All @@ -51,8 +44,6 @@ public override async Task<Unit> Handle(DidChangeConfigurationParams request, Ca
return await Unit.Task.ConfigureAwait(false);
}

SendFeatureChangesTelemetry(incomingSettings);

bool oldScriptAnalysisEnabled = _configurationService.CurrentSettings.ScriptAnalysis.Enable;
string oldScriptAnalysisSettingsPath = _configurationService.CurrentSettings.ScriptAnalysis?.SettingsPath;

Expand Down Expand Up @@ -109,59 +100,6 @@ public override async Task<Unit> 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<string, bool> 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<LanguageServerSettings> ConfigurationUpdated;
}
Expand Down
61 changes: 21 additions & 40 deletions test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -40,7 +39,6 @@ public class LanguageServerProtocolMessageTests : IClassFixture<LSPTestsFixture>
private readonly ILanguageClient PsesLanguageClient;
private readonly List<LogMessageParams> Messages;
private readonly List<Diagnostic> Diagnostics;
private readonly List<PsesTelemetryEvent> TelemetryEvents;
private readonly string PwshExe;

public LanguageServerProtocolMessageTests(ITestOutputHelper output, LSPTestsFixture data)
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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
{
Expand All @@ -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
Expand All @@ -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<TextDocumentContentChangeEvent>(),
TextDocument = new OptionalVersionedTextDocumentIdentifier
{
Version = 4,
Uri = new Uri(filePath)
}
});

await WaitForDiagnosticsAsync().ConfigureAwait(true);

Diagnostic diagnostic = Assert.Single(Diagnostics);
Assert.Equal("PSUseDeclaredVarsMoreThanAssignments", diagnostic.Code);
}

[Fact]
Expand Down

0 comments on commit 515deab

Please sign in to comment.