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.

Sign up for GitHub

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]Adding Support for other reports from alternate container #8472

Merged
merged 1 commit into from
Apr 1, 2021
Merged

[Statistics]Adding Support for other reports from alternate container #8472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,27 +707,41 @@ private static void RegisterDeleteAccountService(ContainerBuilder builder, Confi

private static void RegisterStatisticsServices(ContainerBuilder builder, IGalleryConfigurationService configuration, ITelemetryService telemetryService)
{
// when running on Windows Azure, download counts come from the downloads.v1.json blob
Copy link
Contributor

@zhhyu zhhyu Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This comment here looks a little confused, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment was around this block when i initially made code changes. I'm not sure where it went, but it's back here. I imagine this was originally here to note the difference from running locally, where stats come from DB.

builder.Register(c => new SimpleBlobStorageConfiguration(configuration.Current.AzureStorage_Statistics_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant))
.SingleInstance()
.Keyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);

builder.Register(c => new SimpleBlobStorageConfiguration(configuration.Current.AzureStorage_Statistics_ConnectionString_Alternate, configuration.Current.AzureStorageReadAccessGeoRedundant))
.SingleInstance()
.Keyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);

// when running on Windows Azure, we use a back-end job to calculate stats totals and store in the blobs
builder.RegisterInstance(new JsonAggregateStatsService(configuration.Current.AzureStorage_Statistics_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant))
builder.Register(c =>
{
var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

primaryConfiguration [](start = 20, length = 20)

What do you think about using attributes for resolving keyed dependencies: https://autofaccn.readthedocs.io/en/latest/advanced/keyed-services.html#resolving-with-attributes ?
We might be able to eliminate the creation of JsonAggregateStatsService class here and simply register it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it could be good idea, and might make this block a bit easier to read. I'll experiment with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we typically use WithParameter for this:

NuGetGallery/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

Lines 1407 to 1409 in be38792

.WithParameter(new ResolvedParameter(
(pi, ctx) => pi.ParameterType.IsAssignableFrom(typeof(IFileStorageService)),
(pi, ctx) => ctx.ResolveKeyed<IFileStorageService>(dependent.BindingKey)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a little trouble getting this to work right now. I've filed #8477 to make this change after things are in place (if I am understanding this correctly, we should be able to make this change independent of the overall functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we have two arguments of type IBlobStorageConfiguration here, so it won't work directly.

var alternateConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);
var featureFlagService = c.Resolve<IFeatureFlagService>();
var jsonAggregateStatsService = new JsonAggregateStatsService(featureFlagService, primaryConfiguration, alternateConfiguration);
return jsonAggregateStatsService;
})
.AsSelf()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all those .AsSelf()? Does any service directly depend on JsonAggregateStatsService?

.As<IAggregateStatsService>()
.SingleInstance();

// when running on Windows Azure, pull the statistics from the warehouse via storage
builder.RegisterInstance(new CloudReportService(configuration.Current.AzureStorage_Statistics_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant))
builder.Register(c =>
{
var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
var alternateConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);
var featureFlagService = c.Resolve<IFeatureFlagService>();
var cloudReportService = new CloudReportService(featureFlagService, primaryConfiguration, alternateConfiguration);
return cloudReportService;
})
.AsSelf()
.As<IReportService>()
.SingleInstance();

// when running on Windows Azure, download counts come from the downloads.v1.json blob
builder.Register(c => new SimpleBlobStorageConfiguration(configuration.Current.AzureStorage_Statistics_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant))
.SingleInstance()
.Keyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);

builder.Register(c => new SimpleBlobStorageConfiguration(configuration.Current.AzureStorage_Statistics_ConnectionString_Alternate, configuration.Current.AzureStorageReadAccessGeoRedundant))
.SingleInstance()
.Keyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);

