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.

Sign up for GitHub

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

Jump to bottom

Add fault telemetry for (hopefully) all VS APIs #3775

Merged
merged 9 commits into from
Jan 5, 2021
Merged

Add fault telemetry for (hopefully) all VS APIs #3775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
01e5d9c
Add fault telemetry for (hopefully) all VS APIs
zivkan Nov 23, 2020
7577062
Add tests
zivkan Nov 28, 2020
dc4cfa8
PostFault to wrap PostFaultAsync
zivkan Nov 28, 2020
f3b8320
Make property name generation consistent.
zivkan Dec 10, 2020
b3f8d3b
Fix formatting
zivkan Dec 11, 2020
a6fc680
PR feedback
zivkan Dec 22, 2020
59157cc
fix bad rebase
zivkan Dec 22, 2020
4d6777c
Improve VsFrameworkCompatibility GetNearest exception message
zivkan Dec 23, 2020
6d68f8d
Fix test
zivkan Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/NuGet.Clients/NuGet.Tools/NuGetBrokeredServiceFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using NuGet.VisualStudio;
using NuGet.VisualStudio.Implementation.Extensibility;
using NuGet.VisualStudio.Internal.Contracts;
using NuGet.VisualStudio.Telemetry;
using ContractsNuGetServices = NuGet.VisualStudio.Contracts.NuGetServices;
using IBrokeredServiceContainer = Microsoft.VisualStudio.Shell.ServiceBroker.IBrokeredServiceContainer;
using ISettings = NuGet.Configuration.ISettings;
Expand All @@ -29,6 +30,7 @@ internal sealed class NuGetBrokeredServiceFactory : IDisposable
private AsyncLazy<IVsSolutionManager> _lazySolutionManager;
private INuGetProjectManagerServiceState _projectManagerServiceSharedState;
private ISharedServiceState _sharedServiceState;
private AsyncLazy<INuGetTelemetryProvider> _lazyTelemetryProvider;

private NuGetBrokeredServiceFactory()
{
Expand Down Expand Up @@ -172,8 +174,9 @@ public void Dispose()

IVsSolutionManager solutionManager = await _lazySolutionManager.GetValueAsync(cancellationToken);
ISettings settings = await _lazySettings.GetValueAsync(cancellationToken);
INuGetTelemetryProvider telemetryProvider = await _lazyTelemetryProvider.GetValueAsync(cancellationToken);

return new NuGetProjectService(solutionManager, settings);
return new NuGetProjectService(solutionManager, settings, telemetryProvider);
}

private async Task InitializeAsync()
Expand All @@ -182,6 +185,7 @@ private async Task InitializeAsync()
_lazySolutionManager = new AsyncLazy<IVsSolutionManager>(ServiceLocator.GetInstanceAsync<IVsSolutionManager>, ThreadHelper.JoinableTaskFactory);
_projectManagerServiceSharedState = new NuGetProjectManagerServiceState();
_sharedServiceState = await SharedServiceState.CreateAsync(CancellationToken.None);
_lazyTelemetryProvider = new AsyncLazy<INuGetTelemetryProvider>(ServiceLocator.GetInstanceAsync<INuGetTelemetryProvider>, ThreadHelper.JoinableTaskFactory);
}
}
}
18 changes: 18 additions & 0 deletions src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/INuGetTelemetryProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using NuGet.Common;

namespace NuGet.VisualStudio.Telemetry
{
public interface INuGetTelemetryProvider
{
void EmitEvent(TelemetryEvent telemetryEvent);
Task PostFaultAsync(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null, IDictionary<string, object> extraProperties = null);
void PostFault(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null, IDictionary<string, object> extraProperties = null);
}
}
34 changes: 34 additions & 0 deletions src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/NuGetTelemetryProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.ComponentModel.Composition;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using NuGet.Common;

