Add CLaSP Request Tracking #11015

ryzngard · 2024-10-12T00:39:11Z

This adds the request tracking available for implementation in CLaSP for Razor.

TODO:

Add tests for new aggregate histograms
Double check telemetry disposal in VS and VS Code

ryzngard · 2024-10-12T00:40:33Z

...Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/AggregatingTelemetryLog.cs

+/// of this class corresponds to a specific FunctionId operation and can support aggregated values for each
+/// metric name logged.
+/// </summary>
+internal sealed class AggregatingTelemetryLog


This is taken from Roslyn and adapted to how we do telemetry

https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/Core/Def/Telemetry/Shared/AggregatingTelemetryLog.cs

Should this be a comment in the code so that we can refer back to the original implementation later on?

I can add a comment with a permalink. The implementation itself is fairly straight forward so I wasn't sure it was needed, but I wanted to give context on where I got it for the PR.

For sure, I just wondered if we wanted to capture the place to go look if we have questions or to see any updates.

ryzngard · 2024-10-12T00:40:59Z

...rc/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/AggregatingTelemetryLogManager.cs

+/// <summary>
+/// Manages creation and obtaining aggregated telemetry logs.
+/// </summary>
+internal sealed class AggregatingTelemetryLogManager : IDisposable


Taken from https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/Core/Def/Telemetry/Shared/AggregatingTelemetryLogManager.cs

@dibarbet has a PR out removing the roslyn version of this class. You might want to coordinate with him before duplicating it.

dotnet/roslyn#75402

Same question as before. Is this better placed in the code to make that connection with the Roslyn code a bit more permanent? If so, please use a permanent link in case the Roslyn file gets moved.

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/TelemetryReporter.cs

davidwengier · 2024-10-15T02:41:33Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CLaSPTelemetryService.cs

+        public override void RecordWarning(string message)
+        {
+            _exception = new Exception(message);
+            _result = TelemetryResult.Failed;


Whats the scenario for this? It's a little odd for "Warning" and "Failed" to be mixed. They don't mean the same thing to me. Should this be RecordFailure or RecordError? Even perhaps RecordException, but its just the convenience overload you can call if you just have a string.

I agree. Capturing an exception for a warning and marking it as "failed" is surprising to me.

I think I'm going to remove the exception entirely. It's provided, but we don't record it in a normal histogram related way. It should get caught through fault reporting through

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CLaSPTelemetryService.cs

DustinCampbell · 2024-10-15T15:28:28Z

...Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/AggregatingTelemetryLog.cs

+/// of this class corresponds to a specific FunctionId operation and can support aggregated values for each
+/// metric name logged.
+/// </summary>
+internal sealed class AggregatingTelemetryLog


Should this be a comment in the code so that we can refer back to the original implementation later on?

DustinCampbell · 2024-10-15T15:30:12Z

...rc/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/AggregatingTelemetryLogManager.cs

+/// <summary>
+/// Manages creation and obtaining aggregated telemetry logs.
+/// </summary>
+internal sealed class AggregatingTelemetryLogManager : IDisposable


Same question as before. Is this better placed in the code to make that connection with the Roslyn code a bit more permanent? If so, please use a permanent link in case the Roslyn file gets moved.

...rc/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/AggregatingTelemetryLogManager.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/TelemetryReporter.cs

DustinCampbell · 2024-10-15T16:24:40Z

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Telemetry/ITelemetryReporter.cs

@@ -5,7 +5,7 @@

 namespace Microsoft.AspNetCore.Razor.Telemetry;

-internal interface ITelemetryReporter
+internal interface ITelemetryReporter : IDisposable


Does this interface need to be IDisposable? Or, is it sufficient to mark TelemetryReporter as IDisposable? AFAICT, this isn't needed by the ITelemetryReporter contract itself and results in a do-nothing Dispose() on NoOpTelemetryReporter.

This is where my knowledge gets a little iffy. Disposal is handled by the DI implementation, and it's added as a singleton with AddSingleton<ITelemetryReporter>. Does DI know if the underlying class needs to be disposed? Do I need to add a finalizer pattern with disposal?

Yes, all the DI containers we use -- MEF, Microsoft.Extensions.DependencyInjection, VSMEF -- will dispose the runtime object if it implements IDisposable. It is not necessary for the contract interface to be IDisposable. (Actually, it would be pretty weird if that was a requirement!)

I just have blissful ignorance for MEF and VSMEF :) Will move IDisposable down

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Telemetry/ITelemetryReporter.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/TelemetryReporter.cs

