From 5806c5bf168a243a29cf6cd0c6ca4d045d9e834f Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Mon, 19 Oct 2026 21:30:28 +0200 Subject: [PATCH] refactor: persist telemetry through ServerTelemetryChannel TelemetryService sends through ServerTelemetryChannel with a local store under .cache\.telemetry, so unsent telemetry survives the process, and waits for delivery when it is disposed. TelemetryConsent gives a configured policy precedence over the user preference; saving the options applies it, since the TelemetryEnabled setter no longer does. Program flushes the process-wide instance before rethrowing a fatal exception and disposes it on a normal exit. --- src/Bravo.csproj | 1 + src/Controllers/ApplicationController.cs | 7 +- src/Host/BravoApplicationInitializer.cs | 2 + src/Infrastructure/AppEnvironment.cs | 3 + .../Telemetry/TelemetryConsent.cs | 26 +++ .../Telemetry/TelemetryService.cs | 152 +++++++++++++----- src/Program.cs | 7 + src/packages.lock.json | 20 +++ test/Bravo.Tests/Host/HostFakes.cs | 9 +- .../Telemetry/TelemetryConsentTests.cs | 39 +++++ .../Telemetry/TelemetryServiceTests.cs | 100 ++++++++++++ 11 files changed, 324 insertions(+), 42 deletions(-) create mode 100644 src/Infrastructure/Telemetry/TelemetryConsent.cs create mode 100644 test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs create mode 100644 test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs diff --git a/src/Bravo.csproj b/src/Bravo.csproj index 38b471e6..7957c75b 100644 --- a/src/Bravo.csproj +++ b/src/Bravo.csproj @@ -53,6 +53,7 @@ + diff --git a/src/Controllers/ApplicationController.cs b/src/Controllers/ApplicationController.cs index 11a6c191..16a4a648 100644 --- a/src/Controllers/ApplicationController.cs +++ b/src/Controllers/ApplicationController.cs @@ -11,6 +11,7 @@ using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Extensions; using Sqlbi.Bravo.Infrastructure.Helpers; +using Sqlbi.Bravo.Infrastructure.Policies; using Sqlbi.Bravo.Infrastructure.Security; using Sqlbi.Bravo.Infrastructure.Telemetry; using Sqlbi.Bravo.Models; @@ -27,10 +28,12 @@ namespace Sqlbi.Bravo.Controllers; public class ApplicationController : ControllerBase { private readonly ITelemetryService _telemetryService; + private readonly IPolicyService _policyService; - public ApplicationController(ITelemetryService telemetryService) + public ApplicationController(ITelemetryService telemetryService, IPolicyService policyService) { _telemetryService = telemetryService; + _policyService = policyService; } /// @@ -82,7 +85,7 @@ public IActionResult UpdateOptions(BravoOptions options) return BadRequest(problem); } - _telemetryService.TelemetryEnabled = UserPreferences.Current.TelemetryEnabled; + _telemetryService.TelemetryEnabled = TelemetryConsent.Resolve(_policyService.Current, UserPreferences.Current).IsEnabled; return Ok(); } diff --git a/src/Host/BravoApplicationInitializer.cs b/src/Host/BravoApplicationInitializer.cs index aa2e1e17..6aaf6629 100644 --- a/src/Host/BravoApplicationInitializer.cs +++ b/src/Host/BravoApplicationInitializer.cs @@ -5,6 +5,7 @@ using Sqlbi.Bravo.Infrastructure; using Sqlbi.Bravo.Infrastructure.Helpers; using Sqlbi.Bravo.Infrastructure.Services; +using Sqlbi.Bravo.Infrastructure.Telemetry; namespace Sqlbi.Bravo.Host; @@ -40,6 +41,7 @@ private static void ConfigureDirectories() Directory.SetCurrentDirectory(AppContext.BaseDirectory); Directory.CreateDirectory(AppEnvironment.ApplicationDataPath); Directory.CreateDirectory(AppEnvironment.ApplicationTempPath); + Directory.CreateDirectory(TelemetryService.DefaultStorageFolder); } private static void ConfigureRuntimeOptimization() diff --git a/src/Infrastructure/AppEnvironment.cs b/src/Infrastructure/AppEnvironment.cs index ce638ed6..e3541c20 100644 --- a/src/Infrastructure/AppEnvironment.cs +++ b/src/Infrastructure/AppEnvironment.cs @@ -82,6 +82,7 @@ static AppEnvironment() ApplicationDataPath = Path.Combine(Environment.GetFolderPath(DeploymentMode == AppDeploymentMode.Packaged ? Environment.SpecialFolder.UserProfile : Environment.SpecialFolder.LocalApplicationData, Environment.SpecialFolderOption.DoNotVerify), ApplicationName); ApplicationTempPath = Path.Combine(ApplicationDataPath, ".temp"); + ApplicationCachePath = Path.Combine(ApplicationDataPath, ".cache"); UserSettingsFilePath = Path.Combine(ApplicationDataPath, "usersettings.json"); MsalTokenCacheFilePath = Path.Combine(ApplicationDataPath, ".msalcache"); WebView2VersionInfo = WebView2Helper.GetRuntimeVersionInfo(); @@ -136,6 +137,8 @@ public static RegistryKey? ApplicationInstallerRegistryHKey public static string ApplicationTempPath { get; } + public static string ApplicationCachePath { get; } + public static string UserSettingsFilePath { get; } public static string MsalTokenCacheFilePath { get; } diff --git a/src/Infrastructure/Telemetry/TelemetryConsent.cs b/src/Infrastructure/Telemetry/TelemetryConsent.cs new file mode 100644 index 00000000..acd4ad75 --- /dev/null +++ b/src/Infrastructure/Telemetry/TelemetryConsent.cs @@ -0,0 +1,26 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; + +namespace Sqlbi.Bravo.Infrastructure.Telemetry; + +internal enum TelemetryConsentSource +{ + UserSettings, + Policy, +} + +/// +/// The effective decision on whether telemetry is collected, and where that decision comes from. +/// +internal readonly record struct TelemetryConsent(bool IsEnabled, TelemetryConsentSource Source) +{ + /// + /// A configured policy overrides the user preference: the source tells the user which one applies. + /// + public static TelemetryConsent Resolve(PolicySnapshot policies, IUserSettings settings) + { + return policies.TelemetryEnabled is bool policyValue + ? new TelemetryConsent(policyValue, TelemetryConsentSource.Policy) + : new TelemetryConsent(settings.TelemetryEnabled, TelemetryConsentSource.UserSettings); + } +} \ No newline at end of file diff --git a/src/Infrastructure/Telemetry/TelemetryService.cs b/src/Infrastructure/Telemetry/TelemetryService.cs index 444a42dd..a5c39e91 100644 --- a/src/Infrastructure/Telemetry/TelemetryService.cs +++ b/src/Infrastructure/Telemetry/TelemetryService.cs @@ -1,7 +1,13 @@ using System; +using System.Collections.Generic; using System.Diagnostics; +using System.IO; +using System.Threading; +using System.Threading.Tasks; using Microsoft.ApplicationInsights; +using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.Extensibility; +using Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel; using Sqlbi.Bravo.Infrastructure.Configuration; using Sqlbi.Bravo.Infrastructure.Policies; @@ -10,72 +16,140 @@ namespace Sqlbi.Bravo.Infrastructure.Telemetry; public interface ITelemetryService : IDisposable { bool TelemetryEnabled { get; set; } - void TrackException(Exception exception); + void TrackException(Exception exception, IDictionary? properties = null); + Task FlushAsync(CancellationToken cancellationToken = default); + bool TryFlushBeforeShutdown(int timeoutMilliseconds); } internal sealed class TelemetryService : ITelemetryService { + public static readonly string DefaultStorageFolder = + Path.Combine(AppEnvironment.ApplicationCachePath, ".telemetry"); + + private static readonly Lazy s_instance = new(CreateInstance, isThreadSafe: true); + private readonly TelemetryConfiguration _configuration; + private readonly ITelemetryChannel _channel; private readonly TelemetryClient _client; - private readonly PolicySnapshot _policies; + private bool _disposed; - private static readonly Lazy _instance = new(CreateInstance, isThreadSafe: true); + private TelemetryService(TelemetryConfiguration configuration) + { + _configuration = configuration; + _channel = configuration.TelemetryChannel; + _client = new TelemetryClient(configuration); + } - public static TelemetryService Instance => _instance.Value; + /// + /// Gets the process-wide instance, created on first use with the consent resolved from policies + /// and user settings. + /// + public static TelemetryService Instance => s_instance.Value; - private static TelemetryService CreateInstance() + /// + /// Disposes the process-wide instance, if it was created. + /// + public static void DisposeInstance() { - // Use parameterless constructor to avoid default processors (e.g. adaptive sampling) - // that TelemetryConfiguration.CreateDefault() would register. - var configuration = new TelemetryConfiguration(); - - // Remark: ensure no sampling is applied — every exception must be recorded - var processorChain = configuration.DefaultTelemetrySink.TelemetryProcessorChainBuilder; - processorChain.Use((next) => new DefaultTelemetryProcessor(next)); - processorChain.Build(); + if (s_instance.IsValueCreated) + s_instance.Value.Dispose(); + } - configuration.TelemetryInitializers.Add(new DefaultTelemetryInitializer()); - configuration.ConnectionString = TelemetrySessionInfo.ConnectionString; + public bool TelemetryEnabled + { + get => _configuration.DisableTelemetry == false; + set => _configuration.DisableTelemetry = !value; + } - var policies = new PolicyServiceFactory().Create().Current; + public void TrackException(Exception exception, IDictionary? properties = null) + { + if (exception is AggregateException aggregateException) + exception = aggregateException.GetBaseException(); - // Determine whether telemetry is enabled based on policies and user settings. - var telemetryEnabled = policies.TelemetryEnabled ?? UserPreferences.Current.TelemetryEnabled; - configuration.DisableTelemetry = !telemetryEnabled; -#if DEBUG - configuration.TelemetryChannel.DeveloperMode = Debugger.IsAttached; -#endif - return new TelemetryService(configuration, policies); + _client.TrackException(exception, properties); + // With ServerTelemetryChannel, Flush moves telemetry out of the in-memory telemetry + // buffer and into the transmission pipeline without waiting for delivery. Calling it + // after TrackException improves the chances of preserving the exception if the process + // terminates before the next scheduled flush, while keeping this path non-blocking. + // It does not guarantee that the telemetry has been sent or persisted to local storage. + _client.Flush(); } - private TelemetryService(TelemetryConfiguration configuration, PolicySnapshot policies) + public Task FlushAsync(CancellationToken cancellationToken = default) { - _configuration = configuration; - _client = new TelemetryClient(configuration); - _policies = policies; + return _client.FlushAsync(cancellationToken); } - public bool TelemetryEnabled + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) { - get => !_configuration.DisableTelemetry; - set + // At shutdown, wait for ServerTelemetryChannel to flush telemetry that could + // otherwise be lost when the process exits. FlushAsync completes once pending + // telemetry has been sent or persisted to local storage. The wait is bounded + // because telemetry must never prevent the application from terminating. + using var cts = new CancellationTokenSource(timeoutMilliseconds); + try { - var telemetryEnabled = _policies.TelemetryEnabled ?? value; - _configuration.DisableTelemetry = !telemetryEnabled; + return _client.FlushAsync(cts.Token).GetAwaiter().GetResult(); } + catch (OperationCanceledException) + { + return false; + } + } + + public void Dispose() + { + if (_disposed) + return; + + _disposed = true; + + _ = TryFlushBeforeShutdown(5_000); + + _configuration.Dispose(); + + // A channel assigned to the sink is not owned by it. + _channel.Dispose(); } - public void TrackException(Exception exception) + public static TelemetryService Create(string connectionString, bool telemetryEnabled, string storageFolder) { - if (exception is AggregateException aex) - exception = aex.GetBaseException(); + var channel = new ServerTelemetryChannel + { + // Remark: folder must exist beforehand as the channel does not auto-create it. + // If missing, local storage remains disabled and unsent telemetry is lost. + StorageFolder = storageFolder + }; - _client.TrackException(exception); - _client.Flush(); +#if DEBUG + channel.DeveloperMode = Debugger.IsAttached; +#endif + + // Avoid CreateDefault() to prevent registering default processors (e.g., adaptive sampling) + var configuration = new TelemetryConfiguration + { + ConnectionString = connectionString, + DisableTelemetry = !telemetryEnabled, + TelemetryChannel = channel + }; + + configuration.TelemetryInitializers.Add(new DefaultTelemetryInitializer()); + + // Build clean processor chain with DefaultTelemetryProcessor (disables sampling/filtering). + var processorChain = configuration.DefaultTelemetrySink.TelemetryProcessorChainBuilder; + processorChain.Use(next => new DefaultTelemetryProcessor(next)); + processorChain.Build(); + + channel.Initialize(configuration); + + return new TelemetryService(configuration); } - public void Dispose() + private static TelemetryService CreateInstance() { - _configuration.Dispose(); + var policies = new PolicyServiceFactory().Create().Current; + var consent = TelemetryConsent.Resolve(policies, UserPreferences.Current); + + return Create(TelemetrySessionInfo.ConnectionString, consent.IsEnabled, DefaultStorageFolder); } } diff --git a/src/Program.cs b/src/Program.cs index cdb81d60..6562c5d0 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -35,7 +35,14 @@ private static void Main() report.TrySave(); ErrorReportDialog.Show(report); + // An unhandled rethrow can end the process before the finally block below runs. + _ = TelemetryService.Instance.TryFlushBeforeShutdown(10_000); + throw; } + finally + { + TelemetryService.DisposeInstance(); + } } } diff --git a/src/packages.lock.json b/src/packages.lock.json index d3324a35..cdbb7785 100644 --- a/src/packages.lock.json +++ b/src/packages.lock.json @@ -85,6 +85,16 @@ "resolved": "2.23.0", "contentHash": "nWArUZTdU7iqZLycLKWe0TDms48KKGE6pONH2terYNa8REXiqixrMOkf1sk5DHGMaUTqONU2YkS4SAXBhLStgw==" }, + "Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel": { + "type": "Direct", + "requested": "[2.23.0, )", + "resolved": "2.23.0", + "contentHash": "798Dudr4tkujslk1w+XcXOcCErmVsk+nhp+QCHLa3lcgi25vkAxBmzPUeQlRJVCNL/1f4x/YF+vQZ8RSuTXWCw==", + "dependencies": { + "Microsoft.ApplicationInsights": "2.23.0", + "System.IO.FileSystem.AccessControl": "4.7.0" + } + }, "Microsoft.Identity.Client.Desktop": { "type": "Direct", "requested": "[4.73.1, )", @@ -271,6 +281,11 @@ "type": "Transitive", "resolved": "7.0.0", "contentHash": "rJJony+jsxvpfJM9ZGVxjp0DVpalZv8cAhiMSLW6L2hgUWb7k5qPVuzQHWXtkT8lrG1hQ8vWeR+HUwgCQm9J3A==" + }, + "System.IO.FileSystem.AccessControl": { + "type": "Transitive", + "resolved": "4.7.0", + "contentHash": "vMToiarpU81LR1/KZtnT7VDPvqAZfw9oOS5nY6pPP78nGYz3COLsQH3OfzbR+SjTgltd31R6KmKklz/zDpTmzw==" } }, "net10.0-windows7.0/win-x64": { @@ -299,6 +314,11 @@ "type": "Transitive", "resolved": "0.19.2", "contentHash": "qXNtYJxZ15yKV5pnI+V0sRzIH++vNzmMOU3K8kyNGsMD8/q7+ZejQjm7gJI9MMEGDPQkGg3QaiMvrIgbH2ym2g==" + }, + "System.IO.FileSystem.AccessControl": { + "type": "Transitive", + "resolved": "4.7.0", + "contentHash": "vMToiarpU81LR1/KZtnT7VDPvqAZfw9oOS5nY6pPP78nGYz3COLsQH3OfzbR+SjTgltd31R6KmKklz/zDpTmzw==" } } } diff --git a/test/Bravo.Tests/Host/HostFakes.cs b/test/Bravo.Tests/Host/HostFakes.cs index 9cb61a5d..d2f09e33 100644 --- a/test/Bravo.Tests/Host/HostFakes.cs +++ b/test/Bravo.Tests/Host/HostFakes.cs @@ -1,5 +1,8 @@ using System; +using System.Collections.Generic; using System.Net; +using System.Threading; +using System.Threading.Tasks; using Sqlbi.Bravo.Host; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Policies; @@ -38,10 +41,14 @@ internal sealed class FakeTelemetry : ITelemetryService public bool TelemetryEnabled { get; set; } - public void TrackException(Exception exception) + public void TrackException(Exception exception, IDictionary? properties = null) { } + public Task FlushAsync(CancellationToken cancellationToken = default) => Task.FromResult(true); + + public bool TryFlushBeforeShutdown(int timeoutMilliseconds) => true; + public void Dispose() => Disposed = true; } diff --git a/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs new file mode 100644 index 00000000..8a638132 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryConsentTests.cs @@ -0,0 +1,39 @@ +using Sqlbi.Bravo.Infrastructure.Configuration.Settings; +using Sqlbi.Bravo.Infrastructure.Policies; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Telemetry; + +public class TelemetryConsentTests +{ + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Resolve_WithoutPolicy_UsesTheUserSetting(bool settingValue) + { + var policies = PolicySnapshot.NotConfigured; + var settings = new UserSettings { TelemetryEnabled = settingValue }; + + var consent = TelemetryConsent.Resolve(policies, settings); + + Assert.Equal(settingValue, consent.IsEnabled); + Assert.Equal(TelemetryConsentSource.UserSettings, consent.Source); + } + + [Theory] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(true, true)] + [InlineData(false, false)] + public void Resolve_WithPolicy_OverridesTheUserSetting(bool policyValue, bool settingValue) + { + var policies = PolicySnapshot.NotConfigured with { TelemetryEnabled = policyValue }; + var settings = new UserSettings { TelemetryEnabled = settingValue }; + + var consent = TelemetryConsent.Resolve(policies, settings); + + Assert.Equal(policyValue, consent.IsEnabled); + Assert.Equal(TelemetryConsentSource.Policy, consent.Source); + } +} \ No newline at end of file diff --git a/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs new file mode 100644 index 00000000..aa5a59b9 --- /dev/null +++ b/test/Bravo.Tests/Infrastructure/Telemetry/TelemetryServiceTests.cs @@ -0,0 +1,100 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Net; +using System.Threading; +using System.Threading.Tasks; +using Sqlbi.Bravo.Infrastructure.Telemetry; +using Xunit; + +namespace Bravo.Tests.Infrastructure.Telemetry; + +/// +/// Exercises the real channel against a local ingestion endpoint: the behaviour worth protecting is +/// what reaches the wire before the process exits, and it only exists in the channel itself. +/// +public class TelemetryServiceTests +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(10); + + private static string CreateConnectionString(int port) + => $"InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=http://127.0.0.1:{port}/"; + + private static int GetFreePort() + { + using var socket = new System.Net.Sockets.TcpListener(IPAddress.Loopback, port: 0); + socket.Start(); + return ((IPEndPoint)socket.LocalEndpoint).Port; + } + + /// + /// Regression: Dispose used the synchronous Flush, which hands the buffer to the sender and + /// returns before the request is sent, so the last exceptions of a session never left the process. + /// + [Fact] + public void Dispose_TrackedExceptionsAreSentBeforeReturning() + { + var port = GetFreePort(); + using var listener = new HttpListener(); + listener.Prefixes.Add($"http://127.0.0.1:{port}/"); + listener.Start(); + + using var received = new ManualResetEventSlim(); + string? path = null; + _ = Task.Run(() => + { + var context = listener.GetContext(); + path = context.Request.Url?.AbsolutePath; + // Set before the response is sent: Dispose can only return after the response. + received.Set(); + context.Response.StatusCode = 200; + context.Response.Close(); + }); + + using var storage = new TemporaryFolder(); + var service = TelemetryService.Create(CreateConnectionString(port), telemetryEnabled: true, storage.Path); + service.TrackException(new InvalidOperationException("tracked before exit")); + + service.Dispose(); + + // Checked without waiting: the process exits right after Dispose, so a request that is + // still in flight at this point is lost. + Assert.True(received.IsSet, "The request had not reached the endpoint when Dispose returned."); + Assert.Contains("/track", path); + } + + /// + /// With no endpoint the flush must fail fast and store the transmission for the next start, + /// instead of holding the shutdown for the whole TCP timeout. + /// + [Fact] + public void Dispose_EndpointUnreachable_StoresTheTransmissionAndReturnsWithinTheBound() + { + using var storage = new TemporaryFolder(); + var service = TelemetryService.Create(CreateConnectionString(GetFreePort()), telemetryEnabled: true, storage.Path); + service.TrackException(new InvalidOperationException("tracked while offline")); + + var stopwatch = Stopwatch.StartNew(); + service.Dispose(); + + Assert.True(stopwatch.Elapsed < s_timeout, $"Dispose took {stopwatch.Elapsed}"); + Assert.NotEmpty(Directory.GetFiles(storage.Path, "*.trn")); + } + + private sealed class TemporaryFolder : IDisposable + { + public string Path { get; } = Directory.CreateDirectory(System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"Bravo.Tests.{Guid.NewGuid():N}")).FullName; + + public void Dispose() + { + try + { + Directory.Delete(Path, recursive: true); + } + catch (IOException) + { + // A file still held by the channel is left behind. + } + } + } +}