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

Report services are not using Blob storage SDK classes anymore. #8484

Merged
merged 6 commits into from
Apr 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
78 changes: 78 additions & 0 deletions src/NuGetGallery.Core/Services/SimpleCloudBlobExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// 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.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading.Tasks;
using Microsoft.WindowsAzure.Storage;

namespace NuGetGallery
{
public static class SimpleCloudBlobExtensions
{
agr marked this conversation as resolved.
Show resolved Hide resolved
agr marked this conversation as resolved.
Show resolved Hide resolved
/// <summary>
/// Retrieves the blob contents as a string assuming UTF8 encoding if the blob exists.
/// </summary>
/// <param name="blob">Blob reference.</param>
/// <returns>The text content of the blob or null if the blob does not exist.</returns>
public static async Task<string> DownloadTextIfExistAsync(this ISimpleCloudBlob blob)
agr marked this conversation as resolved.
Show resolved Hide resolved
agr marked this conversation as resolved.
Show resolved Hide resolved
{
using (var stream = new MemoryStream())
{
try
{
await blob.DownloadToStreamAsync(stream);
}
catch (StorageException e) when (IsNotFoundException(e))
{
return null;
}

return Encoding.UTF8.GetString(stream.ToArray());
}
}

/// <summary>
/// Calls <see cref="ISimpleCloudBlob.FetchAttributesAsync()"/> and determines if blob exists.
/// </summary>
/// <param name="blob">Blob reference</param>
/// <returns>True if <see cref="ISimpleCloudBlob.FetchAttributesAsync()"/> call succeeded, false if blob does not exist.</returns>
public static async Task<bool> FetchAttributesIfExistsAsync(this ISimpleCloudBlob blob)
{
try
{
await blob.FetchAttributesAsync();
}
catch (StorageException e) when (IsNotFoundException(e))
{
return false;
}
return true;
}

/// <summary>
/// Calls <see cref="ISimpleCloudBlob.OpenReadAsync(AccessCondition)"/> without access condition and returns
/// resulting stream if blob exists.
/// </summary>
/// <param name="blob">Blob reference.</param>
/// <returns>Stream if the call was successful, null if blob does not exist.</returns>
public static async Task<Stream> OpenReadIfExistAsync(this ISimpleCloudBlob blob)
{
try
{
return await blob.OpenReadAsync(accessCondition: null);
}
catch (StorageException e) when (IsNotFoundException(e))
{
return null;
}
}

private static bool IsNotFoundException(StorageException e)
=> ((e.InnerException as WebException)?.Response as HttpWebResponse)?.StatusCode == HttpStatusCode.NotFound;
}
}
2 changes: 1 addition & 1 deletion src/NuGetGallery/App_Start/AppActivator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration)
if (cloudDownloadCountService != null)
{
// Perform initial refresh + schedule new refreshes every 15 minutes
HostingEnvironment.QueueBackgroundWorkItem(cancellationToken => cloudDownloadCountService.Refresh());
HostingEnvironment.QueueBackgroundWorkItem(_ => cloudDownloadCountService.Refresh());
jobs.Add(new CloudDownloadCountServiceRefreshJob(TimeSpan.FromMinutes(15), cloudDownloadCountService));
}
}
Expand Down
63 changes: 40 additions & 23 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public static class BindingKeys

public const string PrimaryStatisticsKey = "PrimaryStatisticsKey";
public const string AlternateStatisticsKey = "AlternateStatisticsKey";
public const string FeatureFlaggedStatisticsKey = "FeatureFlaggedStatisticsKey";

public const string AccountDeleterTopic = "AccountDeleterBindingKey";
public const string PackageValidationTopic = "PackageValidationBindingKey";
Expand Down Expand Up @@ -705,7 +706,7 @@ private static void RegisterDeleteAccountService(ContainerBuilder builder, Confi
}
}

private static void RegisterStatisticsServices(ContainerBuilder builder, IGalleryConfigurationService configuration, ITelemetryService telemetryService)
private static void RegisterStatisticsServices(ContainerBuilder builder, IGalleryConfigurationService configuration)
{
// 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))
Expand All @@ -716,38 +717,54 @@ private static void RegisterStatisticsServices(ContainerBuilder builder, IGaller
.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.Register(c =>
{
var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
var alternateConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);
var featureFlagService = c.Resolve<IFeatureFlagService>();
var jsonAggregateStatsService = new JsonAggregateStatsService(featureFlagService, primaryConfiguration, alternateConfiguration);
return jsonAggregateStatsService;
})
.AsSelf()
{
var blobConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
return new CloudBlobClientWrapper(blobConfiguration.ConnectionString, blobConfiguration.ReadAccessGeoRedundant);
})
.SingleInstance()
.Keyed<ICloudBlobClient>(BindingKeys.PrimaryStatisticsKey);

