refactor: route host diagnostics through ILogger

docs/design/diagnostics.md

@@ -62,24 +62,62 @@ For an operation with initialized logging and telemetry, report a caught failure
 `ITelemetryService.TrackException`: consent can disable the remote record, while the local log remains. Use the
 reporting type's logger and add telemetry properties only for a specific diagnostic query. A failure that propagates
 to the API boundary or to the global handler is reported there; reporting it again in a lower layer records it
-twice. These are rules for changes; existing catch sites do not all follow them yet.
+twice. An expected condition that the code repairs, such as a token cache that
+cannot be decrypted and is rebuilt at the next sign-in, is logged only: tracking it would count it among the failures.
+These are rules for changes; existing catch sites do not all follow them yet.
 
 ### API boundary
 
 [HostingExtensions.AddAndConfigureProblemDetails](../../src/Infrastructure/Extensions/HostingExtensions.cs) owns
-reporting for exceptions mapped to API responses. Its mappings track exceptions. Specific mappings add host
-diagnostics at `Verbose`; the catch-all mapping adds diagnostics regardless of that setting. Exception details are
-enabled for the UI to display and copy. Do not add duplicate tracking in each controller for exceptions that
-propagate to these mappings.
+reporting for exceptions mapped to API responses. Its mappings track exceptions. Specific mappings log the exception at
+`Information`, which reaches the diagnostics pane only at diagnostic level `Verbose`, highlighted because it carries the
+exception; the catch-all mapping does not log, because the problem details middleware already logs every response
+with status 500 or higher as an error.
+Exception details are enabled for the UI to display and copy. Do not add duplicate tracking in each controller for
+exceptions that propagate to these mappings.
 
 ## Local logging and the diagnostics pane
 
+There is no log file: users are asked to copy errors from the diagnostics pane, and IT support reads the Event Log.
 The host logger factory, created by [BootstrapContextFactory](../../src/Host/BootstrapContextFactory.cs), writes
-`Warning` and above of every category, ASP.NET Core and Kestrel included, to the Windows Event Log and to
-[AppEnvironment.Diagnostics](../../src/Infrastructure/AppEnvironment.cs). That store also receives explicit
-`AddDiagnostics` calls, lives in memory for the whole process and has no eviction. The
-[frontend logger](../../src/Scripts/controllers/logger.ts) polls `GetDiagnostics` only at diagnostic level
-`Verbose`: a message can be stored without ever appearing in the pane.
+`Warning` and above of every category, ASP.NET Core and Kestrel included, to the Windows Event Log and to the
+[DiagnosticLogStore](../../src/Infrastructure/Diagnostics/DiagnosticLogStore.cs) read by the pane. At diagnostic level
+`Verbose` the store also receives `Information` records, except from the `Microsoft` and `System` categories, whose
+records below `Warning` describe every request, the pane's own polling included.
+[DiagnosticLevelSwitch](../../src/Infrastructure/Diagnostics/DiagnosticLevelSwitch.cs) carries the level: the
+logger factory exists before the user settings are read, so the bootstrap applies the saved level and `UpdateOptions`
+applies a changed one. Code that writes to the pane logs through `ILogger`, and guards an expensive argument, such as
+a serialized payload, with `IsEnabled(LogLevel.Information)`. Never log tokens or connection strings with passwords:
+the pane shows them, and redaction removes only the text inside sensitive tags.
+
+| Store property | Consequence |
+| --- | --- |
+| Created with the logger factory and registered in the container as an instance | Records written before the host exists, during the bootstrap, reach the pane |
+| Holds the last 1000 records and at most 8M characters of text, oldest evicted first | Memory is bounded even when records carry large payloads; a read reports how many records after its cursor it does not return |
+| Each record has a sequence number; reading does not change the store | The pane keeps the cursor, so a reload reads every record held again and two readers do not take records from each other |
+| A read takes a maximum count and returns the newest records | The pane asks only for as many records as it keeps |
+| A record keeps the template, the placeholder values and the exception chain apart from the formatted message | The pane shows a JSON value as a tree; the formatted message is the plain text of the record |
+| The exception chain holds every inner exception, those of an aggregate included, and the public properties each exception type adds, such as an error or status code | What a type writes in its `ToString` override is kept without the unstructured text; a property that exposes a secret reaches the pane, so such a type must not be logged as it is |
+
+The [frontend logger](../../src/Scripts/controllers/logger.ts) polls `GetDiagnostics` at every diagnostic level except
+`None`. At `Basic` it shows host records of level `Warning` and above; `Verbose` adds the `Information` records.
+A record of level `Warning` and above, or one that carries an exception, is highlighted.
+The app's own messages, those without the `[HOST]` prefix, live only in the page, so a reload loses them. Clear
+empties the view only: after a reload the pane reads the host records again from the store.
+
+The store keeps the original text and the pane shows it, because the user who reads it owns the data and needs the
+names of the server, database or account that failed. Redaction happens where the text leaves the machine:
+
+| Way out | Redaction |
+| --- | --- |
+| A copied message; Report issue | `Utils.Obj.anonymize`: the same five tags as [SensitiveTags](../../src/Infrastructure/Telemetry/SensitiveTags.cs), plus the values of personal keys such as `email` and `serverName` |
+| Host telemetry | `DefaultTelemetryProcessor`, through `SensitiveTags` |
+| Crash report: saved file and the dialog's Copy | None yet: the exception text is written as `ToString()` returns it, until the crash report redesign |
+| Event Log | None: it stays on the machine and IT support reads it |
+
+Text that leaves the machine, from the store or from an exception, applies `SensitiveTags` or the same tags. The
+two tag lists, in C# and in TypeScript, change together; `SensitiveTagsTests` fails when they differ. Redaction
+removes only tagged text, so copied text can still include report, server and database names.
 
 ## Telemetry consent
 

src/Controllers/ApplicationController.cs

@@ -1,14 +1,15 @@
 using System;
-using System.Collections.Generic;
 using System.Net.Mime;
 using System.Threading;
 using System.Threading.Tasks;
 using Hellang.Middleware.ProblemDetails;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.Logging;
 using Sqlbi.Bravo.Infrastructure;
 using Sqlbi.Bravo.Infrastructure.Configuration;
 using Sqlbi.Bravo.Infrastructure.Configuration.Settings;
+using Sqlbi.Bravo.Infrastructure.Diagnostics;
 using Sqlbi.Bravo.Infrastructure.Extensions;
 using Sqlbi.Bravo.Infrastructure.Helpers;
 using Sqlbi.Bravo.Infrastructure.Policies;
@@ -29,11 +30,22 @@ public class ApplicationController : ControllerBase
 {
     private readonly ITelemetryService _telemetryService;
     private readonly IPolicyService _policyService;
+    private readonly DiagnosticLevelSwitch _diagnosticLevel;
+    private readonly DiagnosticLogStore _diagnosticLog;
+    private readonly ILogger<ApplicationController> _logger;
 
-    public ApplicationController(ITelemetryService telemetryService, IPolicyService policyService)
+    public ApplicationController(
+        ITelemetryService telemetryService,
+        IPolicyService policyService,
+        DiagnosticLevelSwitch diagnosticLevel,
+        DiagnosticLogStore diagnosticLog,
+        ILogger<ApplicationController> logger)
     {
         _telemetryService = telemetryService;
         _policyService = policyService;
+        _diagnosticLevel = diagnosticLevel;
+        _diagnosticLog = diagnosticLog;
+        _logger = logger;
     }
 
     /// <summary>
@@ -84,8 +96,13 @@ public IActionResult UpdateOptions(BravoOptions options)
 
             return BadRequest(problem);
         }
-
-        _telemetryService.TelemetryEnabled = TelemetryConsent.Resolve(_policyService.Current, UserPreferences.Current).IsEnabled;
+        finally
+        {
+            // A failed save can leave the settings changed in memory: the runtime follows what they hold.
+            var settings = UserPreferences.Current;
+            _diagnosticLevel.Level = settings.DiagnosticLevel;
+            _telemetryService.TelemetryEnabled = TelemetryConsent.Resolve(_policyService.Current, settings).IsEnabled;
+        }
 
         return Ok();
     }