DustinCampbell · 2024-10-16T19:59:14Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CLaSPTelemetryService.cs

+
+namespace Microsoft.AspNetCore.Razor.LanguageServer;
+
+internal class CLaSPTelemetryService(ITelemetryReporter telemetryReporter) : AbstractTelemetryService


I guess, consider marking this sealed too. 😄

DustinCampbell · 2024-10-16T20:03:56Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs

@@ -165,6 +165,7 @@ protected override ILspServices ConstructLspServices()
        // TelemetryService.SetDefaultSession and provides the correct
        // appinsights keys etc


Is this comment in the right place? It's not clear to me what code it's referring to.

It looks like it should just be removed

DustinCampbell

Looks good to me!

Add initial implementation for LSP request telemetry

c099354

ryzngard commented Oct 12, 2024

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/TelemetryReporter.cs Outdated Show resolved Hide resolved

ryzngard commented Oct 12, 2024

View reviewed changes

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/Telemetry/TelemetryReporter.cs Outdated Show resolved Hide resolved

ryzngard added 5 commits October 11, 2024 17:43

🤷

a18518b

Change typing and add test

cf0095f

Correctly add to MEF and other fixes

8ba99c4

Get histogram working and add counters

429b08e

#if DEBUG

404546f

ryzngard marked this pull request as ready for review October 15, 2024 00:35

ryzngard requested a review from a team as a code owner October 15, 2024 00:35

ryzngard requested review from dibarbet and ToddGrun October 15, 2024 00:35

davidwengier approved these changes Oct 15, 2024

View reviewed changes

DustinCampbell reviewed Oct 15, 2024

View reviewed changes

ryzngard mentioned this pull request Oct 15, 2024

Add telemetry output to test logs #11028

Open

PR feedback

0a5fd35

ryzngard mentioned this pull request Oct 16, 2024

Change RazorProjectInfo to use HostProject and HostDocument #11030

Closed

DustinCampbell reviewed Oct 16, 2024

View reviewed changes

sealed

26091a6

DustinCampbell reviewed Oct 16, 2024

View reviewed changes

DustinCampbell approved these changes Oct 16, 2024

View reviewed changes

Disposable and sealed

9a8d657

ryzngard enabled auto-merge (squash) October 16, 2024 20:10

ryzngard merged commit d21c9c9 into dotnet:main Oct 16, 2024
12 checks passed

ryzngard deleted the clasp_telemetry branch October 16, 2024 20:40

dotnet-policy-service bot added this to the Next milestone Oct 16, 2024

This was referenced Oct 22, 2024

[Automated] PRs inserted in VS build main-35421.179 #11056

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35424.209 #11093

Closed

dotnet-bot mentioned this pull request Oct 29, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35428.160 #11107

Closed

phil-allen-msft modified the milestones: Next, 17.13 P1 Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLaSP Request Tracking #11015

Add CLaSP Request Tracking #11015

ryzngard commented Oct 12, 2024 •

edited

Loading

ryzngard Oct 12, 2024

DustinCampbell Oct 15, 2024

ryzngard Oct 15, 2024

DustinCampbell Oct 16, 2024

ryzngard Oct 12, 2024

ToddGrun Oct 15, 2024

DustinCampbell Oct 15, 2024

davidwengier Oct 15, 2024

DustinCampbell Oct 15, 2024

ryzngard Oct 15, 2024

DustinCampbell Oct 15, 2024

DustinCampbell Oct 15, 2024

DustinCampbell Oct 15, 2024

ryzngard Oct 15, 2024

DustinCampbell Oct 16, 2024 •

edited

Loading

ryzngard Oct 16, 2024

DustinCampbell Oct 16, 2024

ryzngard Oct 16, 2024

DustinCampbell Oct 16, 2024

ryzngard Oct 16, 2024

DustinCampbell left a comment


		namespace Microsoft.AspNetCore.Razor.LanguageServer;

		internal class CLaSPTelemetryService(ITelemetryReporter telemetryReporter) : AbstractTelemetryService

		@@ -165,6 +165,7 @@ protected override ILspServices ConstructLspServices()
		// TelemetryService.SetDefaultSession and provides the correct
		// appinsights keys etc

Add CLaSP Request Tracking #11015

Add CLaSP Request Tracking #11015

Conversation

ryzngard commented Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

ryzngard commented Oct 12, 2024 •

edited

Loading

DustinCampbell Oct 16, 2024 •

edited

Loading