namespace NuGet.VisualStudio.Telemetry
{
[Export(typeof(INuGetTelemetryProvider))]
internal class NuGetTelemetryProvider : INuGetTelemetryProvider
{
public void EmitEvent(TelemetryEvent telemetryEvent)
{
TelemetryActivity.EmitTelemetryEvent(telemetryEvent);
}

public async Task PostFaultAsync(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null, IDictionary<string, object> extraProperties = null)
{
await TelemetryUtility.PostFaultAsync(e, callerClassName, callerMemberName, extraProperties);
}

public void PostFault(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null, IDictionary<string, object> extraProperties = null)
{
NuGetUIThreadHelper.JoinableTaskFactory.Run(async () =>
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
{
await PostFaultAsync(e, callerClassName, callerMemberName, extraProperties);
});
}
}
}
13 changes: 11 additions & 2 deletions src/NuGet.Clients/NuGet.VisualStudio.Common/Telemetry/TelemetryUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
Expand All @@ -19,7 +20,7 @@ namespace NuGet.VisualStudio.Telemetry
{
public static class TelemetryUtility
{
public static async Task PostFaultAsync(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null)
public static async Task PostFaultAsync(Exception e, string callerClassName, [CallerMemberName] string callerMemberName = null, IDictionary<string, object> extraProperties = null)
{
if (e == null)
{
Expand All @@ -31,8 +32,16 @@ public static async Task PostFaultAsync(Exception e, string callerClassName, [Ca

await NuGetUIThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();

var fault = new FaultEvent($"{VSTelemetrySession.VSEventNamePrefix}Fault", description, FaultSeverity.General, e, gatherEventDetails: null);
var fault = new FaultEvent(VSTelemetrySession.VSEventNamePrefix + "Fault", description, FaultSeverity.General, e, gatherEventDetails: null);
fault.Properties[$"{VSTelemetrySession.VSPropertyNamePrefix}Fault.Caller"] = caller;
if (extraProperties != null)
{
foreach (var kvp in extraProperties)
{
fault.Properties[VSTelemetrySession.VSEventNamePrefix + kvp.Key] = kvp.Value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this one same of line 36, just to keep same coding format.
fault.Properties[$"{VSTelemetrySession.VSPropertyNamePrefix}kvp.Key"] = kvp.Value;

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if the C# compiler is better now, but initially at least interpolated strings ($"... {something}" was just syntactic sugar for string.Format, and the book "Writing High Performance .NET Code" discusses how string.Format harms performance since the runtime has to parse the format string, and build up the final string. Their recommendation was to use string concatenation instead. Hence I changed line 36 to do that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realize string.Format had that perf hit. I think StringBuilder is the most perf and memory efficient, right?
I don't really prefer reading the concatenation, especially inline an indexer like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

c#10 (or maybe later) might have const interpolated strings, so in the future this may no longer be necessary. But string.Concat can produce const strings determined at compile time, rather than runtime. And at runtime, string.Concat (which the + operator is syntactic sugar for) remains to be by far the fastest execution at runtime as well.

string.Concat only takes strings as input, anything that is not a string is implicitly run though .ToString() before being passed to string.Concat, meaning that string.Concat can use args.Sum(a => a.Length) to determine the exact length of the final string, do a single memory allocation of the required length, then copy the input strings one by one.

Using a StringBuilder is the next best. It has a small allocation for the StringBuilder object iteself, and it allocates at least one buffer. As you call Append, it copies your string into the buffer, and if the buffer gets full, it needs to allocate more buffers. Finally, when you call StringBuilder.ToString(), it can do the same work as string.Concat, which as previously stated is an additional allocation of the required length. Hence, we can see that StringBuilder cannot possibly be more efficient than string.Concat, since it does everything that string.Concat does, plus more.

string.Format (which is what interpolated strings are, and also what StringBuilder's AppendFormat method is) is the least efficient. Internally, it creates a string builder (except in the StringBuilder.AppendFormat case), since it can't figure out what the final string length is going to be from the inputs alone. Therefore it can't possibly be faster than StringBuilder, since it uses a StringBuilder. As previously mentioned, Format also needs to parse the input string to find the {x} placeholders, copy the static parts, then parse the placeholder to see if there are formatting options, then call ToString on the relevant object with the discovered formatting options. string.Format and StringBuilder.AppendFormat are therefore by far the slowest at runtime. My memory is that the Writing High Performance .NET Code book claimed that using StringBuilder with a series of Append calls is faster than string.Format.

So in summary, constStr1 + constStr2 will be evaluated at compile time, so runtime does nothing more than load a const string. This should become available for interpolated const strings in the future, but is not yet available. If the strings are not const at compile time, string concatenation remains the fastest, so if you want to hyper-optimize allocations, this is the best bet. It's always best if all strings you need to concat are in a single call, but even if you use string.Concat a small number of times to generate a single string, it can still be faster than using a StringBuilder. Using (non-const) interpolated strings, or string.Format basically means you don't care about performance. Having said that, while we don't want our tools to be slow, client tools don't have the same concerns or cost savings that web services like Bing or Office web applications have.

Copy link
Contributor

Choose a reason for hiding this comment

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

string.Concat only takes strings as input

Strings that are already allocated. I thought the whole memory consumption (not perf) problem with concat was in string1 + string2, you allocate 2 strings, then you allocate a result string and garbage collect 1 and 2. StringBuilder's buffering avoided that.
Anyway, thanks for the writeup and book recommendation!

}
}

TelemetryService.DefaultSession.PostEvent(fault);

if (await IsShellAvailable.GetValueAsync())
Expand Down
62 changes: 37 additions & 25 deletions src/NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/NuGetProjectService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,43 +23,55 @@ public sealed class NuGetProjectService : INuGetProjectService
{
private readonly IVsSolutionManager _solutionManager;
private readonly ISettings _settings;
private readonly INuGetTelemetryProvider _telemetryProvider;

public NuGetProjectService(IVsSolutionManager solutionManager, ISettings settings)
public NuGetProjectService(IVsSolutionManager solutionManager, ISettings settings, INuGetTelemetryProvider telemetryProvider)
{
_solutionManager = solutionManager ?? throw new ArgumentNullException(nameof(solutionManager));
_settings = settings ?? throw new ArgumentNullException(nameof(settings));
_telemetryProvider = telemetryProvider ?? throw new ArgumentNullException(nameof(telemetryProvider));
}

public async Task<InstalledPackagesResult> GetInstalledPackagesAsync(Guid projectId, CancellationToken cancellationToken)
{
// Just in case we're on the UI thread, switch to background thread. Very low cost (does not schedule new task) if already on background thread.
await TaskScheduler.Default;

NuGetProject project = await _solutionManager.GetNuGetProjectAsync(projectId.ToString());
if (project == null)
try
{
return NuGetContractsFactory.CreateInstalledPackagesResult(InstalledPackageResultStatus.ProjectNotReady, packages: null);
// Just in case we're on the UI thread, switch to background thread. Very low cost (does not schedule new task) if already on background thread.
await TaskScheduler.Default;

NuGetProject project = await _solutionManager.GetNuGetProjectAsync(projectId.ToString());
if (project == null)
{
return NuGetContractsFactory.CreateInstalledPackagesResult(InstalledPackageResultStatus.ProjectNotReady, packages: null);
}

InstalledPackageResultStatus status;
IReadOnlyCollection<NuGetInstalledPackage> installedPackages;

switch (project)
{
case BuildIntegratedNuGetProject packageReferenceProject:
(status, installedPackages) = await GetInstalledPackagesAsync(packageReferenceProject, cancellationToken);
break;

case MSBuildNuGetProject packagesConfigProject:
(status, installedPackages) = await GetInstalledPackagesAsync(packagesConfigProject, cancellationToken);
break;

default:
(status, installedPackages) = await GetInstalledPackagesAsync(project, cancellationToken);
break;
}

return NuGetContractsFactory.CreateInstalledPackagesResult(status, installedPackages);
}

InstalledPackageResultStatus status;
IReadOnlyCollection<NuGetInstalledPackage> installedPackages;

switch (project)
catch (Exception exception)
{
case BuildIntegratedNuGetProject packageReferenceProject:
(status, installedPackages) = await GetInstalledPackagesAsync(packageReferenceProject, cancellationToken);
break;

case MSBuildNuGetProject packagesConfigProject:
(status, installedPackages) = await GetInstalledPackagesAsync(packagesConfigProject, cancellationToken);
break;

default:
(status, installedPackages) = await GetInstalledPackagesAsync(project, cancellationToken);
break;
var extraProperties = new Dictionary<string, object>();
extraProperties["projectId"] = projectId.ToString();
await _telemetryProvider.PostFaultAsync(exception, typeof(NuGetProjectService).FullName, extraProperties: extraProperties);
throw;
}

return NuGetContractsFactory.CreateInstalledPackagesResult(status, installedPackages);
}

private async Task<(InstalledPackageResultStatus, IReadOnlyCollection<NuGetInstalledPackage>)> GetInstalledPackagesAsync(BuildIntegratedNuGetProject project, CancellationToken cancellationToken)
Expand Down
134 changes: 93 additions & 41 deletions ...NuGet.Clients/NuGet.VisualStudio.Implementation/Extensibility/VsFrameworkCompatibility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using NuGet.Commands;
using NuGet.Frameworks;
using NuGet.VisualStudio.Implementation.Resources;
using NuGet.VisualStudio.Telemetry;

namespace NuGet.VisualStudio
{
Expand All @@ -17,12 +18,29 @@ namespace NuGet.VisualStudio
[Export(typeof(IVsFrameworkCompatibility3))]
public class VsFrameworkCompatibility : IVsFrameworkCompatibility2, IVsFrameworkCompatibility3
{
private INuGetTelemetryProvider _telemetryProvider;

[ImportingConstructor]
public VsFrameworkCompatibility(INuGetTelemetryProvider telemetryProvider)
{
_telemetryProvider = telemetryProvider;
}

public IEnumerable<FrameworkName> GetNetStandardFrameworks()
{
return DefaultFrameworkNameProvider
.Instance
.GetNetStandardVersions()
.Select(framework => new FrameworkName(framework.DotNetFrameworkName));
try
{
Copy link
Member

Choose a reason for hiding this comment

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

this time of delegation makes me cringe :D
The problem is that the only other alternative I can suggest is to have a wrapper that uses reflection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another alternative, which I implemented, is to call .ToList() at the end of each. This of course has multiple performance impacts. If the caller does Whatever().First(), then we'll now be evaluating every item in the result set, not just the first. Also, we'll always be doing extra allocations, but it's hard to imagine that these APIs are being used in such critical codepaths where that perf is important.

return DefaultFrameworkNameProvider
.Instance
.GetNetStandardVersions()
.Select(framework => new FrameworkName(framework.DotNetFrameworkName))
.ToList();
}
catch (Exception exception)
{
_telemetryProvider.PostFault(exception, typeof(VsFrameworkCompatibility).FullName);
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
throw;
}
}

public IEnumerable<FrameworkName> GetFrameworksSupportingNetStandard(FrameworkName frameworkName)
Expand All @@ -32,21 +50,30 @@ public IEnumerable<FrameworkName> GetFrameworksSupportingNetStandard(FrameworkNa
throw new ArgumentNullException(nameof(frameworkName));
}

var nuGetFramework = NuGetFramework.ParseFrameworkName(frameworkName.ToString(), DefaultFrameworkNameProvider.Instance);
try
{
var nuGetFramework = NuGetFramework.ParseFrameworkName(frameworkName.ToString(), DefaultFrameworkNameProvider.Instance);

if (!StringComparer.OrdinalIgnoreCase.Equals(
nuGetFramework.Framework,
FrameworkConstants.FrameworkIdentifiers.NetStandard))
if (!StringComparer.OrdinalIgnoreCase.Equals(
nuGetFramework.Framework,
FrameworkConstants.FrameworkIdentifiers.NetStandard))
{
throw new ArgumentException(string.Format(
VsResources.InvalidNetStandardFramework,
frameworkName));
}

return CompatibilityListProvider
.Default
.GetFrameworksSupporting(nuGetFramework)
.Select(framework => new FrameworkName(framework.DotNetFrameworkName))
.ToList();
}
catch (Exception exception)
{
throw new ArgumentException(string.Format(
VsResources.InvalidNetStandardFramework,
frameworkName));
_telemetryProvider.PostFault(exception, typeof(VsFrameworkCompatibility).FullName);
throw;
}

return CompatibilityListProvider
.Default
.GetFrameworksSupporting(nuGetFramework)
.Select(framework => new FrameworkName(framework.DotNetFrameworkName));
}

public FrameworkName GetNearest(FrameworkName targetFramework, IEnumerable<FrameworkName> frameworks)
Expand All @@ -71,29 +98,46 @@ public FrameworkName GetNearest(FrameworkName targetFramework, IEnumerable<Frame
throw new ArgumentNullException(nameof(frameworks));
}

var nuGetTargetFramework = NuGetFramework.ParseFrameworkName(targetFramework.ToString(), DefaultFrameworkNameProvider.Instance);
IEnumerable<NuGetFramework> ParseAllFrameworks(IEnumerable<FrameworkName> frameworks, string argumentName)
{
foreach (FrameworkName frameworkName in frameworks)
{
if (frameworkName == null)
{
throw new ArgumentException(message: VsResourcesFormat.PropertyCannotBeNull(nameof(FrameworkName)), paramName: nameof(frameworks));
}

var nuGetFallbackTargetFrameworks = fallbackTargetFrameworks
.Select(framework => NuGetFramework.ParseFrameworkName(framework.ToString(), DefaultFrameworkNameProvider.Instance))
.ToList();
NuGetFramework nugetFramework = NuGetFramework.ParseFrameworkName(frameworkName.ToString(), DefaultFrameworkNameProvider.Instance);
yield return nugetFramework;
}
}

if (nuGetFallbackTargetFrameworks.Any())
var nuGetTargetFramework = NuGetFramework.ParseFrameworkName(targetFramework.ToString(), DefaultFrameworkNameProvider.Instance);
var nuGetFallbackTargetFrameworks = ParseAllFrameworks(fallbackTargetFrameworks, nameof(fallbackTargetFrameworks)).ToList();
var nuGetFrameworks = ParseAllFrameworks(frameworks, nameof(frameworks)).ToList();

try
{
nuGetTargetFramework = new FallbackFramework(nuGetTargetFramework, nuGetFallbackTargetFrameworks);
}
if (nuGetFallbackTargetFrameworks.Any())
{
nuGetTargetFramework = new FallbackFramework(nuGetTargetFramework, nuGetFallbackTargetFrameworks);
}

var nuGetFrameworks = frameworks
.Select(framework => NuGetFramework.ParseFrameworkName(framework.ToString(), DefaultFrameworkNameProvider.Instance));
var reducer = new FrameworkReducer();
var nearest = reducer.GetNearest(nuGetTargetFramework, nuGetFrameworks);

var reducer = new FrameworkReducer();
var nearest = reducer.GetNearest(nuGetTargetFramework, nuGetFrameworks);
if (nearest == null)
{
return null;
}

if (nearest == null)
return new FrameworkName(nearest.DotNetFrameworkName);
}
catch (Exception exception)
{
return null;
_telemetryProvider.PostFault(exception, typeof(VsFrameworkCompatibility).FullName);
throw;
}

return new FrameworkName(nearest.DotNetFrameworkName);
}

public IVsNuGetFramework GetNearest(IVsNuGetFramework targetFramework, IEnumerable<IVsNuGetFramework> frameworks)
Expand Down Expand Up @@ -154,21 +198,29 @@ List<NuGetFramework> ToNuGetFrameworks(IEnumerable<IVsNuGetFramework> enumerable
List<NuGetFramework> nugetFallbackTargetFrameworks = ToNuGetFrameworks(fallbackTargetFrameworks, nameof(fallbackTargetFrameworks));
List<NuGetFramework> nugetFrameworks = ToNuGetFrameworks(frameworks, nameof(frameworks));

if (nugetFallbackTargetFrameworks.Count > 0)
try
{
targetNuGetFramework = new FallbackFramework(targetNuGetFramework, nugetFallbackTargetFrameworks);
}
if (nugetFallbackTargetFrameworks.Count > 0)
{
targetNuGetFramework = new FallbackFramework(targetNuGetFramework, nugetFallbackTargetFrameworks);
}

var reducer = new FrameworkReducer();
var nearest = reducer.GetNearest(targetNuGetFramework, nugetFrameworks);
var reducer = new FrameworkReducer();
var nearest = reducer.GetNearest(targetNuGetFramework, nugetFrameworks);

if (nearest == null)
if (nearest == null)
{
return null;
}

var originalFrameworkString = inputFrameworks[nearest];
return originalFrameworkString;
}
catch (Exception exception)
{
return null;
_telemetryProvider.PostFault(exception, typeof(VsFrameworkCompatibility).FullName);
throw;
}

var originalFrameworkString = inputFrameworks[nearest];
return originalFrameworkString;
}
}
}