[Statistics]Adding Support for other reports from alternate container #8472
Conversation
ryuyu
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
drewgillies,
skofman1,
dannyjdev,
zhhyu and
lyndaidaii
| @@ -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 | |||
There was a problem hiding this comment.
This comment here looks a little confused, is it?
There was a problem hiding this comment.
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.
| _readAccessGeoRedundant = readAccessGeoRedundant; | ||
| _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService)); | ||
| _primaryStorageConfiguration = primaryBlobStorageConfiguration ?? throw new ArgumentNullException(nameof(primaryBlobStorageConfiguration)); | ||
| _alternateBlobStorageConfiguration = alternateBlobStorageConfiguration; |
There was a problem hiding this comment.
Will it be better if we also do the null check here?
There was a problem hiding this comment.
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.
| _readAccessGeoRedundant = readAccessGeoRedundant; | ||
| _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService)); | ||
| _primaryStorageConfiguration = primaryBlobStorageConfiguration ?? throw new ArgumentNullException(nameof(primaryBlobStorageConfiguration)); | ||
| _alternateBlobStorageConfiguration = alternateBlobStorageConfiguration; |
There was a problem hiding this comment.
Will it be better if we also do the null check here?
There was a problem hiding this comment.
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.
| builder.RegisterInstance(new JsonAggregateStatsService(configuration.Current.AzureStorage_Statistics_ConnectionString, configuration.Current.AzureStorageReadAccessGeoRedundant)) | ||
| builder.Register(c => | ||
| { | ||
| var primaryConfiguration = c.ResolveKeyed<IBlobStorageConfiguration>(BindingKeys.PrimaryStatisticsKey); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This looks like it could be good idea, and might make this block a bit easier to read. I'll experiment with it.
There was a problem hiding this comment.
I think we typically use WithParameter for this:
NuGetGallery/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
Lines 1407 to 1409 in be38792
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, we have two arguments of type IBlobStorageConfiguration here, so it won't work directly.
| var featureFlagService = c.Resolve<IFeatureFlagService>(); | ||
| var jsonAggregateStatsService = new JsonAggregateStatsService(featureFlagService, primaryConfiguration, alternateConfiguration); | ||
| return jsonAggregateStatsService; | ||
| }) | ||
| .AsSelf() |
There was a problem hiding this comment.
Do we need all those .AsSelf()? Does any service directly depend on JsonAggregateStatsService?
Adds support for sourcing other reports from the alternate stats container.
Currently, feature flagging is all-or-nothing.