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

Telemetry to Count Top Level & Transitive source mappings automatically created #5377

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Telemetry to Count Top Level & Transitive source mappings automatically created #5377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Actions/UIActionEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,9 @@ private async Task PerformActionImplAsync(

await _lockService.ExecuteNuGetOperationAsync(async () =>
{
int? countCreatedTopLevelSourceMappings = null;
int? countCreatedTransitiveSourceMappings = null;

try
{
uiService.BeginOperation();
Expand Down Expand Up @@ -466,7 +469,7 @@ await _lockService.ExecuteNuGetOperationAsync(async () =>
if (!cancellationToken.IsCancellationRequested)
{
List<string>? addedPackageIds = addedPackages != null ? addedPackages.Select(pair => pair.Item1).Distinct().ToList() : null;
PackageSourceMappingUtility.ConfigureNewPackageSourceMapping(userAction, addedPackageIds, sourceMappingProvider, existingPackageSourceMappingSourceItems);
PackageSourceMappingUtility.ConfigureNewPackageSourceMapping(userAction, addedPackageIds, sourceMappingProvider, existingPackageSourceMappingSourceItems, out countCreatedTopLevelSourceMappings, out countCreatedTransitiveSourceMappings);

await projectManagerService.ExecuteActionsAsync(
actions,
Expand Down Expand Up @@ -559,7 +562,9 @@ await projectManagerService.ExecuteActionsAsync(
removedPackages,
updatedPackagesOld,
updatedPackagesNew,
frameworks);
frameworks,
countCreatedTopLevelSourceMappings,
countCreatedTransitiveSourceMappings);

if (packageToInstallWasTransitive.HasValue)
{
Expand Down Expand Up @@ -609,7 +614,9 @@ internal static void AddUiActionEngineTelemetryProperties(
List<string>? removedPackages,
List<Tuple<string, string>>? updatedPackagesOld,
List<Tuple<string, string>>? updatedPackagesNew,
IReadOnlyCollection<string> targetFrameworks)
IReadOnlyCollection<string> targetFrameworks,
int? countCreatedTopLevelSourceMappings,
int? countCreatedTransitiveSourceMappings)
{
// log possible cancel reasons
if (!continueAfterPreview)
Expand Down Expand Up @@ -715,6 +722,16 @@ internal static void AddUiActionEngineTelemetryProperties(
{
actionTelemetryEvent["TargetFrameworks"] = string.Join(";", targetFrameworks);
}

if (countCreatedTopLevelSourceMappings.HasValue)
{
actionTelemetryEvent["CreatedTopLevelSourceMappingsCount"] = countCreatedTopLevelSourceMappings.Value;
}

if (countCreatedTransitiveSourceMappings.HasValue)
{
actionTelemetryEvent["CreatedTransitiveSourceMappingsCount"] = countCreatedTransitiveSourceMappings.Value;
}
}

private async Task<bool> CheckPackageManagementFormatAsync(
Expand Down
49 changes: 38 additions & 11 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/PackageSourceMappingUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,53 +12,80 @@ namespace NuGet.PackageManagement.UI
{
internal static class PackageSourceMappingUtility
{
/// <summary>
/// Determines if new package source mappings should be written to the <paramref name="sourceMappingProvider"/> which is returned as a count.
/// </summary>
/// <param name="userAction"></param>
/// <param name="addedPackageIds"></param>
/// <param name="sourceMappingProvider"></param>
/// <param name="existingPackageSourceMappingSourceItems"></param>
/// <param name="countCreatedTopLevelSourceMappings">For Top-Level packages: <c>null</c> if not applicable; 0 if none needed to be added; > 0 is the count of new package source mappings added.</param>
/// <param name="countCreatedTransitiveSourceMappings">For Transitive packages: <c>null</c> if not applicable; 0 if none needed to be added; > 0 is the count of new package source mappings added.</param>
internal static void ConfigureNewPackageSourceMapping(
UserAction? userAction,
IReadOnlyList<string>? addedPackageIds,
PackageSourceMappingProvider sourceMappingProvider,
IReadOnlyList<PackageSourceMappingSourceItem> existingPackageSourceMappingSourceItems)
IReadOnlyList<PackageSourceMappingSourceItem> existingPackageSourceMappingSourceItems,
out int? countCreatedTopLevelSourceMappings,
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved
out int? countCreatedTransitiveSourceMappings)
{
countCreatedTopLevelSourceMappings = null;
countCreatedTransitiveSourceMappings = null;

if (userAction?.SelectedSourceName is null || addedPackageIds is null)
{
return;
}

countCreatedTopLevelSourceMappings = 0;
countCreatedTransitiveSourceMappings = 0;

string topLevelPackageId = userAction.PackageId;
Dictionary<string, IReadOnlyList<string>> patternsReadOnly = existingPackageSourceMappingSourceItems
.ToDictionary(pair => pair.Key, pair => (IReadOnlyList<string>)(pair.Patterns.Select(p => p.Pattern).ToList()));

PackageSourceMapping packageSourceMapping = new(patternsReadOnly);

// Expand all patterns/globs so we can later check if this package ID was already mapped.
List<string> addedPackageIdsWithoutExistingMappings = new();
List<string> addedPackageIdsWithoutExistingMappings = new(capacity: addedPackageIds.Count + 1);

foreach (string addedPackageId in addedPackageIds)
{
IReadOnlyList<string> configuredSource = packageSourceMapping.GetConfiguredPackageSources(addedPackageId);
if (configuredSource.Count == 0)

// Top-level package was looked up.
if (addedPackageId == topLevelPackageId)
{
// The top-level package is not already mapped to the selected source.
if (configuredSource.Count == 0 || !configuredSource.Contains(userAction.SelectedSourceName))
{
countCreatedTopLevelSourceMappings = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vision I have here is when multiple top-levels can be passed in and mapped, the count will become:

Suggested change
countCreatedTopLevelSourceMappings = 1;
countCreatedTopLevelSourceMappings++;

addedPackageIdsWithoutExistingMappings.Add(topLevelPackageId);
}
}
// Transitive package was looked up.
else if (configuredSource.Count == 0)
{
addedPackageIdsWithoutExistingMappings.Add(addedPackageId);
}
}

// Get all newly added package IDs that were not previously Source Mapped.
// Always include the Package ID being installed since it takes precedence over any globbing.
string[] packageIdsNeedingNewSourceMappings = addedPackageIdsWithoutExistingMappings
.Append(userAction.PackageId)
.ToArray();
countCreatedTransitiveSourceMappings = addedPackageIdsWithoutExistingMappings.Count - countCreatedTopLevelSourceMappings;
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this calculation is that, if there's a bug in the logic in this function, telemetry would not help us understand what the bug is. Instead, I suggest incrementing the counter after line 61 (within the else if) and reporting that.


CreateAndSavePackageSourceMappings(
userAction.SelectedSourceName,
packageIdsNeedingNewSourceMappings,
addedPackageIdsWithoutExistingMappings,
sourceMappingProvider,
existingPackageSourceMappingSourceItems);
}

private static void CreateAndSavePackageSourceMappings(
string sourceName,
string[] newPackageIdsToSourceMap,
List<string> newPackageIdsToSourceMap,
PackageSourceMappingProvider mappingProvider,
IReadOnlyList<PackageSourceMappingSourceItem> existingPackageSourceMappingSourceItems)
{
if (string.IsNullOrWhiteSpace(sourceName) || newPackageIdsToSourceMap is null || newPackageIdsToSourceMap.Length == 0)
if (string.IsNullOrWhiteSpace(sourceName) || newPackageIdsToSourceMap is null || newPackageIdsToSourceMap.Count == 0)
{
return;
}
Expand Down
61 changes: 60 additions & 1 deletion test/NuGet.Clients.Tests/NuGet.PackageManagement.UI.Test/Actions/UIActionEngineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ public void AddUiActionEngineTelemetryProperties_AddsVulnerabilityInfo_Succeeds(
removedPackages: null,
updatedPackagesOld: null,
updatedPackagesNew: null,
targetFrameworks: null);
targetFrameworks: null,
countCreatedTopLevelSourceMappings: null,
countCreatedTransitiveSourceMappings: null);

// Act
var service = new NuGetVSTelemetryService(telemetrySession.Object);
Expand All @@ -290,6 +292,63 @@ public void AddUiActionEngineTelemetryProperties_AddsVulnerabilityInfo_Succeeds(
item => Assert.Equal(1, item),
item => Assert.Equal(3, item));
Assert.Equal(3, pkgSeverities.Count());
Assert.Null(lastTelemetryEvent["CreatedTopLevelSourceMappingsCount"]);
Assert.Null(lastTelemetryEvent["CreatedTransitiveSourceMappingsCount"]);
}

[Fact]
public void ActionCreatingSourceMappings_TopLevelCountAndTransitiveCount_AddsValue()
{
// Arrange
int expectedCountCreatedTopLevelSourceMappings = 42;
int expectedCountCreatedTransitiveSourceMappings = 24;
var telemetrySession = new Mock<ITelemetrySession>();
TelemetryEvent lastTelemetryEvent = null;
telemetrySession
.Setup(x => x.PostEvent(It.IsAny<TelemetryEvent>()))
.Callback<TelemetryEvent>(x => lastTelemetryEvent = x);

var operationId = Guid.NewGuid().ToString();

var actionTelemetryData = new VSActionsTelemetryEvent(
operationId,
projectIds: new[] { Guid.NewGuid().ToString() },
operationType: NuGetOperationType.Install,
source: OperationSource.PMC,
startTime: DateTimeOffset.Now.AddSeconds(-1),
status: NuGetOperationStatus.NoOp,
packageCount: 1,
endTime: DateTimeOffset.Now,
duration: .40,
isPackageSourceMappingEnabled: false);

UIActionEngine.AddUiActionEngineTelemetryProperties(
actionTelemetryEvent: actionTelemetryData,
continueAfterPreview: true,
acceptedLicense: true,
userAction: null,
selectedIndex: 0,
recommendedCount: 0,
recommendPackages: false,
recommenderVersion: null,
topLevelVulnerablePackagesCount: 3,
topLevelVulnerablePackagesMaxSeverities: new List<int> { 1, 1, 3 }, // each package has its own max severity
existingPackages: null,
addedPackages: null,
removedPackages: null,
updatedPackagesOld: null,
updatedPackagesNew: null,
targetFrameworks: null,
countCreatedTopLevelSourceMappings: expectedCountCreatedTopLevelSourceMappings,
countCreatedTransitiveSourceMappings: expectedCountCreatedTransitiveSourceMappings);

// Act
var service = new NuGetVSTelemetryService(telemetrySession.Object);
service.EmitTelemetryEvent(actionTelemetryData);

// Assert
Assert.Equal(expectedCountCreatedTopLevelSourceMappings, (int)lastTelemetryEvent["CreatedTopLevelSourceMappingsCount"]);
Assert.Equal(expectedCountCreatedTransitiveSourceMappings, (int)lastTelemetryEvent["CreatedTransitiveSourceMappingsCount"]);
}

[Theory]
Expand Down