Add logic for respecting the package auto-delete configuration #5415

joelverhagen · 2018-02-07T00:14:33Z

Address https://github.com/NuGet/Engineering/issues/921.

Could I get some crazy nitpick 👀s on this PR? There is risk associated with this change as it allows users to delete their own packages in some cases.

I check both the DB and the statistics reports for download counts. This is to improve resiliency as separate jobs write these two artifacts.

skofman1 · 2018-02-07T00:35:28Z

src/NuGetGallery/Services/PackageDeleteService.cs

+                    if (package.PackageRegistration.DownloadCount > _config.MaximumDownloadsForPackageId.Value)
+                    {
+                        return false;
+                    }


There are cases when we can avoid _statisticsService.Refresh() which is expensive. Restructure the code to be cheaper.

True, I really only need it before the stale check.

skofman1 · 2018-02-07T00:38:56Z

src/NuGetGallery/Services/PackageDeleteService.cs

+            var sinceCreated = now - package.Created;
+
+            // Always refresh the statistics.
+            await _statisticsService.Refresh();


_statisticsService.Refresh() [](start = 18, length = 28)

We will want to analyze the success of the feature later on and understand how to improve it. Add traces that help understand why we returned false. Consider structuring if in a way that will allow creating a table of input properties and result.

Great idea!

scottbommarito

everything looks fine to me

scottbommarito · 2018-02-07T00:25:14Z

src/NuGetGallery/Services/PackageDeleteService.cs

+
+            // Handle the "early" delete case, where the package version download count is not considered but total
+            // download count on the entire ID (all versions) is considered.
+            if (_config.StatisticsUpdateFrequencyInHours.HasValue


IMO the name StatisticsUpdateFrequencyInHours is a bit misleading...potentially something along the lines of ExpectedStatisticsUpdateFrequencyInHours would be more accurately convey that stats are not GUARANTEED to be updated every X hours.

Expected is good. I'll add that.

Actually, I decided to not make this change as it would require a config change in NuGetDeployment. This is not worth the effort.

scottbommarito · 2018-02-07T00:30:07Z

src/NuGetGallery/Services/PackageDeleteService.cs

+            var sinceCreated = now - package.Created;
+
+            // Always refresh the statistics.
+            await _statisticsService.Refresh();


So we will refresh stats every time someone goes to the ReportMyPackage page or submit the ReportMyPackage form?

I'm not sure about the logistics of this method (does it always refresh or is it just if the cache is stale?) but if this refresh is not cached, somebody could create a lot of turmoil on the site by hitting either of these two methods repeatedly.

The refresh is cached and the cache is a singleton.

scottbommarito · 2018-02-07T00:34:12Z

src/NuGetGallery/Services/PackageDeleteService.cs

+                && !_config.StatisticsUpdateFrequencyInHours.HasValue;
+        }
+
+        private bool AreStatisticsStale(DateTime now)


This sounds like a method that should be on the StatisticsService itself, but if it's too much work to move it there it's fine here.

Stale is a relative thing. Different scenarios may or may not have different "stale" definitions. For now the concept only exists in the auto-delete workflow.

joelverhagen · 2018-02-07T16:21:46Z

src/NuGetGallery/Services/PackageDeleteService.cs

                    $"{nameof(_config.HourLimitWithMaximumDownloads)}.",
                    nameof(config));
            }
        }

-        public Task<bool> CanPackageBeDeletedByUserAsync(Package package)
+        public async Task<bool> CanPackageBeDeletedByUserAsync(Package package)


Thoughts on only emitting telemetry when the support request form is actually submitted?

This would reduce the amount of de-duping we would have to do on the telemetry -- but not eliminate all duplicates, since a user could support multiple support requests for a package. I figured it would just be simpler to submit telemetry whenever this method is called and de-dupe later when we are analyzing the data.

As it stands today, telemetry is emitted before the support request form is loaded and after the form is submitted with a deletion request.

Sounds like we'll do the check before, and only show the link if it's possible? I think we should only submit telemetry on submit, not view rendering.

Sounds like we'll do the check before, and only show the link if it's possible?

Correct.

I think we should only submit telemetry on submit, not view rendering.

Yeah, that's where I'm leaning too.

chenriksson · 2018-02-07T16:35:40Z

src/NuGetGallery/Services/PackageDeleteService.cs

+            // Handle the "early" delete case, where the package version download count is not considered but total
+            // download count on the entire ID (all versions) is considered.
+            if (_config.ExpectedStatisticsUpdateFrequencyInHours.HasValue
+                && details.SinceCreated < TimeSpan.FromHours(_config.ExpectedStatisticsUpdateFrequencyInHours.Value))


Does this mean owners of popular packages that want to delete will need to unlist immediately, then can delete after the hour limit has expired?

Unlisted is not a factor here.

Owners of popular packages (IDs > 125,000 downloads) cannot delete their packages at all.

chenriksson · 2018-02-07T16:44:13Z

src/NuGetGallery/Configuration/IPackageDeleteConfiguration.cs