builder.Register(c =>
{
var blobConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);
return new CloudBlobClientWrapper(blobConfiguration.ConnectionString, blobConfiguration.ReadAccessGeoRedundant);
})
.SingleInstance()
.Keyed<ICloudBlobClient>(BindingKeys.AlternateStatisticsKey);

var hasSecondaryStatisticsSource = !string.IsNullOrWhiteSpace(configuration.Current.AzureStorage_Statistics_ConnectionString_Alternate);

builder.Register(c =>
{
if (!hasSecondaryStatisticsSource)
{
return c.ResolveKeyed<ICloudBlobClient>(BindingKeys.PrimaryStatisticsKey);
}
if (c.Resolve<IFeatureFlagService>().IsAlternateStatisticsSourceEnabled())
agr marked this conversation as resolved.
Show resolved Hide resolved
{
return c.ResolveKeyed<ICloudBlobClient>(BindingKeys.AlternateStatisticsKey);
}
return c.ResolveKeyed<ICloudBlobClient>(BindingKeys.PrimaryStatisticsKey);
})
agr marked this conversation as resolved.
Show resolved Hide resolved
.Keyed<ICloudBlobClient>(BindingKeys.FeatureFlaggedStatisticsKey);

// when running on Windows Azure, we use a back-end job to calculate stats totals and store in the blobs
builder.Register(c => new JsonAggregateStatsService(c.ResolveKeyed<Func<ICloudBlobClient>>(BindingKeys.FeatureFlaggedStatisticsKey)))
agr marked this conversation as resolved.
Show resolved Hide resolved
.As<IAggregateStatsService>()
.SingleInstance();

// when running on Windows Azure, pull the statistics from the warehouse via storage
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;
})
builder.Register(c => new CloudReportService(c.ResolveKeyed<Func<ICloudBlobClient>>(BindingKeys.FeatureFlaggedStatisticsKey)))
.AsSelf()
.As<IReportService>()
.SingleInstance();

builder.Register(c =>
{
var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey);
var alternateConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.AlternateStatisticsKey);
var featureFlagService = c.Resolve<IFeatureFlagService>();
var downloadCountService = new CloudDownloadCountService(telemetryService, featureFlagService, primaryConfiguration, alternateConfiguration);
var cloudBlobClientFactory = c.ResolveKeyed<Func<ICloudBlobClient>>(BindingKeys.FeatureFlaggedStatisticsKey);
var telemetryService = c.Resolve<ITelemetryService>();
var downloadCountService = new CloudDownloadCountService(telemetryService, cloudBlobClientFactory);

var dlCountInterceptor = new DownloadCountObjectMaterializedInterceptor(downloadCountService, telemetryService);
ObjectMaterializedInterception.AddInterceptor(dlCountInterceptor);
Expand Down Expand Up @@ -1434,7 +1451,7 @@ private static void ConfigureForAzureStorage(ContainerBuilder builder, IGalleryC
}
}

RegisterStatisticsServices(builder, configuration, telemetryService);
RegisterStatisticsServices(builder, configuration);

builder.RegisterInstance(new TableErrorLog(configuration.Current.AzureStorage_Errors_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant))
.As<ErrorLog>()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public CloudDownloadCountServiceRefreshJob(TimeSpan interval, CloudDownloadCount

public override Task Execute()
{
return new Task(_downloadCountService.Refresh);
return _downloadCountService.Refresh();
Copy link
Contributor

@loic-sharma loic-sharma Apr 7, 2021

Choose a reason for hiding this comment

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

Can we make sure that all async method names have an Async suffix? Or would that be a huge change?

Also, please add an await here. See: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, our NuGetJobCoordinator complains if that Execute method returns a started task, so we have to wrap it in a new Task(), updated the code.

private static Task CreateWrappingContinuingTask(Task originalTask, Action<Task> continuationTask, TaskContinuationOptions options)
{
var continuation = originalTask.ContinueWith(continuationTask, options);
var wrapper = new Task(() =>
{
if (originalTask.Status == TaskStatus.WaitingForActivation || originalTask.Status == TaskStatus.Created)
{
originalTask.Start();
}
continuation.Wait();
});
return wrapper;
}
}

(line 53)

}
}
}
62 changes: 17 additions & 45 deletions src/NuGetGallery/Services/CloudDownloadCountService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
using System.IO;
using System.Linq;
using System.Net;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.WindowsAzure.Storage;
using Microsoft.WindowsAzure.Storage.Blob;
using Microsoft.WindowsAzure.Storage.RetryPolicies;
Expand All @@ -26,9 +28,7 @@ public class CloudDownloadCountService : IDownloadCountService
private const string TelemetryOriginForRefreshMethod = "CloudDownloadCountService.Refresh";

