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.

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

Add Razor language server telemetry to C# DevKit #9283

Merged
merged 19 commits into from
Sep 22, 2023

Conversation

allisonchou
Copy link
Contributor

@allisonchou allisonchou commented Sep 14, 2023

vscode-csharp PR: dotnet/vscode-csharp#6371

Adds Razor server telemetry to C# DevKit. The flow is as follows:

  1. This PR adds a new project, Microsoft.VisualStudio.DevKit.Razor, which contains our VS telemetry logic.
  2. The above project is published to our CDN (similar to how we publish rzls today).
  3. On the vscode-csharp side, the new bits are downloaded only if the user has C# DevKit installed. The bits are passed back into our language server so that the server has a functional telemetry reporter.

@@ -8,8 +8,8 @@

<ItemGroup>
<!-- Prepare for _UpdatePublishItems target. -->
<_ItemsToPublish Include="$(ArtifactsPackagesDir)**\*.tgz" Condition="'$(OS)' == 'Windows_NT'" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is leftover from O# and is no longer needed

@@ -41,4 +41,35 @@
<!-- Build a .zip file from each platform's directory and write it to 'artifacts' -->
<ZipDirectory SourceDirectory="%(LanguageServiceBinary.SourceDir)" DestinationFile="%(LanguageServiceBinary.ZipFile)" />
</Target>

<Target Name="_ZipDevKitTelemetryBinaries" AfterTargets="_ZipLanguageServerBinaries" Condition="'$(ArcadeBuildFromSource)' != 'true'">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We zip the files to publish to the CDN so we can consume the telemetry binaries on the vscode-csharp side.

@@ -0,0 +1,176 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is almost entirely taken from Roslyn with a few modifications to make it work for Razor

@@ -0,0 +1,96 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is almost entirely taken from Roslyn with a few modifications to make it work for Razor

@@ -0,0 +1,72 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class is almost entirely taken from Roslyn with a few modifications to make it work for Razor

using System.Diagnostics;
using Microsoft.VisualStudio.Telemetry;

namespace Microsoft.AspNetCore.Razor.Telemetry;
Copy link
Contributor Author

@allisonchou allisonchou Sep 15, 2023

Choose a reason for hiding this comment

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

These were classes previously in TelemetryReporter that I refactored out so they could be used by both VSTelemetryReporter and DevKitTelemetryReporter

@allisonchou allisonchou changed the title [draft] add devkit telemetry Add Razor language server telemetry to C# DevKit Sep 15, 2023
@allisonchou allisonchou marked this pull request as ready for review September 15, 2023 01:39
@allisonchou allisonchou requested review from a team as code owners September 15, 2023 01:39
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

I don't have a lot of context, but this makes sense to me. Well, except for the MEF and ALC stuff :)

Razor.sln Show resolved Hide resolved
@@ -1,318 +0,0 @@
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class became VSTelemetryReporter, and mutual logic between it and DevKitTelemetryReporter was moved to an abstract class called TelemetryReporter


public override void InitializeSession(string telemetryLevel, string? sessionId, bool isDefaultSession)
{
// We don't need to do anything here. We're using the default session
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably throw it at least assert this isn't called

@allisonchou allisonchou enabled auto-merge (squash) September 22, 2023 20:28
@allisonchou allisonchou merged commit 9ff3387 into main Sep 22, 2023
12 checks passed
@allisonchou allisonchou deleted the dev/allichou/AddTelemetryDevKit branch September 22, 2023 22:01
@ghost ghost added this to the Next milestone Sep 22, 2023
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants