From f2a8898e31f2cac04477bc01d563481ecb1dbf11 Mon Sep 17 00:00:00 2001 From: Duncan Watson Date: Mon, 10 Mar 2025 17:07:37 +0000 Subject: [PATCH 1/2] EES-5826 - reponding to PR comments. Using Channels to queue up requests for writing queries for analytics, rather than not awaiting the writing process. --- .../Services/AnalyticsServiceTests.cs | 66 ------------------- .../Services/QueryAnalyticsWriterTests.cs | 66 +++++++++++++++++++ ...aSetVersionQueryTests.Success.Query1.snap} | 0 ...aSetVersionQueryTests.Success.Query2.snap} | 0 .../Services/DataSetQueryService.cs | 29 ++++---- .../Services/Interfaces/IAnalyticsService.cs | 18 ----- .../Interfaces/IQueryAnalyticsChannel.cs | 8 +++ .../Interfaces/IQueryAnalyticsWriter.cs | 6 ++ .../Services/QueryAnalyticsChannel.cs | 20 ++++++ .../Services/QueryAnalyticsConsumer.cs | 19 ++++++ ...ticsService.cs => QueryAnalyticsWriter.cs} | 37 +++-------- .../Startup.cs | 19 ++++-- 12 files changed, 154 insertions(+), 134 deletions(-) delete mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/AnalyticsServiceTests.cs create mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/QueryAnalyticsWriterTests.cs rename src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/{AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query1.snap => QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query1.snap} (100%) rename src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/{AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query2.snap => QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query2.snap} (100%) delete mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IAnalyticsService.cs create mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs create mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsWriter.cs create mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs create mode 100644 src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs rename src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/{AnalyticsService.cs => QueryAnalyticsWriter.cs} (59%) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/AnalyticsServiceTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/AnalyticsServiceTests.cs deleted file mode 100644 index 02df17660e..0000000000 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/AnalyticsServiceTests.cs +++ /dev/null @@ -1,66 +0,0 @@ -using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; -using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; -using Microsoft.Extensions.Logging; -using Moq; -using Snapshooter.Xunit; - -namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests.Services; - -public abstract class AnalyticsServiceTests -{ - public class ReportDataSetVersionQuery : AnalyticsServiceTests - { - private const string SnapshotPrefix = $"{nameof(AnalyticsServiceTests)}.{nameof(ReportDataSetVersionQuery)}"; - - [Fact] - public async Task Success() - { - var pathResolver = new TestAnalyticsPathResolver(); - var service = BuildService(pathResolver); - - await service.ReportDataSetVersionQuery( - dataSetId: new Guid("acb97c97-89c9-4b74-88e7-39c27f6bab63"), - dataSetVersionId: new Guid("0da7c640-80a8-44e2-8028-fc529bcedcb1"), - semVersion: "2.3.1", - dataSetTitle: "Data Set Title", - query: DataSetQueryRequestTestData.NestedQuery1, - resultsCount: 55, - totalRowsCount: 5100, - startTime: DateTime.Parse("2025-02-20T12:00:00.000Z"), - endTime: DateTime.Parse("2025-02-20T12:00:10.234Z")); - - await service.ReportDataSetVersionQuery( - dataSetId: new Guid("72e16c8c-2dc0-4063-bdf6-ee52bd127ebe"), - dataSetVersionId: new Guid("bb68fd95-1231-498c-8858-b061d739ae17"), - semVersion: "3.0.0", - dataSetTitle: "Data Set Title 2", - query: DataSetQueryRequestTestData.NestedQuery2, - resultsCount: 120, - totalRowsCount: 10000, - startTime: DateTime.Parse("2025-02-20T01:00:00.000Z"), - endTime: DateTime.Parse("2025-02-20T01:00:10.999Z")); - - var files = Directory - .GetFiles(pathResolver.PublicApiQueriesDirectoryPath()) - .Order() - .ToList(); - - Assert.Equal(2, files.Count); - - var file1Contents = await File.ReadAllTextAsync(files[0]); - Snapshot.Match( - currentResult: file1Contents, - snapshotName: $"{SnapshotPrefix}.{nameof(Success)}.Query1"); - - var file2Contents = await File.ReadAllTextAsync(files[1]); - Snapshot.Match( - currentResult: file2Contents, - snapshotName: $"{SnapshotPrefix}.{nameof(Success)}.Query2"); - } - - private static AnalyticsService BuildService(IAnalyticsPathResolver pathResolver) - { - return new(pathResolver, Mock.Of>()); - } - } -} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/QueryAnalyticsWriterTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/QueryAnalyticsWriterTests.cs new file mode 100644 index 0000000000..84bc908ca5 --- /dev/null +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/QueryAnalyticsWriterTests.cs @@ -0,0 +1,66 @@ +using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; +using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; +using Microsoft.Extensions.Logging; +using Moq; +using Snapshooter.Xunit; + +namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests.Services; + +public abstract class QueryAnalyticsWriterTests +{ + public class ReportDataSetVersionQueryTests : QueryAnalyticsWriterTests + { + private const string SnapshotPrefix = $"{nameof(QueryAnalyticsWriterTests)}.{nameof(ReportDataSetVersionQueryTests)}"; + + [Fact] + public async Task Success() + { + var pathResolver = new TestAnalyticsPathResolver(); + var service = BuildService(pathResolver); + + await service.ReportDataSetVersionQuery(new CaptureDataSetVersionQueryRequest( + DataSetId: new Guid("acb97c97-89c9-4b74-88e7-39c27f6bab63"), + DataSetVersionId: new Guid("0da7c640-80a8-44e2-8028-fc529bcedcb1"), + DataSetVersion: "2.3.1", + DataSetTitle: "Data Set Title", + Query: DataSetQueryRequestTestData.NestedQuery1, + ResultsCount: 55, + TotalRowsCount: 5100, + StartTime: DateTime.Parse("2025-02-20T12:00:00.000Z"), + EndTime: DateTime.Parse("2025-02-20T12:00:10.234Z"))); + + await service.ReportDataSetVersionQuery(new CaptureDataSetVersionQueryRequest( + DataSetId: new Guid("72e16c8c-2dc0-4063-bdf6-ee52bd127ebe"), + DataSetVersionId: new Guid("bb68fd95-1231-498c-8858-b061d739ae17"), + DataSetVersion: "3.0.0", + DataSetTitle: "Data Set Title 2", + Query: DataSetQueryRequestTestData.NestedQuery2, + ResultsCount: 120, + TotalRowsCount: 10000, + StartTime: DateTime.Parse("2025-02-20T01:00:00.000Z"), + EndTime: DateTime.Parse("2025-02-20T01:00:10.999Z"))); + + var files = Directory + .GetFiles(pathResolver.PublicApiQueriesDirectoryPath()) + .Order() + .ToList(); + + Assert.Equal(2, files.Count); + + var file1Contents = await File.ReadAllTextAsync(files[0]); + Snapshot.Match( + currentResult: file1Contents, + snapshotName: $"{SnapshotPrefix}.{nameof(Success)}.Query1"); + + var file2Contents = await File.ReadAllTextAsync(files[1]); + Snapshot.Match( + currentResult: file2Contents, + snapshotName: $"{SnapshotPrefix}.{nameof(Success)}.Query2"); + } + + private static QueryAnalyticsWriter BuildService(IAnalyticsPathResolver pathResolver) + { + return new(pathResolver, Mock.Of>()); + } + } +} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query1.snap b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query1.snap similarity index 100% rename from src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query1.snap rename to src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query1.snap diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query2.snap b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query2.snap similarity index 100% rename from src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/AnalyticsServiceTests.ReportDataSetVersionQuery.Success.Query2.snap rename to src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Services/__snapshots__/QueryAnalyticsWriterTests.ReportDataSetVersionQueryTests.Success.Query2.snap diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs index f884ccf682..97524fc117 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs @@ -39,7 +39,7 @@ internal class DataSetQueryService( IParquetIndicatorRepository indicatorRepository, IParquetLocationRepository locationRepository, IParquetTimePeriodRepository timePeriodRepository, - IAnalyticsService analyticsService) + IQueryAnalyticsChannel queryAnalyticsChannel) : IDataSetQueryService { private static readonly Dictionary ReservedSorts = new() @@ -149,21 +149,18 @@ private async Task> query: query, cancellationToken: cancellationToken, baseCriteriaPath: baseCriteriaPath) - .OnSuccessDo(results => - { - // Deliberately do not await this operation as we do not want to - // delay the return of the query to the end user. - _ = analyticsService.ReportDataSetVersionQuery( - dataSetId: dataSetVersion.DataSetId, - dataSetVersionId: dataSetVersion.Id, - semVersion: dataSetVersion.SemVersion().ToString(), - dataSetTitle: dataSetVersion.DataSet.Title, - query: query, - resultsCount: results.Results.Count, - totalRowsCount: results.Paging.TotalResults, - startTime: startTime, - endTime: DateTime.UtcNow); - }); + .OnSuccessDo(results => queryAnalyticsChannel.WriteQuery( + request: new CaptureDataSetVersionQueryRequest( + DataSetId: dataSetVersion.DataSetId, + DataSetVersionId: dataSetVersion.Id, + DataSetVersion: dataSetVersion.SemVersion().ToString(), + DataSetTitle: dataSetVersion.DataSet.Title, + Query: query, + ResultsCount: results.Results.Count, + TotalRowsCount: results.Paging.TotalResults, + StartTime: startTime, + EndTime: DateTime.UtcNow), + cancellationToken: cancellationToken)); } private async Task> RunQuery( diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IAnalyticsService.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IAnalyticsService.cs deleted file mode 100644 index 347da7395e..0000000000 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IAnalyticsService.cs +++ /dev/null @@ -1,18 +0,0 @@ -using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Requests; -using GovUk.Education.ExploreEducationStatistics.Public.Data.Model; - -namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; - -public interface IAnalyticsService -{ - Task ReportDataSetVersionQuery( - Guid dataSetId, - Guid dataSetVersionId, - string semVersion, - string dataSetTitle, - DataSetQueryRequest query, - int resultsCount, - int totalRowsCount, - DateTime startTime, - DateTime endTime); -} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs new file mode 100644 index 0000000000..f226f6fe01 --- /dev/null +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs @@ -0,0 +1,8 @@ +namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; + +public interface IQueryAnalyticsChannel +{ + Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken); + + ValueTask ReadQuery(CancellationToken cancellationToken); +} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsWriter.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsWriter.cs new file mode 100644 index 0000000000..59e0548fd4 --- /dev/null +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsWriter.cs @@ -0,0 +1,6 @@ +namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; + +public interface IQueryAnalyticsWriter +{ + Task ReportDataSetVersionQuery(CaptureDataSetVersionQueryRequest request); +} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs new file mode 100644 index 0000000000..a9ca138a9b --- /dev/null +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs @@ -0,0 +1,20 @@ +using System.Threading.Channels; +using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; + +namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; + +public class QueryAnalyticsChannel : IQueryAnalyticsChannel +{ + private readonly Channel _channel = + Channel.CreateUnbounded(); + + public async Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) + { + await _channel.Writer.WriteAsync(request, cancellationToken); + } + + public ValueTask ReadQuery(CancellationToken cancellationToken) + { + return _channel.Reader.ReadAsync(cancellationToken); + } +} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs new file mode 100644 index 0000000000..a7815591d7 --- /dev/null +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs @@ -0,0 +1,19 @@ +using System.Threading.Channels; +using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; + +namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; + +public class QueryAnalyticsConsumer( + IQueryAnalyticsChannel channel, + IQueryAnalyticsWriter queryAnalyticsWriter + ) : BackgroundService +{ + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + while (!stoppingToken.IsCancellationRequested) + { + var message = await channel.ReadQuery(stoppingToken); + await queryAnalyticsWriter.ReportDataSetVersionQuery(message); + } + } +} diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/AnalyticsService.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs similarity index 59% rename from src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/AnalyticsService.cs rename to src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs index 5184a9d6c7..bc500c3ccf 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/AnalyticsService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs @@ -7,38 +7,21 @@ namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; -public class AnalyticsService( +public class QueryAnalyticsWriter( IAnalyticsPathResolver analyticsPathResolver, - ILogger logger) : IAnalyticsService + ILogger logger) : IQueryAnalyticsWriter { - public async Task ReportDataSetVersionQuery( - Guid dataSetId, - Guid dataSetVersionId, - string semVersion, - string dataSetTitle, - DataSetQueryRequest query, - int resultsCount, - int totalRowsCount, - DateTime startTime, - DateTime endTime) + public async Task ReportDataSetVersionQuery(CaptureDataSetVersionQueryRequest request) { logger.LogInformation( "Capturing query for analytics for data set {DataSetTitle}", - dataSetTitle); - - var request = new CaptureDataSetVersionQueryRequest( - DataSetId: dataSetId, - DataSetVersionId: dataSetVersionId, - DataSetVersion: semVersion, - DataSetTitle: dataSetTitle, - ResultsCount: resultsCount, - TotalRowsCount: totalRowsCount, - StartTime: startTime, - EndTime: endTime, - Query: DataSetQueryNormalisationUtil.NormaliseQuery(query)); + request.DataSetTitle); var serialisedRequest = JsonSerializationUtils.Serialize( - obj: request, + obj: request with + { + Query = DataSetQueryNormalisationUtil.NormaliseQuery(request.Query) + }, formatting: Formatting.Indented, orderedProperties: true, camelCase: true); @@ -47,7 +30,7 @@ public async Task ReportDataSetVersionQuery( Directory.CreateDirectory(directory); - var filename = $"{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}_{dataSetVersionId}.json"; + var filename = $"{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}_{request.DataSetVersionId}.json"; await File.WriteAllTextAsync( Path.Combine(directory, filename), @@ -55,7 +38,7 @@ await File.WriteAllTextAsync( } } -internal record CaptureDataSetVersionQueryRequest( +public record CaptureDataSetVersionQueryRequest( Guid DataSetId, Guid DataSetVersionId, string DataSetVersion, diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs index 275136b0a9..4da54653c6 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs @@ -256,12 +256,14 @@ public void ConfigureServices(IServiceCollection services) if (_analyticsOptions.Enabled) { - services.AddScoped(); + services.AddSingleton(); + services.AddHostedService(); services.AddSingleton(); + services.AddSingleton(); } else { - services.AddScoped(); + services.AddSingleton(); } } @@ -373,13 +375,16 @@ private static void ApplyCustomMigrations(IApplicationBuilder app) migrations.ForEach(migration => migration.Apply()); } - private class NoOpAnalyticsService : IAnalyticsService + private class NoopQueryAnalyticsChannel : IQueryAnalyticsChannel { - public Task ReportDataSetVersionQuery(Guid dataSetId, Guid dataSetVersionId, string semVersion, - string dataSetTitle, - DataSetQueryRequest query, int resultsCount, int totalRowsCount, DateTime startTime, DateTime endTime) + public Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) { - return Task.CompletedTask; + return Task.CompletedTask; + } + + public ValueTask ReadQuery(CancellationToken cancellationToken) + { + return default; } } } From 8450ac3d62e7c66982a6a7af794f55c3b02c3ce7 Mon Sep 17 00:00:00 2001 From: Duncan Watson Date: Thu, 13 Mar 2025 10:57:41 +0000 Subject: [PATCH 2/2] EES-5826 - various changes in response to PR comments. Added guards and logging around the producing and consuming of queries via the Channel implementation in QueryAnalyticsManager (renamed form QueryAnalyticsChannel to encapsulate implementation). Added additional tests to ensure that failure to write to QueryAnalyticsManager does not prevent thee successful return of API query results. --- .../DataSetsControllerGetQueryTests.cs | 52 +++++++++++++++- .../DataSetsControllerPostQueryTests.cs | 60 ++++++++++++++++++- .../Fixture/TestApplicationFactory.cs | 3 +- .../Services/DataSetQueryService.cs | 40 +++++++++---- ...csChannel.cs => IQueryAnalyticsManager.cs} | 4 +- .../Services/QueryAnalyticsConsumer.cs | 22 +++++-- ...icsChannel.cs => QueryAnalyticsManager.cs} | 4 +- .../Services/QueryAnalyticsWriter.cs | 23 +++++-- .../Startup.cs | 13 ++-- 9 files changed, 181 insertions(+), 40 deletions(-) rename src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/{IQueryAnalyticsChannel.cs => IQueryAnalyticsManager.cs} (58%) rename src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/{QueryAnalyticsChannel.cs => QueryAnalyticsManager.cs} (77%) diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerGetQueryTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerGetQueryTests.cs index 6953a436ba..6f19e7f17a 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerGetQueryTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerGetQueryTests.cs @@ -3,6 +3,7 @@ using GovUk.Education.ExploreEducationStatistics.Common.Model; using GovUk.Education.ExploreEducationStatistics.Common.Model.Data; using GovUk.Education.ExploreEducationStatistics.Common.Tests.Extensions; +using GovUk.Education.ExploreEducationStatistics.Common.Tests.Fixtures; using GovUk.Education.ExploreEducationStatistics.Common.Utils; using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Requests; using GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; @@ -23,7 +24,9 @@ using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Primitives; +using Moq; using static GovUk.Education.ExploreEducationStatistics.Common.Services.CollectionUtils; +using static GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils.MockUtils; namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests.Controllers; @@ -2951,6 +2954,50 @@ public async Task UnsuccessfulQuery_NotCapturedByAnalytics() Assert.False(Directory.Exists(_analyticsPathResolver.PublicApiQueriesDirectoryPath())); } } + + public class QueryAnalyticsExceptionTests(TestApplicationFactory testApp) + : DataSetsControllerGetQueryTests(testApp) + { + [Fact] + public async Task ExceptionThrownByQueryAnalyticsManager_SuccessfulResultsStillReturned() + { + // Set up the manager to throw an exception when the service attempts to add a query to it. + var analyticsManagerMock = new Mock(MockBehavior.Strict); + analyticsManagerMock + .Setup(m => m.AddQuery( + It.IsAny(), + It.IsAny())) + .Throws(new Exception("Error")); + + var app = TestApp.ConfigureServices(services => services + .ReplaceService(_dataSetVersionPathResolver) + .ReplaceService(analyticsManagerMock)); + + var dataSetVersion = await SetupDefaultDataSetVersion(); + + var response = await QueryDataSet( + app: app, + dataSetId: dataSetVersion.DataSetId, + indicators: [AbsenceSchoolData.IndicatorEnrolments], + queryParameters: new Dictionary + { + { "filters.eq", AbsenceSchoolData.FilterSchoolTypeTotal }, + { "geographicLevels.eq", "NAT" }, + { "timePeriods.eq", "2020/2021|AY" }, + { "locations.eq", $"NAT|id|{AbsenceSchoolData.LocationNatEngland}" } + }, + sorts: ["timePeriod|Asc"] + ); + + // Verify that the manager threw the Exception as planned. + VerifyAllMocks(analyticsManagerMock); + + // Verify that despite the Exception being thrown, the service still returned + // the expected successful query. + var viewModel = response.AssertOk(useSystemJson: true); + Assert.Equal(4, viewModel.Results.Count); + } + } public class QueryAnalyticsDisabledTests(TestApplicationFactory testApp) : DataSetsControllerGetQueryTests(testApp) { @@ -3002,7 +3049,8 @@ private async Task QueryDataSet( IEnumerable? sorts = null, bool? debug = null, IDictionary? queryParameters = null, - Guid? previewTokenId = null) + Guid? previewTokenId = null, + WebApplicationFactory? app = null) { var query = new Dictionary { @@ -3039,7 +3087,7 @@ private async Task QueryDataSet( query.AddRange(queryParameters); } - var client = BuildApp().CreateClient(); + var client = (app ?? BuildApp()).CreateClient(); client.AddPreviewTokenHeader(previewTokenId); var uri = QueryHelpers.AddQueryString($"{BaseUrl}/{dataSetId}/query", query); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerPostQueryTests.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerPostQueryTests.cs index 342326f237..9a06c55ab9 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerPostQueryTests.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Controllers/DataSetsControllerPostQueryTests.cs @@ -24,7 +24,9 @@ using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Primitives; +using Moq; using static GovUk.Education.ExploreEducationStatistics.Common.Services.CollectionUtils; +using static GovUk.Education.ExploreEducationStatistics.Common.Tests.Utils.MockUtils; namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests.Controllers; @@ -3984,11 +3986,65 @@ public async Task SuccessfulQuery_AnalyticsDisabled_NotCapturedByAnalytics() } } + public class QueryAnalyticsExceptionTests(TestApplicationFactory testApp) : DataSetsControllerPostQueryTests(testApp) + { + [Fact] + public async Task ExceptionThrownByQueryAnalyticsManager_SuccessfulResultsStillReturned() + { + // Set up the manager to throw an exception when the service attempts to add a query to it. + var analyticsManagerMock = new Mock(MockBehavior.Strict); + analyticsManagerMock + .Setup(m => m.AddQuery( + It.IsAny(), + It.IsAny())) + .Throws(new Exception("Error")); + + var app = TestApp.ConfigureServices(services => services + .ReplaceService(_dataSetVersionPathResolver) + .ReplaceService(analyticsManagerMock)); + + var dataSetVersion = await SetupDefaultDataSetVersion(); + + var request = new DataSetQueryRequest + { + Indicators = ListOf(AbsenceSchoolData.IndicatorEnrolments), + Criteria = new DataSetQueryCriteriaFacets + { + Filters = new DataSetQueryCriteriaFilters { Eq = AbsenceSchoolData.FilterSchoolTypeTotal }, + GeographicLevels = new DataSetQueryCriteriaGeographicLevels { Eq = "NAT" }, + TimePeriods = + new DataSetQueryCriteriaTimePeriods + { + Eq = new DataSetQueryTimePeriod { Code = "AY", Period = "2020/2021" } + }, + Locations = new DataSetQueryCriteriaLocations + { + Eq = new DataSetQueryLocationId { Id = AbsenceSchoolData.LocationNatEngland, Level = "NAT" } + } + }, + Sorts = ListOf(new DataSetQuerySort { Direction = "Asc", Field = "timePeriod" }) + }; + + var response = await QueryDataSet( + app: app, + dataSetId: dataSetVersion.DataSetId, + request: request); + + // Assert that the mock threw an exception as expected. + VerifyAllMocks(analyticsManagerMock); + + // Assert that the result was still returned despite the above exception. + var viewModel = response.AssertOk(useSystemJson: true); + Assert.Equal(4, viewModel.Results.Count); + } + } + private async Task QueryDataSet( Guid dataSetId, DataSetQueryRequest request, string? dataSetVersion = null, - Guid? previewTokenId = null) + Guid? previewTokenId = null, + WebApplicationFactory? app = null) { var query = new Dictionary(); @@ -3997,7 +4053,7 @@ private async Task QueryDataSet( query["dataSetVersion"] = dataSetVersion; } - var client = BuildApp().CreateClient(); + var client = (app ?? BuildApp()).CreateClient(); client.AddPreviewTokenHeader(previewTokenId); var uri = QueryHelpers.AddQueryString($"{BaseUrl}/{dataSetId}/query", query); diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Fixture/TestApplicationFactory.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Fixture/TestApplicationFactory.cs index 9834bc066b..cb64df68b0 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Fixture/TestApplicationFactory.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Tests/Fixture/TestApplicationFactory.cs @@ -47,9 +47,10 @@ public async Task ClearAllTestData() await ClearTestData(); } - public void AddAppSettings(string filename) + public TestApplicationFactory AddAppSettings(string filename) { _additionalAppsettingsFiles.Add(filename); + return this; } protected override IHostBuilder CreateHostBuilder() diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs index 97524fc117..b69a42a316 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/DataSetQueryService.cs @@ -39,7 +39,8 @@ internal class DataSetQueryService( IParquetIndicatorRepository indicatorRepository, IParquetLocationRepository locationRepository, IParquetTimePeriodRepository timePeriodRepository, - IQueryAnalyticsChannel queryAnalyticsChannel) + IQueryAnalyticsManager queryAnalyticsManager, + ILogger logger) : IDataSetQueryService { private static readonly Dictionary ReservedSorts = new() @@ -149,18 +150,31 @@ private async Task> query: query, cancellationToken: cancellationToken, baseCriteriaPath: baseCriteriaPath) - .OnSuccessDo(results => queryAnalyticsChannel.WriteQuery( - request: new CaptureDataSetVersionQueryRequest( - DataSetId: dataSetVersion.DataSetId, - DataSetVersionId: dataSetVersion.Id, - DataSetVersion: dataSetVersion.SemVersion().ToString(), - DataSetTitle: dataSetVersion.DataSet.Title, - Query: query, - ResultsCount: results.Results.Count, - TotalRowsCount: results.Paging.TotalResults, - StartTime: startTime, - EndTime: DateTime.UtcNow), - cancellationToken: cancellationToken)); + .OnSuccessDo(async results => + { + try + { + await queryAnalyticsManager.AddQuery( + request: new CaptureDataSetVersionQueryRequest( + DataSetId: dataSetVersion.DataSetId, + DataSetVersionId: dataSetVersion.Id, + DataSetVersion: dataSetVersion.SemVersion().ToString(), + DataSetTitle: dataSetVersion.DataSet.Title, + Query: query, + ResultsCount: results.Results.Count, + TotalRowsCount: results.Paging.TotalResults, + StartTime: startTime, + EndTime: DateTime.UtcNow), + cancellationToken: cancellationToken); + } + catch (Exception e) + { + logger.LogError( + exception: e, + message: "Error whilst adding a query to {QueryManager}", + nameof(IQueryAnalyticsManager)); + } + }); } private async Task> RunQuery( diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsManager.cs similarity index 58% rename from src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs rename to src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsManager.cs index f226f6fe01..74a0639804 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsChannel.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/Interfaces/IQueryAnalyticsManager.cs @@ -1,8 +1,8 @@ namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services.Interfaces; -public interface IQueryAnalyticsChannel +public interface IQueryAnalyticsManager { - Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken); + Task AddQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken); ValueTask ReadQuery(CancellationToken cancellationToken); } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs index a7815591d7..69ba2f4ad0 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsConsumer.cs @@ -4,16 +4,26 @@ namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; public class QueryAnalyticsConsumer( - IQueryAnalyticsChannel channel, - IQueryAnalyticsWriter queryAnalyticsWriter - ) : BackgroundService + IQueryAnalyticsManager manager, + IQueryAnalyticsWriter queryAnalyticsWriter, + ILogger logger) : BackgroundService { protected override async Task ExecuteAsync(CancellationToken stoppingToken) { while (!stoppingToken.IsCancellationRequested) - { - var message = await channel.ReadQuery(stoppingToken); - await queryAnalyticsWriter.ReportDataSetVersionQuery(message); + { + try + { + var message = await manager.ReadQuery(stoppingToken); + await queryAnalyticsWriter.ReportDataSetVersionQuery(message); + } + catch (Exception e) + { + logger.LogError( + exception: e, + message: "Error whilst reading a query from {QueryManager}", + nameof(IQueryAnalyticsManager)); + } } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsManager.cs similarity index 77% rename from src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs rename to src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsManager.cs index a9ca138a9b..126381f21e 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsChannel.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsManager.cs @@ -3,12 +3,12 @@ namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; -public class QueryAnalyticsChannel : IQueryAnalyticsChannel +public class QueryAnalyticsManager : IQueryAnalyticsManager { private readonly Channel _channel = Channel.CreateUnbounded(); - public async Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) + public async Task AddQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) { await _channel.Writer.WriteAsync(request, cancellationToken); } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs index bc500c3ccf..1ab122add6 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Services/QueryAnalyticsWriter.cs @@ -28,13 +28,24 @@ public async Task ReportDataSetVersionQuery(CaptureDataSetVersionQueryRequest re var directory = analyticsPathResolver.PublicApiQueriesDirectoryPath(); - Directory.CreateDirectory(directory); + try + { + Directory.CreateDirectory(directory); - var filename = $"{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}_{request.DataSetVersionId}.json"; - - await File.WriteAllTextAsync( - Path.Combine(directory, filename), - contents: serialisedRequest); + var filename = $"{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}_{request.DataSetVersionId}.json"; + + await File.WriteAllTextAsync( + Path.Combine(directory, filename), + contents: serialisedRequest); + } + catch (Exception e) + { + logger.LogError( + exception: e, + message: "Error whilst writing {QueryRequest} to disk", + nameof(CaptureDataSetVersionQueryRequest)); + throw; + } } } diff --git a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs index 4da54653c6..8f810b64a1 100644 --- a/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs +++ b/src/GovUk.Education.ExploreEducationStatistics.Public.Data.Api/Startup.cs @@ -256,14 +256,14 @@ public void ConfigureServices(IServiceCollection services) if (_analyticsOptions.Enabled) { - services.AddSingleton(); + services.AddSingleton(); services.AddHostedService(); services.AddSingleton(); services.AddSingleton(); } else { - services.AddSingleton(); + services.AddSingleton(); } } @@ -375,16 +375,17 @@ private static void ApplyCustomMigrations(IApplicationBuilder app) migrations.ForEach(migration => migration.Apply()); } - private class NoopQueryAnalyticsChannel : IQueryAnalyticsChannel + private class NoopQueryAnalyticsManager : IQueryAnalyticsManager { - public Task WriteQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) + public Task AddQuery(CaptureDataSetVersionQueryRequest request, CancellationToken cancellationToken) { return Task.CompletedTask; } - public ValueTask ReadQuery(CancellationToken cancellationToken) + public async ValueTask ReadQuery(CancellationToken cancellationToken) { - return default; + await Task.Delay(Timeout.Infinite, cancellationToken); + return default!; } } }