-
Notifications
You must be signed in to change notification settings - Fork 696
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
Telemetry to Count Top Level & Transitive source mappings automatically created #5377
Conversation
// The top-level package is not already mapped to the selected source. | ||
if (configuredSource.Count == 0 || !configuredSource.Contains(userAction.SelectedSourceName)) | ||
{ | ||
countCreatedTopLevelSourceMappings = 1; |
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 vision I have here is when multiple top-levels can be passed in and mapped, the count will become:
countCreatedTopLevelSourceMappings = 1; | |
countCreatedTopLevelSourceMappings++; |
src/NuGet.Clients/NuGet.PackageManagement.UI/Utility/PackageSourceMappingUtility.cs
Show resolved
Hide resolved
string[] packageIdsNeedingNewSourceMappings = addedPackageIdsWithoutExistingMappings | ||
.Append(userAction.PackageId) | ||
.ToArray(); | ||
countCreatedTransitiveSourceMappings = addedPackageIdsWithoutExistingMappings.Count - countCreatedTopLevelSourceMappings; |
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 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.
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/2450
Fixes: NuGet/Home#12839
Regression? Last working version:
Description
ConfigureNewPackageSourceMapping
whether the top-level is already mapped, to avoid telemetry emittingcount:1
for top-level when it's already mapped.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation