KavitaStats Cleanup (#695)

* Refactored Stats code to be much cleaner and user better naming.

* Cleaned up the actual http code to use Flurl and to return if the upload was successful or not so we can delete the file where appropriate.

* More refactoring for the stats code to clean it up and keep it consistent with our standards.

* Removed a confusing log statement

* Added support for old api key header from original stat server

* Use the correct endpoint, not the new one.

* Code smell

API/Controllers/StatsController.cs

@@ -25,7 +25,7 @@ namespace API.Controllers
         {
             try
             {
-                await _statsService.PathData(clientInfoDto);
+                await _statsService.RecordClientInfo(clientInfoDto);
 
                 return Ok();
             }

API/Extensions/ServiceCollectionExtensions.cs

@@ -1,24 +0,0 @@
-using API.Interfaces.Services;
-using API.Services.Clients;
-using Microsoft.Extensions.Configuration;
-using Microsoft.Extensions.DependencyInjection;
-
-namespace API.Extensions
-{
-    public static class ServiceCollectionExtensions
-    {
-        public static IServiceCollection AddStartupTask<T>(this IServiceCollection services)
-            where T : class, IStartupTask
-            => services.AddTransient<IStartupTask, T>();
-
-        public static IServiceCollection AddStatsClient(this IServiceCollection services, IConfiguration configuration)
-        {
-            services.AddHttpClient<StatsApiClient>(client =>
-            {
-                client.DefaultRequestHeaders.Add("api-key", "MsnvA2DfQqxSK5jh");
-            });
-
-            return services;
-        }
-    }
-}

API/Interfaces/Services/IStatsService.cs

@@ -5,7 +5,7 @@ namespace API.Interfaces.Services
 {
     public interface IStatsService
     {
-        Task PathData(ClientInfoDto clientInfoDto);
+        Task RecordClientInfo(ClientInfoDto clientInfoDto);
-        Task CollectAndSendStatsData();
+        Task Send();
     }
 }

API/Services/Clients/StatsApiClient.cs

@@ -1,55 +0,0 @@
-using System;
-using System.Net.Http;
-using System.Net.Http.Json;
-using System.Threading.Tasks;
-using API.DTOs.Stats;
-using Microsoft.Extensions.Logging;
-
-namespace API.Services.Clients
-{
-    public class StatsApiClient
-    {
-        private readonly HttpClient _client;
-        private readonly ILogger<StatsApiClient> _logger;
-#pragma warning disable S1075
-        private const string ApiUrl = "http://stats.kavitareader.com";
-#pragma warning restore S1075
-
-        public StatsApiClient(HttpClient client, ILogger<StatsApiClient> logger)
-        {
-            _client = client;
-            _logger = logger;
-            _client.Timeout = TimeSpan.FromSeconds(30);
-        }
-
-        public async Task SendDataToStatsServer(UsageStatisticsDto data)
-        {
-            var responseContent = string.Empty;
-
-            try
-            {
-                using var response = await _client.PostAsJsonAsync(ApiUrl + "/api/InstallationStats", data);
-
-                responseContent = await response.Content.ReadAsStringAsync();
-
-                response.EnsureSuccessStatusCode();
-            }
-            catch (HttpRequestException e)
-            {
-                var info = new
-                {
-                    dataSent = data,
-                    response = responseContent
-                };
-
-                _logger.LogError(e, "KavitaStats did not respond successfully. {Content}", info);
-                throw;
-            }
-            catch (Exception e)
-            {
-                _logger.LogError(e, "An error happened during the request to KavitaStats");
-                throw;
-            }
-        }
-    }
-}

API/Services/DirectoryService.cs

@@ -20,6 +20,7 @@ namespace API.Services
        public static readonly string LogDirectory = Path.Join(Directory.GetCurrentDirectory(), "logs");
        public static readonly string CacheDirectory = Path.Join(Directory.GetCurrentDirectory(), "cache");
        public static readonly string CoverImageDirectory = Path.Join(Directory.GetCurrentDirectory(), "covers");
+       public static readonly string StatsDirectory = Path.Join(Directory.GetCurrentDirectory(), "stats");
 
        public DirectoryService(ILogger<DirectoryService> logger)
        {

API/Services/TaskScheduler.cs

@@ -89,7 +89,7 @@ namespace API.Services
             }
 
             _logger.LogDebug("Scheduling stat collection daily");
-            RecurringJob.AddOrUpdate(SendDataTask, () => _statsService.CollectAndSendStatsData(), Cron.Daily, TimeZoneInfo.Local);
+            RecurringJob.AddOrUpdate(SendDataTask, () => _statsService.Send(), Cron.Daily, TimeZoneInfo.Local);
         }
 
         public void CancelStatsTasks()
@@ -102,7 +102,7 @@ namespace API.Services
         public void RunStatCollection()
         {
             _logger.LogInformation("Enqueuing stat collection");
-            BackgroundJob.Enqueue(() => _statsService.CollectAndSendStatsData());
+            BackgroundJob.Enqueue(() => _statsService.Send());
         }
 
         #endregion

API/Services/Tasks/StatsService.cs

@@ -1,6 +1,7 @@
 using System;
 using System.IO;
 using System.Linq;
+using System.Net.Http;
 using System.Runtime.InteropServices;
 using System.Text.Json;
 using System.Threading;
@@ -9,10 +10,11 @@ using API.Data;
 using API.DTOs.Stats;
 using API.Interfaces;
 using API.Interfaces.Services;
-using API.Services.Clients;
+using Flurl.Http;
 using Hangfire;
 using Kavita.Common;
 using Kavita.Common.EnvironmentInfo;
+using Microsoft.AspNetCore.Http;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
 
@@ -20,75 +22,36 @@ namespace API.Services.Tasks
 {
     public class StatsService : IStatsService
     {
-        private const string TempFilePath = "stats/";
+        private const string StatFileName = "app_stats.json";
-        private const string TempFileName = "app_stats.json";
 
-        private readonly StatsApiClient _client;
         private readonly DataContext _dbContext;
         private readonly ILogger<StatsService> _logger;
         private readonly IUnitOfWork _unitOfWork;
 
-        public StatsService(StatsApiClient client, DataContext dbContext, ILogger<StatsService> logger,
+#pragma warning disable S1075
+        private const string ApiUrl = "http://stats.kavitareader.com";
+#pragma warning restore S1075
+        private static readonly string StatsFilePath = Path.Combine(DirectoryService.StatsDirectory, StatFileName);
+
+        private static bool FileExists => File.Exists(StatsFilePath);
+
+        public StatsService(DataContext dbContext, ILogger<StatsService> logger,
             IUnitOfWork unitOfWork)
         {
-            _client = client;
             _dbContext = dbContext;
             _logger = logger;
             _unitOfWork = unitOfWork;
         }
 
-        private static string FinalPath => Path.Combine(Directory.GetCurrentDirectory(), TempFilePath, TempFileName);
-        private static bool FileExists => File.Exists(FinalPath);
-
-        public async Task PathData(ClientInfoDto clientInfoDto)
-        {
-            _logger.LogDebug("Pathing client data to the file");
-
-            var statisticsDto = await GetData();
-
-            statisticsDto.AddClientInfo(clientInfoDto);
-
-            await SaveFile(statisticsDto);
-        }
-
-        private async Task CollectRelevantData()
-        {
-            _logger.LogDebug("Collecting data from the server and database");
-
-            _logger.LogDebug("Collecting usage info");
-            var usageInfo = await GetUsageInfo();
-
-            _logger.LogDebug("Collecting server info");
-            var serverInfo = GetServerInfo();
-
-            await PathData(serverInfo, usageInfo);
-        }
-
-        private async Task FinalizeStats()
-        {
-            try
-            {
-                var data = await GetExistingData<UsageStatisticsDto>();
-                await _client.SendDataToStatsServer(data);
-                if (FileExists) File.Delete(FinalPath);
-            }
-            catch (Exception ex)
-            {
-                _logger.LogError(ex, "Error Finalizing Stats collection flow");
-                throw;
-            }
-        }
-
         /// <summary>
         /// Due to all instances firing this at the same time, we can DDOS our server. This task when fired will schedule the task to be run
         /// randomly over a 6 hour spread
         /// </summary>
-        public async Task CollectAndSendStatsData()
+        public async Task Send()
         {
             var allowStatCollection = (await _unitOfWork.SettingsRepository.GetSettingsDtoAsync()).AllowStatCollection;
             if (!allowStatCollection)
             {
-                _logger.LogDebug("User has opted out of stat collection, not registering tasks");
                 return;
             }
 
@@ -111,15 +74,92 @@ namespace API.Services.Tasks
         // ReSharper disable once MemberCanBePrivate.Global
         public async Task SendData()
         {
-            _logger.LogDebug("Sending data to the Stats server");
             await CollectRelevantData();
             await FinalizeStats();
         }
 
+        public async Task RecordClientInfo(ClientInfoDto clientInfoDto)
+        {
+            var statisticsDto = await GetData();
+            statisticsDto.AddClientInfo(clientInfoDto);
+
+            await SaveFile(statisticsDto);
+        }
+
+        private async Task CollectRelevantData()
+        {
+            var usageInfo = await GetUsageInfo();
+            var serverInfo = GetServerInfo();
+
+            await PathData(serverInfo, usageInfo);
+        }
+
+        private async Task FinalizeStats()
+        {
+            try
+            {
+                var data = await GetExistingData<UsageStatisticsDto>();
+                var successful = await SendDataToStatsServer(data);
+
+                if (successful)
+                {
+                    ResetStats();
+                }
+            }
+            catch (Exception ex)
+            {
+                _logger.LogError(ex, "There was an exception while sending data to KavitaStats");
+            }
+        }
+
+        private async Task<bool> SendDataToStatsServer(UsageStatisticsDto data)
+        {
+            var responseContent = string.Empty;
+
+            try
+            {
+                var response = await (ApiUrl + "/api/InstallationStats")
+                    .WithHeader("Accept", "application/json")
+                    .WithHeader("User-Agent", "Kavita")
+                    .WithHeader("x-api-key", "MsnvA2DfQqxSK5jh")
+                    .WithHeader("api-key", "MsnvA2DfQqxSK5jh")
+                    .WithHeader("x-kavita-version", BuildInfo.Version)
+                    .WithTimeout(TimeSpan.FromSeconds(30))
+                    .PostJsonAsync(data);
+
+                if (response.StatusCode != StatusCodes.Status200OK)
+                {
+                    _logger.LogError("KavitaStats did not respond successfully. {Content}", response);
+                    return false;
+                }
+
+                return true;
+            }
+            catch (HttpRequestException e)
+            {
+                var info = new
+                {
+                    dataSent = data,
+                    response = responseContent
+                };
+
+                _logger.LogError(e, "KavitaStats did not respond successfully. {Content}", info);
+            }
+            catch (Exception e)
+            {
+                _logger.LogError(e, "An error happened during the request to KavitaStats");
+            }
+
+            return false;
+        }
+
+        private static void ResetStats()
+        {
+            if (FileExists) File.Delete(StatsFilePath);
+        }
+
         private async Task PathData(ServerInfoDto serverInfoDto, UsageInfoDto usageInfoDto)
         {
-            _logger.LogDebug("Pathing server and usage info to the file");
-
             var data = await GetData();
 
             data.ServerInfo = serverInfoDto;
@@ -130,7 +170,7 @@ namespace API.Services.Tasks
             await SaveFile(data);
         }
 
-        private async ValueTask<UsageStatisticsDto> GetData()
+        private static async ValueTask<UsageStatisticsDto> GetData()
         {
             if (!FileExists) return new UsageStatisticsDto {InstallId = HashUtil.AnonymousToken()};
 
@@ -176,39 +216,17 @@ namespace API.Services.Tasks
             return serverInfo;
         }
 
-        private async Task<T> GetExistingData<T>()
+        private static async Task<T> GetExistingData<T>()
         {
-            _logger.LogInformation("Fetching existing data from file");
+            var json = await File.ReadAllTextAsync(StatsFilePath);
-            var existingDataJson = await GetFileDataAsString();
+            return JsonSerializer.Deserialize<T>(json);
-
-            _logger.LogInformation("Deserializing data from file to object");
-            var existingData = JsonSerializer.Deserialize<T>(existingDataJson);
-
-            return existingData;
         }
 
-        private async Task<string> GetFileDataAsString()
+        private static async Task SaveFile(UsageStatisticsDto statisticsDto)
         {
-            _logger.LogInformation("Reading file from disk");
+            DirectoryService.ExistOrCreate(DirectoryService.StatsDirectory);
-            return await File.ReadAllTextAsync(FinalPath);
-        }
 
-        private async Task SaveFile(UsageStatisticsDto statisticsDto)
+            await File.WriteAllTextAsync(StatsFilePath, JsonSerializer.Serialize(statisticsDto));
-        {
-            _logger.LogDebug("Saving file");
-
-            var finalDirectory = FinalPath.Replace(TempFileName, string.Empty);
-            if (!Directory.Exists(finalDirectory))
-            {
-                _logger.LogDebug("Creating tmp directory");
-                Directory.CreateDirectory(finalDirectory);
-            }
-
-            _logger.LogDebug("Serializing data to write");
-            var dataJson = JsonSerializer.Serialize(statisticsDto);
-
-            _logger.LogDebug("Writing file to the disk");
-            await File.WriteAllTextAsync(FinalPath, dataJson);
         }
     }
 }

API/Startup.cs

@@ -106,7 +106,6 @@ namespace API
 
             services.AddResponseCaching();
 
-            services.AddStatsClient(_config);
 
             services.AddHangfire(configuration => configuration
                 .UseSimpleAssemblyNameTypeSerializer()