EES-5826 - use channels to separate Public API query response from the writing of queries for analytics #5678
Conversation
duncan-at-hiveit
force-pushed
the
EES-5826-use-channels-for-fire-and-forget
branch
from
81d9c6e to
9307ef6
Compare
…sts for writing queries for analytics, rather than not awaiting the writing process.
duncan-at-hiveit
force-pushed
the
EES-5826-use-channels-for-fire-and-forget
branch
from
9307ef6 to
f2a8898
Compare
There was a problem hiding this comment.
Unless you think it provides added clarity, I wouldn't put Channel in the name since that is an implementation detail. You can think if this more as the QueryAnalyticsReporter or Manager or Handler? ie it's just the place where all other components can go to send/report their analytics.
| while (!stoppingToken.IsCancellationRequested) | ||
| { | ||
| var message = await channel.ReadQuery(stoppingToken); | ||
| await queryAnalyticsWriter.ReportDataSetVersionQuery(message); |
There was a problem hiding this comment.
If this throws, I'm guessing this background service will stop writing the files? Probably want to guard against that - and log errors?
| @@ -47,15 +30,15 @@ public async Task ReportDataSetVersionQuery( | |||
|
|
|||
| Directory.CreateDirectory(directory); | |||
There was a problem hiding this comment.
I would catch and log errors here - permissions, disk full, all sorts of things that could go wrong.
|
|
||
| public ValueTask<CaptureDataSetVersionQueryRequest> ReadQuery(CancellationToken cancellationToken) | ||
| { | ||
| return default; |
There was a problem hiding this comment.
Could this potentially lead to a spinning process? If something was consuming it, it would be pouring out a stream of values? Perhaps add :
await Task.Delay(Timeout.Infinite, cancellationToken);
There was a problem hiding this comment.
Not something for you to change - but just whilst I have your attention :-)
It would be usual for a there to be two interfaces, one as the "Reporter" where components can send their data, and another as a "Consumer" that would take the data out and process it. This cleans up the intentions of components and prevents any possibility of anything else reading from the channel when they should only ever be writing. The container of course resolves to the same instance.
| IQueryAnalyticsWriter queryAnalyticsWriter | ||
| ) : BackgroundService | ||
| { | ||
| protected override async Task ExecuteAsync(CancellationToken stoppingToken) |
There was a problem hiding this comment.
Can this be unit tested?
|
|
||
| namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Api.Services; | ||
|
|
||
| public class QueryAnalyticsChannel : IQueryAnalyticsChannel |
There was a problem hiding this comment.
Unit tests for this class?
This PR:
Rather than spawning off a separate Task from the main request-processing Thread and simply not awaiting it, instead we provide a Channel whereby the DataSetQueryService can write the desired query to be captured to the channel.
A BackgroundService continually monitors the Channel, and reads from the Channel when new messages are available. It then forwards the message to the QueryAnalyticsWriter service, which then performs the actual work of writing out the query for analytics.