-        /// allowed since the download data is too stale. If <see cref="StatisticsUpdateFrequencyInHours"/> is null,
+        /// <see cref="ExpectedStatisticsUpdateFrequencyInHours"/> hours. Also, if the package has not had its download
+        /// statistics updated in the last <see cref="ExpectedStatisticsUpdateFrequencyInHours"/> hours, the delete is not
+        /// allowed since the download data is too stale. If <see cref="ExpectedStatisticsUpdateFrequencyInHours"/> is null,


Should this be configured based on the frequency of the stats jobs? Should we have a specific recommendation in comments, based on job?

Should this be configured based on the frequency of the stats jobs?

That's not too necessary. As mentioned in the XML doc, this value is used for a stale check (so the value can be larger than the average frequency).

Should we have a specific recommendation in comments, based on job?

Yeah, I'll try to improve this.

chenriksson · 2018-02-07T16:49:46Z

src/NuGetGallery/Telemetry/UserPackageDeleteOutcome.cs

+        StaleStatistics,
+        TooManyVersionDatabaseDownloads,
+        TooManyVersionReportDownloads,
+        TooLate,


Will the UI always show an option to delete, but present the error message if the criteria aren't met?

Or do we check before showing a link, and after / on submit?

Will the UI always show an option to delete, but present the error message if the criteria aren't met?

Nope, we only show the option if at the time of form load their package is eligible.

Or do we check before showing a link, and after / on submit?

Correct. And if the eligibility has changed since the form was presented, we just treat it as a normal support request.

chenriksson · 2018-02-07T16:52:22Z

src/NuGetGallery/Services/PackageDeleteService.cs

                    $"{nameof(_config.HourLimitWithMaximumDownloads)}.",
                    nameof(config));
            }
        }

-        public Task<bool> CanPackageBeDeletedByUserAsync(Package package)
+        public async Task<bool> CanPackageBeDeletedByUserAsync(Package package)


Sounds like we'll do the check before, and only show the link if it's possible? I think we should only submit telemetry on submit, not view rendering.

loic-sharma · 2018-02-07T18:03:07Z

src/NuGetGallery/Services/PackageDeleteService.cs

+
+        private bool HasVersion(StatisticsFact fact, string version)
+        {
+            if (fact != null


if [](start = 12, length = 2)

Consider return xyz; instead of if (xyz) { return true } return false;,

loic-sharma · 2018-02-07T18:08:25Z

src/NuGetGallery/Services/PackageDeleteService.cs

+            }
+
+            // If no time ranges are configured, allow downloads any time.
+            if (_config.HourLimitWithMaximumDownloads.HasValue


if [](start = 12, length = 2)

Could we convert the chains of if to else ifs? It'd be a little more clear I think.

loic-sharma · 2018-02-07T18:34:50Z

src/NuGetGallery/Services/TelemetryClientWrapper.cs

+            }
+            catch
+            {
+                // logging failed, don't allow exception to escape


// logging failed, don't allow exception to escape [](start = 16, length = 50)

Should we maybe log these exceptions?

Out of scope.

loic-sharma · 2018-02-07T18:36:41Z

src/NuGetGallery/Services/TelemetryService.cs

+
+            var hours = details.SinceCreated.TotalHours;
+
+            TrackMetric(Events.UserPackageDeleteAfterHours, hours, properties => {


properties [](start = 67, length = 10)

The action here seems unnecessary. I think this just creating a dictionary and using its initializer syntax would be better.

Existing pattern.

(it saves the initialization of the dictionary)

Progress on NuGet/Engineering#921

joelverhagen requested review from agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum, skofman1 and scottbommarito February 7, 2018 00:14

skofman1 reviewed Feb 7, 2018

View reviewed changes

scottbommarito reviewed Feb 7, 2018

View reviewed changes

joelverhagen force-pushed the jver-fresh-stats branch from 9a6afed to 8d3bd1a Compare February 7, 2018 16:17

joelverhagen commented Feb 7, 2018

View reviewed changes

chenriksson reviewed Feb 7, 2018

View reviewed changes

chenriksson approved these changes Feb 7, 2018

View reviewed changes

scottbommarito approved these changes Feb 7, 2018

View reviewed changes

loic-sharma reviewed Feb 7, 2018

View reviewed changes

loic-sharma approved these changes Feb 7, 2018

View reviewed changes

joelverhagen force-pushed the jver-fresh-stats branch from 3f38e5f to 00e5a4e Compare February 7, 2018 19:37

joelverhagen added a commit that referenced this pull request Feb 7, 2018

Add logic for respecting the package auto-delete configuration (#5415)

00e5a4e

Progress on NuGet/Engineering#921

Add logic for respecting the package auto-delete configuration (#5415)

044866c

Progress on NuGet/Engineering#921

joelverhagen force-pushed the jver-fresh-stats branch from 00e5a4e to 044866c Compare February 8, 2018 00:32

joelverhagen merged commit 044866c into dev Feb 8, 2018

joelverhagen deleted the jver-fresh-stats branch February 8, 2018 00:45


		var hours = details.SinceCreated.TotalHours;

		TrackMetric(Events.UserPackageDeleteAfterHours, hours, properties => {

Add logic for respecting the package auto-delete configuration #5415

Add logic for respecting the package auto-delete configuration #5415

Conversation

joelverhagen commented Feb 7, 2018

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

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

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