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

[Statistics] Introduce Community Statistics Reports #4955

Merged
merged 15 commits into from
Nov 8, 2017
Merged

Conversation

loic-sharma
Copy link
Contributor

@loic-sharma loic-sharma commented Nov 3, 2017

Introduce Community Statistics Reports

This change introduces the concept of "community" statistics report that will be displayed by default. These reports filter out packages owned by Microsoft and .NET Framework. Users can choose to "Show all packages" to view reports that include Microsoft's and .NET Framework's packages. With this change, packages like System.Globalization will no longer clutter our default statistic reports! 😄

Note: The JsonStatisticsService had some serious concurrency issue and did not cache reports that it loaded from blob storage. This pull request fixes that by loading reports once an hour.

This change depends on NuGet/NuGet.Jobs#239.
Fixes #3417.

Screenshots

Statistics Homepage

one

Detailed Report

Clicking on the "Show more..." links at the bottom of the homepage will take you to the following reports:

Most Downloaded Packages

most-downloaded-packages

Most Downloaded Package Versions

most-downloaded-package-versions

@anangaur
Copy link
Member

anangaur commented Nov 3, 2017

I dont think we should have the filter on the https://www.nuget.org/stats page. The filter should only be there on the details pages:
https://www.nuget.org/stats/packageversions
https://www.nuget.org/stats/packages

@anangaur
Copy link
Member

anangaur commented Nov 3, 2017

Something like this (ignore the actual list that shows the current data):

image

@loic-sharma
Copy link
Contributor Author

Added a filter to both index and detailed pages. I will update the screenshots on Monday.

public async Task<StatisticsReportResult> LoadDownloadPackageVersions()
private Task<StatisticsReportResult> LoadDownloadPackageVersions()
{
var statisticsPackagesItemViewModels = (List<StatisticsPackagesItemViewModel>)DownloadPackageVersionsAll;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Delete this.

/// Breakout by version for a community package (drill down from RecentPopularity).
/// This excludes Microsoft and ASP.NET packages.
/// </summary>
RecentCommunityPopularityDetail_,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: This is not needed, remove.

@@ -704,9 +704,9 @@ protected internal virtual Stream ReadPackageFromRequest()
[ActionName("StatisticsDownloadsApi")]
public virtual async Task<ActionResult> GetStatsDownloads(int? count)
{
var result = await StatisticsService.LoadDownloadPackageVersions();
await StatisticsService.Refresh();
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@loic-sharma loic-sharma Nov 7, 2017

Choose a reason for hiding this comment

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

See the description:

The JsonStatisticsService had some serious concurrency issue and did not cache reports that it loaded from blob storage. This pull request fixes that by loading reports once an hour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To elaborate a little: all the Load... reports were merged into the Refresh method.


<p>
Show all packages:
<input type="checkbox" data-bind="checked: showAllPackageDownloads" />
Copy link
Member

Choose a reason for hiding this comment

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

The include prerelease checkbox on search and the per-package stats page both are linkable. Meaning clicking on them updates the URL in the address bar so people can link to the results. You should do this either with a GET form or replaceState in JavaScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. However, due to feedback this will no longer be linkable.

@joelverhagen
Copy link
Member

Do users really care about the old report? Is this driven by a specific customer ask? It is unclear from the linked issue if we should provide a way to toggle between them.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

⌚️
@anangaur's points are good. Let's close the loop here.

@anangaur
Copy link
Member

anangaur commented Nov 6, 2017

The filter (check box) should be enabled by default to filter out the Microsoft packages as that is what users want. It serves 2 purposes:

  1. Soft landing for users who are used to seeing the Microsoft packages in the list.
  2. Messaging: We need to tell what is it that we show on the page :)

@loic-sharma
Copy link
Contributor Author

loic-sharma commented Nov 7, 2017

Yes, non-community packages (including Microsoft packages) are filtered out by default. Talked to @anangaur offline, we cannot use "Hide Microsoft packages" as non-community packages include non-Microsoft packages such as .NET Framework System.* packages. We decided to go with "Show all packages" instead.

Addressed all other feedback.

IsDownloadPackageDetailAvailable = availablity[1].Loaded,
DownloadPackageVersionsSummary = _statisticsService.DownloadPackageVersionsSummary,
IsNuGetClientVersionAvailable = availablity[2].Loaded,
IsDownloadPackageAvailable = _statisticsService.DownloadCommunityPackagesResult.Loaded,
Copy link
Member

Choose a reason for hiding this comment

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

Whilst at it, should we rename the *Result properties to be a little more clear? e.g. DownloadCommunityPackagesResult > CommunityPackageDownloadsResult

Copy link
Member

Choose a reason for hiding this comment

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

Same for the Loaded property which currently sounds like an event: IsLoaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! :)

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.

5 participants