From 877a4b733ddacf41bcb41d6b106f9c5a28dc5833 Mon Sep 17 00:00:00 2001 From: Rohit Ranjan <90008725+RohitRanjanMS@users.noreply.github.com> Date: Mon, 12 Aug 2024 13:51:53 -0700 Subject: [PATCH 1/2] Inital commit. --- .../Diagnostics/DiagnosticEventLogger.cs | 10 +++++----- .../Diagnostics/DiagnosticEventNullRepository.cs | 12 +++++------- .../DiagnosticEventTableStorageRepository.cs | 10 +++++++++- .../Diagnostics/IDiagnosticEventRepository.cs | 4 ++-- .../DiagnosticEventTableStorageRepositoryTests.cs | 8 ++++++-- .../TestDiagnosticEventRepository.cs | 5 +++++ 6 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs index 8a615f36db..110cd13c7d 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs @@ -54,13 +54,13 @@ private bool IsDiagnosticEvent(IDictionary state) public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) { - if (!IsEnabled(logLevel)) + if (_diagnosticEventRepository is null || _diagnosticEventRepository.IsEnabled()) { - return; - } + if (!IsEnabled(logLevel) || !(state is IDictionary stateInfo && IsDiagnosticEvent(stateInfo))) + { + return; + } - if (state is IDictionary stateInfo && IsDiagnosticEvent(stateInfo)) - { string message = formatter(state, exception); if (_diagnosticEventRepository == null) { diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventNullRepository.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventNullRepository.cs index c1034fc001..b97fde8784 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventNullRepository.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventNullRepository.cs @@ -2,13 +2,6 @@ // Licensed under the MIT License. See License.txt in the project root for license information. using System; -using System.Collections.Concurrent; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Azure.Cosmos.Table; -using Microsoft.Azure.WebJobs.Host.Executors; -using Microsoft.Azure.WebJobs.Script.WebHost.Helpers; -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics @@ -16,5 +9,10 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics public class DiagnosticEventNullRepository : IDiagnosticEventRepository { public void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel level, string message, string helpLink, Exception exception) { } + + public bool IsEnabled() + { + return false; + } } } \ No newline at end of file diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs index bd3c10932a..6f21614d6e 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs @@ -39,6 +39,7 @@ public class DiagnosticEventTableStorageRepository : IDiagnosticEventRepository, private bool _disposed = false; private bool _purged = false; private string _tableName; + private bool _isEnabled = true; internal DiagnosticEventTableStorageRepository(IConfiguration configuration, IHostIdProvider hostIdProvider, IEnvironment environment, IScriptHostManager scriptHostManager, ILogger logger, int logFlushInterval) @@ -69,7 +70,9 @@ internal TableServiceClient TableClient } catch (Exception ex) { - _logger.LogError(ex, "Azure Storage connection string is empty or invalid. Unable to write diagnostic events."); + _logger.LogError(ex, "The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped."); + _isEnabled = false; + StopTimer(); } } @@ -269,6 +272,11 @@ public void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel } } + public bool IsEnabled() + { + return _isEnabled; + } + private bool IsPrimaryHost() { var primaryHostStateProvider = _serviceProvider?.GetService(); diff --git a/src/WebJobs.Script.WebHost/Diagnostics/IDiagnosticEventRepository.cs b/src/WebJobs.Script.WebHost/Diagnostics/IDiagnosticEventRepository.cs index 28c50b67e3..6db85da2be 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/IDiagnosticEventRepository.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/IDiagnosticEventRepository.cs @@ -2,8 +2,6 @@ // Licensed under the MIT License. See License.txt in the project root for license information. using System; -using System.Collections.Generic; -using System.Threading.Tasks; using Microsoft.Extensions.Logging; namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics @@ -11,5 +9,7 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics public interface IDiagnosticEventRepository { void WriteDiagnosticEvent(DateTime timestamp, string errorCode, LogLevel level, string message, string helpLink, Exception exception); + + bool IsEnabled(); } } \ No newline at end of file diff --git a/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs b/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs index ebbb6f3198..f739bd0db1 100644 --- a/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs +++ b/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs @@ -153,11 +153,15 @@ public void GetDiagnosticEventsTable_LogsError_StorageConnectionStringIsNotPrese var configuration = new ConfigurationBuilder().Build(); DiagnosticEventTableStorageRepository repository = new DiagnosticEventTableStorageRepository(configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger); + + // The repository should be initially enabled and then disabled after TableServiceClient failure. + Assert.True(repository.IsEnabled()); DateTime dateTime = new DateTime(2021, 1, 1); var cloudTable = repository.GetDiagnosticEventsTable(dateTime); Assert.Null(cloudTable); var messages = _loggerProvider.GetAllLogMessages(); - Assert.Equal(messages[0].FormattedMessage, "Azure Storage connection string is empty or invalid. Unable to write diagnostic events."); + Assert.Equal(messages[0].FormattedMessage, "The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped."); + Assert.False(repository.IsEnabled()); } [Fact] @@ -274,7 +278,7 @@ public async Task FlushLogs_LogsErrorAndClearsEvents_WhenTableCreatingFails() var logMessage = _loggerProvider.GetAllLogMessages().SingleOrDefault(m => m.FormattedMessage.Contains("Unable to get table reference")); Assert.NotNull(logMessage); - var messagePresent = _loggerProvider.GetAllLogMessages().Any(m => m.FormattedMessage.Contains("Azure Storage connection string is empty or invalid. Unable to write diagnostic events.")); + var messagePresent = _loggerProvider.GetAllLogMessages().Any(m => m.FormattedMessage.Contains("The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped.")); Assert.True(messagePresent); Assert.Equal(0, repository.Events.Values.Count()); diff --git a/test/WebJobs.Script.Tests.Shared/TestDiagnosticEventRepository.cs b/test/WebJobs.Script.Tests.Shared/TestDiagnosticEventRepository.cs index f0f00b7547..ad86226885 100644 --- a/test/WebJobs.Script.Tests.Shared/TestDiagnosticEventRepository.cs +++ b/test/WebJobs.Script.Tests.Shared/TestDiagnosticEventRepository.cs @@ -35,4 +35,9 @@ public void FlushLogs() { Events.Clear(); } + + public bool IsEnabled() + { + return true; + } } From 786c5a37516b292e4ab3179d053c0822d0a63ba9 Mon Sep 17 00:00:00 2001 From: Rohit Ranjan <90008725+RohitRanjanMS@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:38:26 -0700 Subject: [PATCH 2/2] Added GetProperties(). --- .../Diagnostics/DiagnosticEventLogger.cs | 21 ++++++++----------- .../DiagnosticEventTableStorageRepository.cs | 4 ++++ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs index 110cd13c7d..87ca6297ba 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventLogger.cs @@ -54,20 +54,17 @@ private bool IsDiagnosticEvent(IDictionary state) public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) { - if (_diagnosticEventRepository is null || _diagnosticEventRepository.IsEnabled()) + // Exit early if logging is not enabled or the state is not a diagnostic event + if (!IsEnabled(logLevel) || + (_diagnosticEventRepository != null && !_diagnosticEventRepository.IsEnabled()) || + !(state is IDictionary stateInfo && IsDiagnosticEvent(stateInfo))) { - if (!IsEnabled(logLevel) || !(state is IDictionary stateInfo && IsDiagnosticEvent(stateInfo))) - { - return; - } - - string message = formatter(state, exception); - if (_diagnosticEventRepository == null) - { - _diagnosticEventRepository = _diagnosticEventRepositoryFactory.Create(); - } - _diagnosticEventRepository.WriteDiagnosticEvent(DateTime.UtcNow, stateInfo[ScriptConstants.ErrorCodeKey].ToString(), logLevel, message, stateInfo[ScriptConstants.HelpLinkKey].ToString(), exception); + return; } + + string message = formatter(state, exception); + _diagnosticEventRepository ??= _diagnosticEventRepositoryFactory.Create(); + _diagnosticEventRepository.WriteDiagnosticEvent(DateTime.UtcNow, stateInfo[ScriptConstants.ErrorCodeKey].ToString(), logLevel, message, stateInfo[ScriptConstants.HelpLinkKey].ToString(), exception); } public void Dispose() diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs index 6f21614d6e..a57b4799ac 100644 --- a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs +++ b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs @@ -67,6 +67,10 @@ internal TableServiceClient TableClient try { _tableClient = new TableServiceClient(storageConnectionString); + + // The TableServiceClient only verifies the format of the connection string. + // To ensure the storage account exists and supports Table storage, validate the connection string by retrieving the properties of the table service. + _ = _tableClient.GetProperties(); } catch (Exception ex) {