Reduce the amount of telemetry emitted #11094
Conversation
phil-allen-msft
force-pushed
the
dev/phil-allen-msft/telemThreshold
branch
from
0958a31
to
cfad35a
Compare
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Telemetry/TelemetryThresholds.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Telemetry/TelemetryThresholds.cs
Outdated
Show resolved
Hide resolved
...udio.LanguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_Diagnostics.cs
Outdated
Show resolved
Hide resolved
...soft.VisualStudio.LanguageServices.Razor.Test/LanguageClient/RazorCustomMessageTargetTest.cs
Outdated
Show resolved
Hide resolved
| @@ -11,9 +11,9 @@ internal interface ITelemetryReporter | |||
| TelemetryScope BeginBlock(string name, Severity severity, Property property); | |||
| TelemetryScope BeginBlock(string name, Severity severity, Property property1, Property property2); | |||
| TelemetryScope BeginBlock(string name, Severity severity, Property property1, Property property2, Property property3); | |||
| TelemetryScope BeginBlock(string name, Severity severity, params Property[] properties); | |||
There was a problem hiding this comment.
❗ The purpose of the overloads above is to avoid allocating a params array in cases where there are zero to three properties. This change will cause any calls that now pass a TimeSpan and zero to three properties to create an extra array: one for the params array, and one for the final telemetry array of length + . I recommend adding TimeSpan to each of these overloads and then adding extension methods on ITelemetryReporter that don't have the TimeSpan parameter and just pass TimeSpan.Zero.
Also, consider turning the final overload into params ReadOnlySpan<Property> to match ReportEvent below.
There was a problem hiding this comment.
I think minTimeToReport seems weird on BeginBlock. Maybe it would be better to add something to TelemetryScope for this?
There was a problem hiding this comment.
You're right. It does feel a bit strange on a method called BeginBlock. However, I think it would be awkward ergonomically to have some property to set on TelemetryScope. There are two reasons for this:
- Nothing actually uses a
TelemetryScopetoday. It's always captured in a using statement. - There's a race condition between the
TelemetryScopebeing created and the value being set. So, passing it as a parameter so that it can be included in theTelemetryScopeis probably the right thing.
Of course, the truth is that BeginBlock is kind of a weird name because since it produces a TelemetryScope. Also, TelemetryScope is currently a class. Should it become a struct with [NonCopyable] to avoid the extra allocation?
There was a problem hiding this comment.
Nothing actually uses a TelemetryScope today. It's always captured in a using statement.
But they could. Scopes probably should have a cancel anyways...
I like the idea of them being a [NonCopyable] struct. I still find it odd to add a timespan to every BeginBlock when only TrackLspRequest is using it. It seems like we're taking an implementation of one part of the API surface and expanding it.
In the end it's not a huge deal though, and with the added extension methods it won't cause any real pain
There was a problem hiding this comment.
I still find it odd to add a timespan to every
BeginBlockwhen onlyTrackLspRequestis using it.
The reason that there're five overloads of BeginBlock is really for performance. That can definitely be improved, and I'm happy to take a look at it. IMO, BeginBlock is the core API that an ITelemetryReporter should implement and the TimeSpan is a feature of that API, so it should go there. However, we should work out some of the ergonomic problems that you've pointed out. Although, there aren't actually any callers of BeginBlock other than TrackLspRequest and unit tests today.
There was a problem hiding this comment.
But they could. Scopes probably should have a cancel anyways...
Maybe? At the moment that's kind of YAGNI isn't it?
...r/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Telemetry/ITelemetryReporterExtensions.cs
Outdated
Show resolved
Hide resolved
…or is used, _reporter is null so no "report" happens on Dispose. If the _reporter check were removed from Dispose, multiple unittests would fail.
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Telemetry/TelemetryScope.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.AspNetCore.Razor.Telemetry; | ||
|
|
||
| internal sealed class TelemetryScope : IDisposable | ||
| [NonCopyable] | ||
| internal struct TelemetryScope : IDisposable | ||
| { | ||
| public static readonly TelemetryScope Null = new(); |
There was a problem hiding this comment.
Now that TelemetryScope is a struct, this is an unnecessary affordance. Plus, there's no such thing as "Null" for a struct. 😄 Please remove these and update all usages to just refer to default.
Reduce the amount of telemetry emitted
The beginLspRequest event is removed, and the trackLspRequest event is only emitted if the amount of time is over a certain threshold. This allows analysis of cases in which the amount of taken is significantly over expectations while reducing the total telemetry significantly.