@@ -146,7 +163,7 @@ public IActionResult PBIDesktopOpenPBIX(bool waitForStarted, CancellationToken c
         {
             if (ProcessHelper.OpenShellExecute(path, waitForStarted, out var processId, cancellationToken))
             {
-                var report = PBIDesktopReport.CreateFrom(processId.Value);
+                var report = PBIDesktopReport.CreateFrom(processId.Value, _logger);
                 return Ok(report);
             }
 
@@ -177,29 +194,21 @@ public IActionResult FileSystemOpen(string path)
     }
 
     /// <summary>
-    /// Gets all the <see cref="DiagnosticMessage"/> for the application
+    /// Gets the host log records added after the specified sequence number
     /// </summary>
+    /// <param name="after">The <see cref="DiagnosticLogPage.LastSequence"/> of the previous read, 0 to read every record held</param>
+    /// <param name="max">The maximum number of records to return, the newest ones</param>
     /// <response code="200">Status200OK - Success</response>
     [HttpGet]
     [ActionName("GetDiagnostics")]
     [Consumes(MediaTypeNames.Application.Json)]
     [Produces(MediaTypeNames.Application.Json)]
-    [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(IEnumerable<DiagnosticMessage>))]
+    [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(DiagnosticLogPage))]
     [ProducesDefaultResponseType]
-    public IActionResult GetDiagnostics(bool? all = null)
+    public IActionResult GetDiagnostics(long after = 0, int max = DiagnosticLogStore.DefaultCapacity)
     {
-        var messages = new SortedList<DateTime, DiagnosticMessage>();
-
-        foreach (var message in AppEnvironment.Diagnostics.Values)
-        {
-            if (all == true || message.ReadTimestamp is null)
-            {
-                message.ReadTimestamp = DateTime.UtcNow;
-                messages.Add(message.Timestamp, message);
-            }
-        }
-
-        return Ok(messages.Values);
+        var page = _diagnosticLog.Read(after, max);
+        return Ok(page);
     }
 
     /// <summary>
@@ -214,7 +223,7 @@ public IActionResult GetDiagnostics(bool? all = null)
     [ProducesDefaultResponseType]
     public async Task<IActionResult> GetCurrentVersion(UpdateChannelType updateChannel, CancellationToken cancellationToken)
     {
-        var bravoUpdate = await CommonHelper.CheckForUpdateAsync(updateChannel, cancellationToken);
+        var bravoUpdate = await CommonHelper.CheckForUpdateAsync(updateChannel, _logger, cancellationToken);
         if (bravoUpdate.IsNewerVersion)
             System.Media.SystemSounds.Beep.Play();
 

src/Controllers/AuthenticationController.cs

@@ -1,13 +1,10 @@
 using System;
 using System.Net.Mime;
-using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
-using Sqlbi.Bravo.Infrastructure;
 using Sqlbi.Bravo.Infrastructure.PowerBI.Cloud;
-using Sqlbi.Bravo.Models;
 using Sqlbi.Bravo.Models.Authentication;
 using Sqlbi.Bravo.Services;
 
@@ -44,9 +41,6 @@ public async Task<IActionResult> GetEnvironmentsAsync(
             request.Email,
             cancellationToken);
 
-        if (AppEnvironment.IsDiagnosticLevelVerbose)
-            AppEnvironment.AddDiagnostics(DiagnosticMessageType.Json, name: $"{nameof(AuthenticationController)}.{nameof(GetEnvironmentsAsync)}", JsonSerializer.Serialize(environments));
-
         var response = new GetEnvironmentsResponse(environments);
         return Ok(response);
     }

src/Controllers/TemplateDevelopmentController.cs

@@ -4,6 +4,7 @@
 using System.Threading;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.Logging;
 using Sqlbi.Bravo.Infrastructure.Helpers;
 using Sqlbi.Bravo.Models;
 using Sqlbi.Bravo.Models.AnalyzeModel;
