Change parsing algorithm from download URL to package id+version #343

skofman1 · 2018-02-17T01:21:08Z

A proposal to change the algorithm that parses download URL to Package id+version.
Comes as a bug fix for: NuGet/NuGetGallery#5498

The issue is that version 2.4.0-beta.1.build3958 is not parsed correctly, because the current alg counts the number of '.' in the file name.
Note, that this code doesn't solve all issues (like [.][Number] in the id) - only improved the current state a bit.
The proposal assumes that the version in the URL has to be normalized. It makes sense because this is how we save packages, and the same assumption is used in Gallery to generate URLs to packages.
It brute forces all the version options, and comes up with a list of possible combinations of [id, version] where the version is a legal normalized string.
In this PR, I only log the different combinations if there is more then one, and use the first one.
In the future we should consider a "smart" way to decide which combination is valid. Based on the logs we can determine how frequently we get multiple options, and which solution we need (i.e. how much DB load we should expect if we use Gallery DB).

I still got a bunch of tests to run, but would appreciate your feedback early on.

joelverhagen

I think it makes sense here to hit some other data source to resolve the conflict case multiple package definitions parsed, e.g. HEAD on packages container, gallery DB query etc. It looks like we have improved the parsing a bit but it doesn't resolve the root problem.

Also, do we have data on the number of packages the old algorithm vs. the new algorithm could parse successfully? We should run the two impls over the entire gallery DB. You could even weight the results by download count in DB.

joelverhagen · 2018-02-17T16:07:03Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+
+            requestUrl = HttpUtility.UrlDecode(requestUrl);
+
+            var urlSegments = requestUrl.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);


If this is a flat container URL, you can use the n-3 and n-2 segments as well to avoid any ambiguity.

Done. Was surprised that this wasn't done up until now.

