-
Notifications
You must be signed in to change notification settings - Fork 196
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
Reduce the amount of telemetry emitted #11094
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
cfad35a
Reduce the amount of telemetry emitted
phil-allen-msft c4576c9
Accept code review feedback
phil-allen-msft 3aa876b
Accept code review feedback (2)
phil-allen-msft b5703f9
Change TelemetryScope to NonCopyable struct; if the default construct…
phil-allen-msft c021a55
Undo telemetry-scope-as-struct changes
phil-allen-msft File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 changes: 25 additions & 0 deletions
25
...rc/Microsoft.AspNetCore.Razor.ProjectEngineHost/Telemetry/ITelemetryReporterExtensions.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.AspNetCore.Razor.Telemetry; | ||
|
||
internal static class ITelemetryReporterExtensions | ||
{ | ||
// These extensions effectively make TimeSpan an optional parameter on BeginBlock | ||
public static TelemetryScope BeginBlock(this ITelemetryReporter reporter, string name, Severity severity) | ||
=> reporter.BeginBlock(name, severity, minTimeToReport: TimeSpan.Zero); | ||
|
||
public static TelemetryScope BeginBlock(this ITelemetryReporter reporter, string name, Severity severity, Property property) | ||
=> reporter.BeginBlock(name, severity, minTimeToReport: TimeSpan.Zero, property); | ||
|
||
public static TelemetryScope BeginBlock(this ITelemetryReporter reporter, string name, Severity severity, Property property1, Property property2) | ||
=> reporter.BeginBlock(name, severity, minTimeToReport: TimeSpan.Zero, property1, property2); | ||
|
||
public static TelemetryScope BeginBlock(this ITelemetryReporter reporter, string name, Severity severity, Property property1, Property property2, Property property3) | ||
=> reporter.BeginBlock(name, severity, minTimeToReport: TimeSpan.Zero, property1, property2, property3); | ||
|
||
public static TelemetryScope BeginBlock(this ITelemetryReporter reporter, string name, Severity severity, params ReadOnlySpan<Property> properties) | ||
=> reporter.BeginBlock(name, severity, minTimeToReport: TimeSpan.Zero, properties); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,12 @@ | |
using System; | ||
using System.Diagnostics; | ||
using Microsoft.AspNetCore.Razor.PooledObjects; | ||
using Microsoft.AspNetCore.Razor.Utilities; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that |
||
|
||
|
@@ -16,11 +18,12 @@ internal sealed class TelemetryScope : IDisposable | |
private readonly Severity _severity; | ||
private readonly Property[] _properties; | ||
private readonly Stopwatch _stopwatch; | ||
private readonly TimeSpan _minTimeToReport; | ||
private bool _disposed; | ||
|
||
private TelemetryScope() | ||
public TelemetryScope() | ||
phil-allen-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
// This constructor is only called to initialize the Null instance | ||
// This constructor should be called only to initialize the Null instance | ||
// above. Rather than make _name, _properties, and _stopwatch | ||
// nullable, we use a ! to initialize them to null for this case only. | ||
_reporter = null; | ||
|
@@ -32,6 +35,7 @@ private TelemetryScope() | |
private TelemetryScope( | ||
ITelemetryReporter reporter, | ||
string name, | ||
TimeSpan minTimeToReport, | ||
Severity severity, | ||
Property[] properties) | ||
{ | ||
|
@@ -45,6 +49,7 @@ private TelemetryScope( | |
_properties = properties; | ||
|
||
_stopwatch = StopwatchPool.Default.Get(); | ||
_minTimeToReport = minTimeToReport; | ||
_stopwatch.Restart(); | ||
} | ||
|
||
|
@@ -59,54 +64,57 @@ public void Dispose() | |
|
||
_stopwatch.Stop(); | ||
|
||
// We know that we were created with an array of at least length one. | ||
_properties[^1] = new("eventscope.ellapsedms", _stopwatch.ElapsedMilliseconds); | ||
var elapsed = _stopwatch.Elapsed; | ||
if (elapsed >= _minTimeToReport) | ||
{ | ||
// We know that we were created with an array of at least length one. | ||
_properties[^1] = new("eventscope.ellapsedms", _stopwatch.ElapsedMilliseconds); | ||
|
||
_reporter.ReportEvent(_name, _severity, _properties); | ||
_reporter.ReportEvent(_name, _severity, _properties); | ||
} | ||
|
||
StopwatchPool.Default.Return(_stopwatch); | ||
} | ||
|
||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity) | ||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, TimeSpan minTimeToReport) | ||
{ | ||
var array = new Property[1]; | ||
|
||
return new(reporter, name, severity, array); | ||
return new(reporter, name, minTimeToReport, severity, array); | ||
} | ||
|
||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, Property property) | ||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, TimeSpan minTimeToReport, Property property) | ||
{ | ||
var array = new Property[2]; | ||
array[0] = property; | ||
|
||
return new(reporter, name, severity, array); | ||
return new(reporter, name, minTimeToReport, severity, array); | ||
} | ||
|
||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, Property property1, Property property2) | ||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, TimeSpan minTimeToReport, Property property1, Property property2) | ||
{ | ||
var array = new Property[3]; | ||
array[0] = property1; | ||
array[1] = property2; | ||
|
||
return new(reporter, name, severity, array); | ||
return new(reporter, name, minTimeToReport, severity, array); | ||
} | ||
|
||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, Property property1, Property property2, Property property3) | ||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, TimeSpan minTimeToReport, Property property1, Property property2, Property property3) | ||
{ | ||
var array = new Property[4]; | ||
array[0] = property1; | ||
array[1] = property2; | ||
array[2] = property3; | ||
|
||
return new(reporter, name, severity, array); | ||
return new(reporter, name, minTimeToReport, severity, array); | ||
} | ||
|
||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, Property[] properties) | ||
public static TelemetryScope Create(ITelemetryReporter reporter, string name, Severity severity, TimeSpan minTimeToReport, ReadOnlySpan<Property> properties) | ||
{ | ||
var array = new Property[properties.Length + 1]; | ||
|
||
Array.Copy(properties, array, properties.Length); | ||
|
||
return new(reporter, name, severity, array); | ||
properties.CopyTo(array); | ||
|
||
return new(reporter, name, minTimeToReport, severity, array); | ||
} | ||
} |
28 changes: 28 additions & 0 deletions
28
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Telemetry/TelemetryThresholds.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.CodeAnalysis.Razor.Workspaces.Telemetry; | ||
|
||
/// <summary> | ||
/// A set of constants used to reduce the telemetry emitted to the set that help us understand | ||
/// which LSP is taking the most time in the case that the overall call is lengthy. | ||
/// </summary> | ||
internal static class TelemetryThresholds | ||
{ | ||
internal static readonly TimeSpan CodeActionRazorTelemetryThreshold = TimeSpan.FromMilliseconds(2000); | ||
internal static readonly TimeSpan CodeActionSubLSPTelemetryThreshold = TimeSpan.FromMilliseconds(1000); | ||
|
||
internal static readonly TimeSpan CompletionRazorTelemetryThreshold = TimeSpan.FromMilliseconds(4000); | ||
internal static readonly TimeSpan CompletionSubLSPTelemetryThreshold = TimeSpan.FromMilliseconds(2000); | ||
|
||
internal static readonly TimeSpan DiagnosticsRazorTelemetryThreshold = TimeSpan.FromMilliseconds(4000); | ||
internal static readonly TimeSpan DiagnosticsSubLSPTelemetryThreshold = TimeSpan.FromMilliseconds(2000); | ||
|
||
internal static readonly TimeSpan MapCodeRazorTelemetryThreshold = TimeSpan.FromMilliseconds(2000); | ||
internal static readonly TimeSpan MapCodeSubLSPTelemetryThreshold = TimeSpan.FromMilliseconds(1000); | ||
|
||
internal static readonly TimeSpan SemanticTokensRazorTelemetryThreshold = TimeSpan.FromMilliseconds(2000); | ||
internal static readonly TimeSpan SemanticTokensSubLSPTelemetryThreshold = TimeSpan.FromMilliseconds(1000); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗ 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 addingTimeSpan
to each of these overloads and then adding extension methods onITelemetryReporter
that don't have theTimeSpan
parameter and just passTimeSpan.Zero
.Also, consider turning the final overload into
params ReadOnlySpan<Property>
to matchReportEvent
below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
minTimeToReport
seems weird on BeginBlock. Maybe it would be better to add something toTelemetryScope
for this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onTelemetryScope
. There are two reasons for this:TelemetryScope
today. It's always captured in a using statement.TelemetryScope
being created and the value being set. So, passing it as a parameter so that it can be included in theTelemetryScope
is probably the right thing.Of course, the truth is that
BeginBlock
is kind of a weird name because since it produces aTelemetryScope
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 everyBeginBlock
when onlyTrackLspRequest
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anITelemetryReporter
should implement and theTimeSpan
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 ofBeginBlock
other thanTrackLspRequest
and unit tests today.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? At the moment that's kind of YAGNI isn't it?