@@ -26,11 +27,13 @@ public class TemplateDevelopmentController : ControllerBase
 
     private readonly ITemplateDevelopmentService _templateDevelopmentService;
     private readonly IAnalyzeModelService _analyzeModelService;
+    private readonly ILogger<TemplateDevelopmentController> _logger;
 
-    public TemplateDevelopmentController(ITemplateDevelopmentService templateDevelopmentService, IAnalyzeModelService analyzeModelService)
+    public TemplateDevelopmentController(ITemplateDevelopmentService templateDevelopmentService, IAnalyzeModelService analyzeModelService, ILogger<TemplateDevelopmentController> logger)
     {
         _templateDevelopmentService = templateDevelopmentService;
         _analyzeModelService = analyzeModelService;
+        _logger = logger;
     }
 
     /// <summary>
@@ -175,7 +178,7 @@ public IActionResult PBIDesktopOpenPBIX(string path, bool waitForStarted, Cancel
     {
         if (ProcessHelper.OpenShellExecute(path, waitForStarted, out var processId, cancellationToken))
         {
-            var report = PBIDesktopReport.CreateFrom(processId.Value);
+            var report = PBIDesktopReport.CreateFrom(processId.Value, _logger);
             return Ok(report);
         }
         return Forbid();

src/Host/BootstrapContext.cs

@@ -1,6 +1,7 @@
 using System;
 using Microsoft.Extensions.Logging;
 using Sqlbi.Bravo.Infrastructure.Configuration.Settings;
+using Sqlbi.Bravo.Infrastructure.Diagnostics;
 using Sqlbi.Bravo.Infrastructure.Policies;
 using Sqlbi.Bravo.Infrastructure.Telemetry;
 
@@ -19,19 +20,25 @@ internal sealed class BootstrapContext : IDisposable
 
     internal BootstrapContext(
         ILoggerFactory loggerFactory,
+        DiagnosticLevelSwitch diagnosticLevel,
+        DiagnosticLogStore diagnosticLog,
         IPolicyService policyService,
         IUserSettings settings,
         ITelemetryService telemetry,
         IDisposable? exceptionHandlerAttachment = null)
     {
         LoggerFactory = loggerFactory;
+        DiagnosticLevel = diagnosticLevel;
+        DiagnosticLog = diagnosticLog;
         PolicyService = policyService;
         Settings = settings;
         Telemetry = telemetry;
         _exceptionHandlerAttachment = exceptionHandlerAttachment;
     }
 
     public ILoggerFactory LoggerFactory { get; }
+    public DiagnosticLevelSwitch DiagnosticLevel { get; }
+    public DiagnosticLogStore DiagnosticLog { get; }
     public IPolicyService PolicyService { get; }
     public IUserSettings Settings { get; }
     public ITelemetryService Telemetry { get; }

src/Host/BootstrapContextFactory.cs

@@ -10,24 +10,32 @@ namespace Sqlbi.Bravo.Host;
 
 internal sealed partial class BootstrapContextFactory(
     ILoggerFactory loggerFactory,
+    DiagnosticLevelSwitch diagnosticLevel,
+    DiagnosticLogStore diagnosticLog,
     IPolicyServiceFactory policyFactory,
     IUserSettingsServiceFactory settingsFactory,
     ITelemetryServiceFactory telemetryFactory,
     UnhandledExceptionHandler exceptionHandler)
 {
     private readonly ILoggerFactory _loggerFactory = loggerFactory;
+    private readonly DiagnosticLevelSwitch _diagnosticLevel = diagnosticLevel;
+    private readonly DiagnosticLogStore _diagnosticLog = diagnosticLog;
     private readonly IPolicyServiceFactory _policyFactory = policyFactory;
     private readonly IUserSettingsServiceFactory _settingsFactory = settingsFactory;
     private readonly ITelemetryServiceFactory _telemetryFactory = telemetryFactory;
     private readonly UnhandledExceptionHandler _exceptionHandler = exceptionHandler;
 
     public static BootstrapContext CreateDefault(UnhandledExceptionHandler exceptionHandler)
     {
-        var loggerFactory = CreateLoggerFactory();
+        var diagnosticLevel = new DiagnosticLevelSwitch();
+        var diagnosticLog = new DiagnosticLogStore();
+        var loggerFactory = CreateLoggerFactory(diagnosticLevel, diagnosticLog);
         try
         {
             var factory = new BootstrapContextFactory(
                 loggerFactory,
+                diagnosticLevel,
+                diagnosticLog,
                 new PolicyServiceFactory(),
                 new UserSettingsServiceFactory(),
                 new TelemetryServiceFactory(),
@@ -50,22 +58,22 @@ public BootstrapContext Create()
     {
         var policies = CreatePolicies();
         var settings = CreateSettings();
+        _diagnosticLevel.Level = settings.DiagnosticLevel;
 
         var consent = TelemetryConsent.Resolve(policies.Current, settings);
         var telemetry = CreateTelemetry(consent.IsEnabled);
         var exceptionHandlerAttachment = _exceptionHandler.AttachServices(_loggerFactory, telemetry);
 
-        return new BootstrapContext(_loggerFactory, policies, settings, telemetry, exceptionHandlerAttachment);
+        return new BootstrapContext(_loggerFactory, _diagnosticLevel, _diagnosticLog, policies, settings, telemetry, exceptionHandlerAttachment);
     }
 
-    private static ILoggerFactory CreateLoggerFactory()
+    private static ILoggerFactory CreateLoggerFactory(DiagnosticLevelSwitch diagnosticLevel, DiagnosticLogStore diagnosticLog)
     {
         return LoggerFactory.Create((builder) =>
         {
             builder.AddEventSourceLogger();
             builder.AddBravoEventLog();
-            builder.AddDiagnosticMessages();
-            builder.AddFilter<DiagnosticMessageLoggerProvider>((level) => level >= LogLevel.Warning);
+            builder.AddBravoDiagnosticLog(diagnosticLog, diagnosticLevel, TimeProvider.System);
 #if DEBUG
             builder.AddConsole();
             builder.AddDebug();

src/Host/BravoLoggingBuilderExtensions.cs

@@ -1,5 +1,7 @@
-using Microsoft.Extensions.Logging;
+using System;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.EventLog;
+using Sqlbi.Bravo.Infrastructure.Diagnostics;
 
 namespace Sqlbi.Bravo.Host;
 
@@ -13,4 +15,20 @@ public static ILoggingBuilder AddBravoEventLog(this ILoggingBuilder builder)
 
         return builder;
     }
+
+    public static ILoggingBuilder AddBravoDiagnosticLog(
+        this ILoggingBuilder builder,
+        DiagnosticLogStore store,
+        DiagnosticLevelSwitch levelSwitch,
+        TimeProvider timeProvider)
+    {
+        builder.AddDiagnosticLog(store, timeProvider);
+        builder.AddFilter<DiagnosticLogProvider>((category, level) => level >= levelSwitch.MinimumLevel);
+
+        // Framework records below Warning describe every request, the pane's own polling included.
+        builder.AddFilter<DiagnosticLogProvider>("Microsoft", LogLevel.Warning);
+        builder.AddFilter<DiagnosticLogProvider>("System", LogLevel.Warning);
+
+        return builder;
+    }
 }

src/Host/BravoServiceCollectionExtensions.cs

@@ -67,6 +67,8 @@ private static IServiceCollection AddBootstrapServices(this IServiceCollection s
 
         // Replace the default ILoggerFactory registered by the WebApplicationBuilder
         services.Replace(ServiceDescriptor.Singleton(bootstrap.LoggerFactory));
+        services.AddSingleton(bootstrap.DiagnosticLevel);
+        services.AddSingleton(bootstrap.DiagnosticLog);
 
         services.AddSingleton(bootstrap.PolicyService);
         services.AddSingleton(bootstrap.Settings);