joelverhagen · 2018-02-17T16:10:16Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+                    {
+                        var normalizedVersion = parsedVersion.ToNormalizedString();
+
+                        if (string.Compare(normalizedVersion, versionPart, ignoreCase: true) == 0)


No need to use Compare here. Equals is a better fit. The == 0 is a bit more cryptic that a boolean returned by Equals.

joelverhagen · 2018-02-17T16:13:16Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+            var urlSegments = requestUrl.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
+            var fileName = urlSegments.Last();
+
+            if (fileName.EndsWith(_nupkgExtension))


Redundant with the check above. Also, do we have a CDN rule making this case insensitive? I think no but it's worth checking if .NUPKG works.

joelverhagen · 2018-02-17T16:14:08Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+
+        private static bool IsNumeric(string segment)
+        {
+            return int.TryParse(segment, out _);


How is this method used?

I think this was previously used but is no longer...should be removed.

joelverhagen · 2018-02-17T16:15:17Z

src/Stats.ImportAzureCdnStatistics/PackageStatisticsParser.cs

+                _logger.LogWarning(LogEvents.MultiplePackageIDVersionParseOptions, message);
+            }
+
+            var packageDefinition = packageDefinitions.First();


Why do we assume the first here? Could all of them be run against the translator?

Translator could potentially de-dupe some PackageDefinitions and then get rid of the need to warn.

If you look at the cases the translator is fixing, those are the cases when a package id ends with [dot][Number].
For example:
"incorrectpackageid": "xrmlibrary.extensionmethods",
"incorrectpackageversionpattern": "2016.(.*)",
"correctedpackageid": "xrmlibrary.extensionmethods.2016",
"correctedpackageversionpattern": "$1"

in order for this to work, the incorrect package id should be without a number. This is exactly what you get in the first result of the new algorithm, meaning, any other check will be redundant and impact perf.

joelverhagen · 2018-02-17T16:16:13Z

src/Stats.ImportAzureCdnStatistics/PackageStatisticsParser.cs

+            if (packageDefinitions.Count > 1)
+            {
+                var message = $"Found multiple parse options for URL {cdnLogEntry.RequestUrl}: {string.Join(", ", packageDefinitions)}";
+                _logger.LogWarning(LogEvents.MultiplePackageIDVersionParseOptions, message);


Will we have a metric or Sev4 on this? Seems like a warning we will probably never remember to look for.

This data is meant to be used to assess future solutions for resolving ambiguity (like DB check/blob HEAD request). There is no intention to monitor this, since it's not actionable.

scottbommarito

I agree with @joelverhagen --this is a step forward but it doesn't resolve the root problem. We should make a simple HEAD request on the feed or gallery DB to resolve conflicts. I don't think it would be too hard to implement or would add too much complexity to the job and it would have enormous benefit to us. Regardless, I would be ok with taking this change on its own and implementing checking the feed in a separate PR. I also agree with @joelverhagen that we should try and run some tests on how this would perform on our existing packages so that we don't break any existing download counts.

scottbommarito · 2018-02-20T16:15:18Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+
+                var nextDotIndex = fileName.IndexOf('.');
+
+                while (nextDotIndex != -1)


I'm pretty sure you can write this as a do while and remove the redundancy between

var nextDotIndex = fileName.IndexOf('.');

and

nextDotIndex = fileName.IndexOf('.', nextDotIndex + 1);

e.g.

var nextDotIndex = -1; do { nextDotIndex = fileName.IndexOf('.', nextDotIndex + 1); ... rest of the code ... } while (nextDotIndex != -1)

Don't think it makes a big difference. Will keep as is.

scottbommarito · 2018-02-20T16:19:22Z

src/Stats.ImportAzureCdnStatistics/PackageDefinition.cs

+
+        private static bool IsNumeric(string segment)
+        {
+            return int.TryParse(segment, out _);


I think this was previously used but is no longer...should be removed.

scottbommarito · 2018-02-20T16:43:15Z

src/Stats.ImportAzureCdnStatistics/PackageTranslator.cs

        {
+            bool translateOccurred = false;


No need for this variable. Just return true where you set translateOccurred to true and return false at the end of the function.

I prefer a method to have a single return statement. Makes the code more readable.

scottbommarito · 2018-02-20T16:44:24Z

tests/Tests.Stats.ImportAzureCdnStatistics/PackageDefinitionFacts.cs

        [InlineData("nuget.core", "1.0.1-beta1", "http://localhost/packages/nuget.core.1.0.1-beta1.nupkg")]
        [InlineData("nuget.core", "1.0.1-beta1.1", "http://localhost/packages/nuget.core.1.0.1-beta1.1.nupkg")]
-        [InlineData("nuget.core", "1.0.1+metadata", "http://localhost/packages/nuget.core.1.0.1%2Bmetadata.nupkg")]


Why are we removing tests?

a package URL contains only normalized strings. I removed all the tests cases that contain non-normalized strings, since the new algorithm uses this assumption.

scottbommarito · 2018-02-20T16:47:41Z

src/Stats.ImportAzureCdnStatistics/PackageStatisticsParser.cs

+                _logger.LogWarning(LogEvents.MultiplePackageIDVersionParseOptions, message);
+            }
+
+            var packageDefinition = packageDefinitions.First();


Translator could potentially de-dupe some PackageDefinitions and then get rid of the need to warn.

xavierdecoster · 2018-02-20T18:26:47Z

I think it makes sense here to hit some other data source to resolve the conflict case multiple package definitions parsed, e.g. HEAD on packages container, gallery DB query etc. It looks like we have improved the parsing a bit but it doesn't resolve the root problem.

The (eventual) fix to reach out to the gallery (db or API) should be insanely fast, and cached in the job client. Also note that we have this job running in parallel. Data ingress is enormous, and there's no room to afford a mix-up really, as that would potentially mean to halt the job, retroactively correct parsed blobs and db records (and potentially rolled-up records!), and play catch-up with incoming data when re-enabling the job once a fix is deployed. In other words, this job should really be resilient against gallery or gallery db outages. Currently, it is, as it has no dependency on it.

In essence, we'll need a circuit-breaker for the newly added dependency to gallery db/api when the root cause is being tackled. In addition, a fresh deployment will have no cache for these auto-corrections, which is a bit of a waste. Would be good if the final solution learned over time. There's already some mechanism built-in for corrective mappings, which could be used as a persisted cache.

skofman1 · 2018-02-20T19:48:52Z

Here are some results from comparing the two algorithms. I ran them against all packages in prod.

The old algorithm had 4,442 wrong results.
wrongresult.txt
The new algorithm had 1,999 wrong results:
wrongresult.txt

The above numbers don't include packages that are fixed with the translation json.

Run time: running the new algorithm on ~1.4M packages took ~12 seconds vs. the old algorithm that took ~6 seconds. Although the perf is worse, I think the quality of results justifies this.

loic-sharma · 2018-02-20T19:54:07Z

src/Stats.ImportAzureCdnStatistics/PackageStatisticsParser.cs

+            if (packageDefinitions.Count > 1)
+            {
+                var message = $"Found multiple parse options for URL {cdnLogEntry.RequestUrl}: {string.Join(", ", packageDefinitions)}";
+                _logger.LogWarning(LogEvents.MultiplePackageIDVersionParseOptions, message);


Log formatting can be used here, right?

Same thing with line 52

skofman1 · 2018-02-20T23:15:41Z

@xavierdecoster , agree. This PR doesn't aim to achieve this eventual fix.

joelverhagen · 2018-02-21T00:26:17Z

The results from PROD DB are conclusive to me.

joelverhagen · 2018-02-21T00:30:55Z

These ones don't work anymore:

[ImisComplexIpart20.2.1.235-EA][20.2.1.235-EA]
[ImisIboNet-20.2.1.235-EA][20.2.1.235-EA]
[ImisIboNet20.2.1.235-EA][20.2.1.235-EA]
[Inovout.Data.1.0.6-beta][1.0.6-beta]
[Inovout.Web.Mvc.1.0.6-beta][1.0.6-beta]
[Magick.NET-7.0.0.18-Q16-x64][1.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.App][2.0.0-preview1-002111-00]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.App][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.App][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetAppHost][2.0.0-preview1-002111-00]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetAppHost][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetAppHost][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHost][2.0.0-preview1-002111-00]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHost][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHost][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostPolicy][2.0.0-preview1-002111-00]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostPolicy][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostPolicy][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostResolver][2.0.0-preview1-002111-00]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostResolver][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.DotNetHostResolver][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILAsm][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILAsm][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILAsm][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILDAsm][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILDAsm][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.ILDAsm][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Jit][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Jit][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Jit][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Native][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Native][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Native][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Runtime.CoreCLR][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Runtime.CoreCLR][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.Runtime.CoreCLR][2.0.0]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.TestHost][2.0.0-preview1-25301-02]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.TestHost][2.0.0-preview2-25407-01]
[runtime.tizen.4.0.0-armel.Microsoft.NETCore.TestHost][2.0.0]
[test.System.DirectoryServices.4.5.0-preview1-2572][4.5.0-preview1-25726-06]
[test.System.DirectoryServices.AccountManagement.4.5.0-preview1-25726][4.5.0-preview1-25726-06]