private readonly ITelemetryService _telemetryService;
private readonly IFeatureFlagService _featureFlagService;
private readonly IBlobStorageConfiguration _primaryStorageConfiguration;
private readonly IBlobStorageConfiguration _alternateBlobStorageConfiguration;
private readonly Func<ICloudBlobClient> _cloudBlobClientFactory;

private readonly object _refreshLock = new object();
private bool _isRefreshing;
Expand All @@ -38,14 +38,10 @@ private readonly ConcurrentDictionary<string, ConcurrentDictionary<string, int>>

public CloudDownloadCountService(
ITelemetryService telemetryService,
IFeatureFlagService featureFlagService,
IBlobStorageConfiguration primaryBlobStorageConfiguration,
IBlobStorageConfiguration alternateBlobStorageConfiguration)
Func<ICloudBlobClient> cloudBlobClientFactory)
{
_telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
_featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
_primaryStorageConfiguration = primaryBlobStorageConfiguration ?? throw new ArgumentNullException(nameof(primaryBlobStorageConfiguration));
_alternateBlobStorageConfiguration = alternateBlobStorageConfiguration;
_cloudBlobClientFactory = cloudBlobClientFactory ?? throw new ArgumentNullException(nameof(cloudBlobClientFactory));
}

public bool TryGetDownloadCountForPackageRegistration(string id, out int downloadCount)
Expand Down Expand Up @@ -91,7 +87,7 @@ public bool TryGetDownloadCountForPackage(string id, string version, out int dow
return false;
}

public void Refresh()
public async Task Refresh()
{
bool shouldRefresh = false;
lock (_refreshLock)
Expand All @@ -108,7 +104,7 @@ public void Refresh()
try
{
var stopwatch = Stopwatch.StartNew();
RefreshCore();
await RefreshCore();
stopwatch.Stop();
_telemetryService.TrackDownloadJsonRefreshDuration(stopwatch.ElapsedMilliseconds);

Expand Down Expand Up @@ -152,24 +148,19 @@ protected virtual int CalculateSum(ConcurrentDictionary<string, int> versions)
/// This method is added for unit testing purposes. It can return a null stream if the blob does not exist
/// and assumes the caller will properly dispose of the returned stream.
/// </summary>
agr marked this conversation as resolved.
Show resolved Hide resolved
protected virtual Stream GetBlobStream()
protected virtual async Task<Stream> GetBlobStream()
{
var blob = GetBlobReference();
if (blob == null)
{
return null;
}

return blob.OpenRead();
return await blob.OpenReadIfExistAsync();
}

private void RefreshCore()
private async Task RefreshCore()
{
try
{
// The data in downloads.v1.json will be an array of Package records - which has Id, Array of Versions and download count.
// Sample.json : [["AutofacContrib.NSubstitute",["2.4.3.700",406],["2.5.0",137]],["Assman.Core",["2.0.7",138]]....
using (var blobStream = GetBlobStream())
using (var blobStream = await GetBlobStream())
{
if (blobStream == null)
{
Expand All @@ -180,15 +171,15 @@ private void RefreshCore()
{
try
{
jsonReader.Read();
await jsonReader.ReadAsync();

while (jsonReader.Read())
while (await jsonReader.ReadAsync())
{
try
{
if (jsonReader.TokenType == JsonToken.StartArray)
{
JToken record = JToken.ReadFrom(jsonReader);
JToken record = await JToken.ReadFromAsync(jsonReader);
string id = record[0].ToString().ToLowerInvariant();

// The second entry in each record should be an array of versions, if not move on to next entry.
Expand Down Expand Up @@ -252,32 +243,13 @@ private void RefreshCore()
}
}

private CloudBlockBlob GetBlobReference()
private ISimpleCloudBlob GetBlobReference()
{
string connectionString = _primaryStorageConfiguration.ConnectionString;
bool 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)
{
blobClient.DefaultRequestOptions.LocationMode = LocationMode.PrimaryThenSecondary;
}
var blobClient = _cloudBlobClientFactory();

var container = blobClient.GetContainerReference(StatsContainerName);
var blob = container.GetBlockBlobReference(DownloadCountBlobName);
var blob = container.GetBlobReference(DownloadCountBlobName);

if (!blob.Exists())
{
return null;
}
return blob;
}
}
Expand Down
Loading