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.

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

Conversation

donnie-msft
Copy link
Contributor

@donnie-msft donnie-msft commented Aug 25, 2023

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2450
Fixes: NuGet/Home#12839

Regression? Last working version:

Description

  • Telemetry will now have 2 integer properties for counts of added top-level and transitive package source mappings.
  • Top-level values can either be 0 or 1 in the current implementation (may become >1 in future iterations)
  • In scenarios when automatic mapping is not possible, the properties are not created (will be missing from telemetry).
  • See description of Issue for telemetry samples
  • Implements more of Assessment: https://github.com/NuGet/Client.Engineering/blob/main/designs/telemetry/assessment-source-mapping.md
  • Secondarily, some refactoring is done in the utility class to return the counts
  • Thirdly, the utility class implementation has changed
    • Determine in ConfigureNewPackageSourceMapping whether the top-level is already mapped, to avoid telemetry emitting count:1 for top-level when it's already mapped.
    • Remove an unnecessary array.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

// 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++;

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.

@donnie-msft donnie-msft requested a review from nkolev92 August 28, 2023 23:42
@donnie-msft donnie-msft merged commit 545b3ce into dev Aug 29, 2023
@donnie-msft donnie-msft deleted the dev-donnie-msft-telSourceMappingActions branch August 29, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package Source Mapping utility always appends package ID
3 participants