If we are regressing in some cases I feel less exciting about this fix.

skofman1 · 2018-02-21T22:17:05Z

@joelverhagen , the new algorithm doesn't work well when there's a valid prerelease version as part of the id. In those cases it considers the rest of the string to be part of the prerelease label.
The fact that majority of downloads are from a V3 client (verified for tizen), will mitigate this.

* [Orchestrator] Add maximum retry count for missing packages (#342) * Change parsing algorithm from download URL to package id+version (#343) * Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases * Fix id+version parsing issue for Microsoft.VisualStudio.Shell.15.0 (#347)

* Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases

…size of the test package (#343)

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

* Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases

* [Orchestrator] Add maximum retry count for missing packages (#342) * Change parsing algorithm from download URL to package id+version (#343) * Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases * Fix id+version parsing issue for Microsoft.VisualStudio.Shell.15.0 (#347)

…size of the test package (#343)

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

Change parsing algorithm from download URL to package id+version

0ed8b57

skofman1 requested review from joelverhagen, agr, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum and scottbommarito February 17, 2018 01:21

joelverhagen reviewed Feb 17, 2018

View reviewed changes

scottbommarito reviewed Feb 20, 2018

View reviewed changes

loic-sharma reviewed Feb 20, 2018

View reviewed changes

skofman1 added 2 commits February 20, 2018 13:52

PR comments

cee7fc8

bug fix

1c33e76

joelverhagen approved these changes Feb 21, 2018

View reviewed changes

check failing cases

d0f3206

Merge branch 'dev' into fixparse

8bb6421

skofman1 merged commit fe2af29 into dev Feb 23, 2018

loic-sharma pushed a commit that referenced this pull request Mar 1, 2018

Change parsing algorithm from download URL to package id+version (#343)

abd1334

* Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases

joelverhagen pushed a commit that referenced this pull request Sep 27, 2019

Functional tests should not be dependent on a specific hash and file …

a6f8796

…size of the test package (#343)

joelverhagen pushed a commit that referenced this pull request Sep 27, 2019

Functional tests should not be dependent on a specific hash and file …

7a9acf7

…size of the test package (#343)

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Change parsing algorithm from download URL to package id+version (#343)

c5c255c

* Change parsing algorithm from download URL to package id+version * PR comments * bug fix * check failing cases

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Functional tests should not be dependent on a specific hash and file …

cc21056

…size of the test package (#343)

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Functional tests should not be dependent on a specific hash and file …

aa782c4

…size of the test package (#343)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change parsing algorithm from download URL to package id+version #343

Change parsing algorithm from download URL to package id+version #343

skofman1 commented Feb 17, 2018

joelverhagen left a comment

joelverhagen Feb 17, 2018

skofman1 Feb 20, 2018

joelverhagen Feb 17, 2018

skofman1 Feb 20, 2018

joelverhagen Feb 17, 2018

skofman1 Feb 20, 2018

joelverhagen Feb 17, 2018

scottbommarito Feb 20, 2018

skofman1 Feb 20, 2018

joelverhagen Feb 17, 2018

scottbommarito Feb 20, 2018

skofman1 Feb 20, 2018

joelverhagen Feb 17, 2018

skofman1 Feb 20, 2018

scottbommarito left a comment •

edited

scottbommarito Feb 20, 2018

skofman1 Feb 20, 2018

scottbommarito Feb 20, 2018

scottbommarito Feb 20, 2018

skofman1 Feb 20, 2018

scottbommarito Feb 20, 2018

skofman1 Feb 20, 2018

scottbommarito Feb 20, 2018

xavierdecoster commented Feb 20, 2018

skofman1 commented Feb 20, 2018

loic-sharma Feb 20, 2018 •

edited

skofman1 commented Feb 20, 2018

joelverhagen commented Feb 21, 2018

joelverhagen commented Feb 21, 2018

skofman1 commented Feb 21, 2018


		requestUrl = HttpUtility.UrlDecode(requestUrl);

		var urlSegments = requestUrl.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);


		var nextDotIndex = fileName.IndexOf('.');

		while (nextDotIndex != -1)

Change parsing algorithm from download URL to package id+version #343

Change parsing algorithm from download URL to package id+version #343

Conversation

skofman1 commented Feb 17, 2018

joelverhagen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster commented Feb 20, 2018

skofman1 commented Feb 20, 2018

loic-sharma Feb 20, 2018 • edited

Choose a reason for hiding this comment

skofman1 commented Feb 20, 2018

joelverhagen commented Feb 21, 2018

joelverhagen commented Feb 21, 2018

skofman1 commented Feb 21, 2018

scottbommarito left a comment •

edited

loic-sharma Feb 20, 2018 •

edited