builder.Register(c =>
{
var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
Expand Down
30 changes: 23 additions & 7 deletions src/NuGetGallery/Services/CloudReportService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,30 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Threading.Tasks;
using Microsoft.WindowsAzure.Storage;
using Microsoft.WindowsAzure.Storage.Blob;
using Microsoft.WindowsAzure.Storage.RetryPolicies;
using NuGetGallery.Services;

namespace NuGetGallery
{
public class CloudReportService : IReportService
{
private const string _statsContainerName = "nuget-cdnstats";
private readonly string _connectionString;
private readonly bool _readAccessGeoRedundant;
private readonly IFeatureFlagService _featureFlagService;
private readonly IBlobStorageConfiguration _primaryStorageConfiguration;
private readonly IBlobStorageConfiguration _alternateBlobStorageConfiguration;

public CloudReportService(string connectionString, bool readAccessGeoRedundant)
public CloudReportService(
IFeatureFlagService featureFlagService,
IBlobStorageConfiguration primaryBlobStorageConfiguration,
IBlobStorageConfiguration alternateBlobStorageConfiguration)
{
_connectionString = connectionString;
_readAccessGeoRedundant = readAccessGeoRedundant;
_featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
_primaryStorageConfiguration = primaryBlobStorageConfiguration ?? throw new ArgumentNullException(nameof(primaryBlobStorageConfiguration));
_alternateBlobStorageConfiguration = alternateBlobStorageConfiguration;
Copy link
Contributor

@zhhyu zhhyu Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Will it be better if we also do the null check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did no null check intentionally to not force alternate blob storage configuration to have to be set so that it works in place with current config.

}

public async Task<StatisticsReport> Load(string reportName)
Expand All @@ -42,10 +49,19 @@ public async Task<StatisticsReport> Load(string reportName)

private CloudBlobContainer GetCloudBlobContainer()
{
var storageAccount = CloudStorageAccount.Parse(_connectionString);
var connectionString = _primaryStorageConfiguration.ConnectionString;
var readAccessGeoRedundant = _primaryStorageConfiguration.ReadAccessGeoRedundant;

if(_alternateBlobStorageConfiguration != null && _featureFlagService.IsAlternateStatisticsSourceEnabled())
{
connectionString = _alternateBlobStorageConfiguration.ConnectionString;
readAccessGeoRedundant = _alternateBlobStorageConfiguration.ReadAccessGeoRedundant;
}

var storageAccount = CloudStorageAccount.Parse(connectionString);
var blobClient = storageAccount.CreateCloudBlobClient();

if (_readAccessGeoRedundant)
if (readAccessGeoRedundant)
{
blobClient.DefaultRequestOptions.LocationMode = LocationMode.PrimaryThenSecondary;
}
Expand Down
30 changes: 23 additions & 7 deletions src/NuGetGallery/Services/JsonAggregateStatsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,47 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Threading.Tasks;
using Microsoft.WindowsAzure.Storage;
using Microsoft.WindowsAzure.Storage.Blob;
using Microsoft.WindowsAzure.Storage.RetryPolicies;
using Newtonsoft.Json;
using NuGetGallery.Services;

namespace NuGetGallery
{
public class JsonAggregateStatsService : IAggregateStatsService
{
private readonly string _connectionString;
private readonly bool _readAccessGeoRedundant;
private readonly IFeatureFlagService _featureFlagService;
private readonly IBlobStorageConfiguration _primaryStorageConfiguration;
private readonly IBlobStorageConfiguration _alternateBlobStorageConfiguration;

public JsonAggregateStatsService(string connectionString, bool readAccessGeoRedundant)
public JsonAggregateStatsService(
IFeatureFlagService featureFlagService,
IBlobStorageConfiguration primaryBlobStorageConfiguration,
IBlobStorageConfiguration alternateBlobStorageConfiguration)
{
_connectionString = connectionString;
_readAccessGeoRedundant = readAccessGeoRedundant;
_featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
_primaryStorageConfiguration = primaryBlobStorageConfiguration ?? throw new ArgumentNullException(nameof(primaryBlobStorageConfiguration));
_alternateBlobStorageConfiguration = alternateBlobStorageConfiguration;
Copy link
Contributor

@zhhyu zhhyu Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Will it be better if we also do the null check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did no null check intentionally to not force alternate blob storage configuration to have to be set so that it works in place with current config.

}

public async Task<AggregateStats> GetAggregateStats()
{
CloudStorageAccount storageAccount = CloudStorageAccount.Parse(_connectionString);
var connectionString = _primaryStorageConfiguration.ConnectionString;
var readAccessGeoRedundant = _primaryStorageConfiguration.ReadAccessGeoRedundant;

if (_alternateBlobStorageConfiguration != null && _featureFlagService.IsAlternateStatisticsSourceEnabled())
{
connectionString = _alternateBlobStorageConfiguration.ConnectionString;
readAccessGeoRedundant = _alternateBlobStorageConfiguration.ReadAccessGeoRedundant;
}

CloudStorageAccount storageAccount = CloudStorageAccount.Parse(connectionString);
CloudBlobClient blobClient = storageAccount.CreateCloudBlobClient();

if (_readAccessGeoRedundant)
if (readAccessGeoRedundant)
{
blobClient.DefaultRequestOptions.LocationMode = LocationMode.PrimaryThenSecondary;
}
Expand Down