-
Notifications
You must be signed in to change notification settings - Fork 199
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 2 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
16 changes: 16 additions & 0 deletions
16
...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,16 @@ | ||
// 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 | ||
internal static TelemetryScope BeginBlock(this ITelemetryReporter self, string name, Severity severity) { return self.BeginBlock(name, severity, TimeSpan.Zero); } | ||
internal static TelemetryScope BeginBlock(this ITelemetryReporter self, string name, Severity severity, Property property) { return self.BeginBlock(name, severity, TimeSpan.Zero, property); } | ||
internal static TelemetryScope BeginBlock(this ITelemetryReporter self, string name, Severity severity, Property property1, Property property2) { return self.BeginBlock(name, severity, TimeSpan.Zero, property1, property2); } | ||
internal static TelemetryScope BeginBlock(this ITelemetryReporter self, string name, Severity severity, Property property1, Property property2, Property property3) { return self.BeginBlock(name, severity, TimeSpan.Zero, property1, property2 , property3); } | ||
internal static TelemetryScope BeginBlock(this ITelemetryReporter self, string name, Severity severity, params ReadOnlySpan<Property> properties) { return self.BeginBlock(name, severity, TimeSpan.Zero, properties); } | ||
phil-allen-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
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
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
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?