From d0f874b7c4f9bfe44755669e4d5b6bf59bf1ce5c Mon Sep 17 00:00:00 2001 From: Rob Gordon Date: Thu, 8 Jul 2021 21:48:14 -0700 Subject: [PATCH 1/5] Replacing System.AggregateException with custom aggregate exception --- .../Service/ParallelTaskWorker.cs | 2 +- .../SimpleAggregateException.cs | 54 +++++++++++++++++++ .../MeasurementEventNormalizationService.cs | 3 +- ...asurementEventNormalizationServiceTests.cs | 27 +++++++++- .../MeasurementFhirImportServiceTests.cs | 3 +- 5 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 src/lib/Microsoft.Health.Common/SimpleAggregateException.cs diff --git a/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs b/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs index 6a57a7c8..b6378130 100644 --- a/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs +++ b/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs @@ -66,7 +66,7 @@ await consumer.Completion { if (!exceptions.IsEmpty) { - throw new AggregateException(exceptions); + throw new SimpleAggregateException(exceptions); } }, cts.Token, diff --git a/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs b/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs new file mode 100644 index 00000000..f35e7c50 --- /dev/null +++ b/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs @@ -0,0 +1,54 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System; +using System.Collections.Generic; +using System.Text; +using EnsureThat; + +namespace Microsoft.Health.Common +{ + public class SimpleAggregateException : Exception + { + private readonly string _aggregatedMessage; + + public SimpleAggregateException(IEnumerable exceptions) + : this(new List(exceptions)) + { + } + + public SimpleAggregateException(ICollection exceptions) + : this("One or more exceptions have occured", exceptions) + { + } + + public SimpleAggregateException(string message, ICollection exceptions) + { + EnsureArg.IsNotNullOrWhiteSpace(message, nameof(message)); + EnsureArg.HasItems(exceptions, nameof(exceptions)); + + _aggregatedMessage = ConstructMessage(message, exceptions); + InnerExceptions = new List(exceptions); + } + + public override string Message => _aggregatedMessage; + + public IReadOnlyCollection InnerExceptions { get; } + + private string ConstructMessage(string message, IEnumerable exceptions) + { + var exceptionMessage = new StringBuilder(message); + + exceptionMessage.AppendLine(":"); + + foreach (var exception in exceptions) + { + exceptionMessage.AppendLine($"---- {exception.GetType()}: {exception.Message}"); + } + + return exceptionMessage.ToString(); + } + } +} diff --git a/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs b/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs index 5dfd98a3..859cc4b8 100644 --- a/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs +++ b/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs @@ -12,6 +12,7 @@ using EnsureThat; using Microsoft.Azure.EventHubs; using Microsoft.Azure.WebJobs; +using Microsoft.Health.Common; using Microsoft.Health.Fhir.Ingest.Data; using Microsoft.Health.Fhir.Ingest.Telemetry; using Microsoft.Health.Fhir.Ingest.Template; @@ -124,7 +125,7 @@ await consumer.Completion { if (!exceptions.IsEmpty) { - throw new AggregateException(exceptions); + throw new SimpleAggregateException(exceptions); } }, cts.Token, diff --git a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs index 8ad0ef70..33f42c5e 100644 --- a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs +++ b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs @@ -8,6 +8,7 @@ using System.Threading.Tasks; using Microsoft.Azure.EventHubs; using Microsoft.Azure.WebJobs; +using Microsoft.Health.Common; using Microsoft.Health.Fhir.Ingest.Data; using Microsoft.Health.Fhir.Ingest.Template; using Microsoft.Health.Logging.Telemetry; @@ -106,6 +107,30 @@ await consumer.Received(1) } } + [Fact] + public async Task GivenEvents_WhenProcessAsync_AndGetMeasurementThrowsException_ThenNoEventsAreConsumed_Test() + { + var template = Substitute.For(); + + template.GetMeasurements(null).ReturnsForAnyArgs(arg => throw new Exception("Mock Exception")); + + var converter = Substitute.For>(); + + var events = Enumerable.Range(0, 10).Select(i => BuildEvent(i)).ToArray(); + + var log = Substitute.For(); + + var consumer = Substitute.For>(); + + var srv = new MeasurementEventNormalizationService(log, template, converter, 1); + var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); + + Assert.Equal(events.Length, exception.InnerExceptions.Count); + template.ReceivedWithAnyArgs(10).GetMeasurements(null); + converter.ReceivedWithAnyArgs(10).Convert(null); + await consumer.ReceivedWithAnyArgs(0).AddAsync(null); + } + [Fact] public async Task GivenEventsAndDefaultErrorConsumer_WhenProcessAsyncAndConsumerErrors_ThenEachEventResultConsumedAndErrorProprogated_Test() { @@ -121,7 +146,7 @@ public async Task GivenEventsAndDefaultErrorConsumer_WhenProcessAsyncAndConsumer consumer.AddAsync(null).ReturnsForAnyArgs(v => Task.FromException(new Exception())); var srv = new MeasurementEventNormalizationService(log, template, converter, 1); - var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); + var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); Assert.Equal(events.Length, exception.InnerExceptions.Count); template.ReceivedWithAnyArgs(events.Length).GetMeasurements(null); diff --git a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs index 7d076a82..319c925d 100644 --- a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs +++ b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs @@ -8,6 +8,7 @@ using System.IO; using System.Linq; using System.Threading.Tasks; +using Microsoft.Health.Common; using Microsoft.Health.Common.Config; using Microsoft.Health.Common.Telemetry; using Microsoft.Health.Fhir.Ingest.Config; @@ -76,7 +77,7 @@ public async void GivenExceptionDuringParseStreamAsync_WhenParseStreamAsync_Then var measurements = new MeasurementGroup[] { Substitute.For(), Substitute.For() }; - var aggEx = await Assert.ThrowsAsync(async () => await fhirImport.ProcessStreamAsync(ToStream(measurements), string.Empty, log)); + var aggEx = await Assert.ThrowsAsync(async () => await fhirImport.ProcessStreamAsync(ToStream(measurements), string.Empty, log)); Assert.Collection( aggEx.InnerExceptions, From 22b81084457194991ca9bde0c7e530ad288cc82c Mon Sep 17 00:00:00 2001 From: Rob Gordon Date: Thu, 8 Jul 2021 22:14:05 -0700 Subject: [PATCH 2/5] Adding unit test --- .../SimpleAggregateExceptionTest.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs diff --git a/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs b/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs new file mode 100644 index 00000000..a1039a60 --- /dev/null +++ b/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs @@ -0,0 +1,50 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System; +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace Microsoft.Health.Common.UnitTests +{ + public class SimpleAggregateExceptionTest + { + [Fact] + public void When_BadArgumentsSupplied_ExceptionIsThrown() + { + Assert.Throws(() => new SimpleAggregateException(new List())); + Assert.Throws(() => new SimpleAggregateException(string.Empty, new List() { new Exception() })); + } + + [Fact] + public void When_SuppliedWithExceptions_ExpectedMessageCreated() + { + var exceptions = Enumerable.Range(0, 3).Select(i => new Exception("Mock Exception")).ToList(); + var expectedMessage = @"One or more exceptions have occured: +---- System.Exception: Mock Exception +---- System.Exception: Mock Exception +---- System.Exception: Mock Exception +"; + var exception = new SimpleAggregateException(exceptions); + Assert.Equal(3, exception.InnerExceptions.Count); + Assert.Equal(expectedMessage, exception.Message); + } + + [Fact] + public void When_SuppliedWithExceptions_AndMessage_ExpectedMessageCreated() + { + var exceptions = Enumerable.Range(0, 3).Select(i => new Exception("Mock Exception")).ToList(); + var expectedMessage = @"Custom exception message: +---- System.Exception: Mock Exception +---- System.Exception: Mock Exception +---- System.Exception: Mock Exception +"; + var exception = new SimpleAggregateException("Custom exception message", exceptions); + Assert.Equal(3, exception.InnerExceptions.Count); + Assert.Equal(expectedMessage, exception.Message); + } + } +} From 84be85eca58840425b8455db6d718e108643b6b5 Mon Sep 17 00:00:00 2001 From: Rob Gordon Date: Fri, 9 Jul 2021 10:26:23 -0700 Subject: [PATCH 3/5] Revert "Replacing System.AggregateException with custom aggregate exception" This reverts commit d0f874b7c4f9bfe44755669e4d5b6bf59bf1ce5c. --- .../Service/ParallelTaskWorker.cs | 2 +- .../SimpleAggregateException.cs | 54 ------------------- .../MeasurementEventNormalizationService.cs | 3 +- ...asurementEventNormalizationServiceTests.cs | 27 +--------- .../MeasurementFhirImportServiceTests.cs | 3 +- 5 files changed, 4 insertions(+), 85 deletions(-) delete mode 100644 src/lib/Microsoft.Health.Common/SimpleAggregateException.cs diff --git a/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs b/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs index b6378130..6a57a7c8 100644 --- a/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs +++ b/src/lib/Microsoft.Health.Common/Service/ParallelTaskWorker.cs @@ -66,7 +66,7 @@ await consumer.Completion { if (!exceptions.IsEmpty) { - throw new SimpleAggregateException(exceptions); + throw new AggregateException(exceptions); } }, cts.Token, diff --git a/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs b/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs deleted file mode 100644 index f35e7c50..00000000 --- a/src/lib/Microsoft.Health.Common/SimpleAggregateException.cs +++ /dev/null @@ -1,54 +0,0 @@ -// ------------------------------------------------------------------------------------------------- -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. -// ------------------------------------------------------------------------------------------------- - -using System; -using System.Collections.Generic; -using System.Text; -using EnsureThat; - -namespace Microsoft.Health.Common -{ - public class SimpleAggregateException : Exception - { - private readonly string _aggregatedMessage; - - public SimpleAggregateException(IEnumerable exceptions) - : this(new List(exceptions)) - { - } - - public SimpleAggregateException(ICollection exceptions) - : this("One or more exceptions have occured", exceptions) - { - } - - public SimpleAggregateException(string message, ICollection exceptions) - { - EnsureArg.IsNotNullOrWhiteSpace(message, nameof(message)); - EnsureArg.HasItems(exceptions, nameof(exceptions)); - - _aggregatedMessage = ConstructMessage(message, exceptions); - InnerExceptions = new List(exceptions); - } - - public override string Message => _aggregatedMessage; - - public IReadOnlyCollection InnerExceptions { get; } - - private string ConstructMessage(string message, IEnumerable exceptions) - { - var exceptionMessage = new StringBuilder(message); - - exceptionMessage.AppendLine(":"); - - foreach (var exception in exceptions) - { - exceptionMessage.AppendLine($"---- {exception.GetType()}: {exception.Message}"); - } - - return exceptionMessage.ToString(); - } - } -} diff --git a/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs b/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs index 859cc4b8..5dfd98a3 100644 --- a/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs +++ b/src/lib/Microsoft.Health.Fhir.Ingest/Service/MeasurementEventNormalizationService.cs @@ -12,7 +12,6 @@ using EnsureThat; using Microsoft.Azure.EventHubs; using Microsoft.Azure.WebJobs; -using Microsoft.Health.Common; using Microsoft.Health.Fhir.Ingest.Data; using Microsoft.Health.Fhir.Ingest.Telemetry; using Microsoft.Health.Fhir.Ingest.Template; @@ -125,7 +124,7 @@ await consumer.Completion { if (!exceptions.IsEmpty) { - throw new SimpleAggregateException(exceptions); + throw new AggregateException(exceptions); } }, cts.Token, diff --git a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs index 33f42c5e..8ad0ef70 100644 --- a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs +++ b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementEventNormalizationServiceTests.cs @@ -8,7 +8,6 @@ using System.Threading.Tasks; using Microsoft.Azure.EventHubs; using Microsoft.Azure.WebJobs; -using Microsoft.Health.Common; using Microsoft.Health.Fhir.Ingest.Data; using Microsoft.Health.Fhir.Ingest.Template; using Microsoft.Health.Logging.Telemetry; @@ -107,30 +106,6 @@ await consumer.Received(1) } } - [Fact] - public async Task GivenEvents_WhenProcessAsync_AndGetMeasurementThrowsException_ThenNoEventsAreConsumed_Test() - { - var template = Substitute.For(); - - template.GetMeasurements(null).ReturnsForAnyArgs(arg => throw new Exception("Mock Exception")); - - var converter = Substitute.For>(); - - var events = Enumerable.Range(0, 10).Select(i => BuildEvent(i)).ToArray(); - - var log = Substitute.For(); - - var consumer = Substitute.For>(); - - var srv = new MeasurementEventNormalizationService(log, template, converter, 1); - var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); - - Assert.Equal(events.Length, exception.InnerExceptions.Count); - template.ReceivedWithAnyArgs(10).GetMeasurements(null); - converter.ReceivedWithAnyArgs(10).Convert(null); - await consumer.ReceivedWithAnyArgs(0).AddAsync(null); - } - [Fact] public async Task GivenEventsAndDefaultErrorConsumer_WhenProcessAsyncAndConsumerErrors_ThenEachEventResultConsumedAndErrorProprogated_Test() { @@ -146,7 +121,7 @@ public async Task GivenEventsAndDefaultErrorConsumer_WhenProcessAsyncAndConsumer consumer.AddAsync(null).ReturnsForAnyArgs(v => Task.FromException(new Exception())); var srv = new MeasurementEventNormalizationService(log, template, converter, 1); - var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); + var exception = await Assert.ThrowsAsync(() => srv.ProcessAsync(events, consumer)); Assert.Equal(events.Length, exception.InnerExceptions.Count); template.ReceivedWithAnyArgs(events.Length).GetMeasurements(null); diff --git a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs index 319c925d..7d076a82 100644 --- a/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs +++ b/test/Microsoft.Health.Fhir.Ingest.UnitTests/Service/MeasurementFhirImportServiceTests.cs @@ -8,7 +8,6 @@ using System.IO; using System.Linq; using System.Threading.Tasks; -using Microsoft.Health.Common; using Microsoft.Health.Common.Config; using Microsoft.Health.Common.Telemetry; using Microsoft.Health.Fhir.Ingest.Config; @@ -77,7 +76,7 @@ public async void GivenExceptionDuringParseStreamAsync_WhenParseStreamAsync_Then var measurements = new MeasurementGroup[] { Substitute.For(), Substitute.For() }; - var aggEx = await Assert.ThrowsAsync(async () => await fhirImport.ProcessStreamAsync(ToStream(measurements), string.Empty, log)); + var aggEx = await Assert.ThrowsAsync(async () => await fhirImport.ProcessStreamAsync(ToStream(measurements), string.Empty, log)); Assert.Collection( aggEx.InnerExceptions, From 5d550a1a4fe1afcf6249d25292916d40e1a520bb Mon Sep 17 00:00:00 2001 From: Rob Gordon Date: Fri, 9 Jul 2021 10:27:00 -0700 Subject: [PATCH 4/5] Revert "Adding unit test" This reverts commit 22b81084457194991ca9bde0c7e530ad288cc82c. --- .../SimpleAggregateExceptionTest.cs | 50 ------------------- 1 file changed, 50 deletions(-) delete mode 100644 test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs diff --git a/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs b/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs deleted file mode 100644 index a1039a60..00000000 --- a/test/Microsoft.Health.Common.UnitTests/SimpleAggregateExceptionTest.cs +++ /dev/null @@ -1,50 +0,0 @@ -// ------------------------------------------------------------------------------------------------- -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. -// ------------------------------------------------------------------------------------------------- - -using System; -using System.Collections.Generic; -using System.Linq; -using Xunit; - -namespace Microsoft.Health.Common.UnitTests -{ - public class SimpleAggregateExceptionTest - { - [Fact] - public void When_BadArgumentsSupplied_ExceptionIsThrown() - { - Assert.Throws(() => new SimpleAggregateException(new List())); - Assert.Throws(() => new SimpleAggregateException(string.Empty, new List() { new Exception() })); - } - - [Fact] - public void When_SuppliedWithExceptions_ExpectedMessageCreated() - { - var exceptions = Enumerable.Range(0, 3).Select(i => new Exception("Mock Exception")).ToList(); - var expectedMessage = @"One or more exceptions have occured: ----- System.Exception: Mock Exception ----- System.Exception: Mock Exception ----- System.Exception: Mock Exception -"; - var exception = new SimpleAggregateException(exceptions); - Assert.Equal(3, exception.InnerExceptions.Count); - Assert.Equal(expectedMessage, exception.Message); - } - - [Fact] - public void When_SuppliedWithExceptions_AndMessage_ExpectedMessageCreated() - { - var exceptions = Enumerable.Range(0, 3).Select(i => new Exception("Mock Exception")).ToList(); - var expectedMessage = @"Custom exception message: ----- System.Exception: Mock Exception ----- System.Exception: Mock Exception ----- System.Exception: Mock Exception -"; - var exception = new SimpleAggregateException("Custom exception message", exceptions); - Assert.Equal(3, exception.InnerExceptions.Count); - Assert.Equal(expectedMessage, exception.Message); - } - } -} From f8c322d82ebf465e7c3cf1ea45ff568523c9e371 Mon Sep 17 00:00:00 2001 From: Rob Gordon Date: Fri, 9 Jul 2021 10:57:52 -0700 Subject: [PATCH 5/5] Log inner exception when AggregateException is received --- .../Telemetry/IomtTelemetryLogger.cs | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/lib/Microsoft.Health.Logger/Telemetry/IomtTelemetryLogger.cs b/src/lib/Microsoft.Health.Logger/Telemetry/IomtTelemetryLogger.cs index 5647c75a..550f763a 100644 --- a/src/lib/Microsoft.Health.Logger/Telemetry/IomtTelemetryLogger.cs +++ b/src/lib/Microsoft.Health.Logger/Telemetry/IomtTelemetryLogger.cs @@ -28,7 +28,15 @@ public virtual void LogMetric(Common.Telemetry.Metric metric, double metricValue public virtual void LogError(Exception ex) { - _telemetryClient.TrackException(ex); + if (ex is AggregateException e) + { + // Address bug https://github.com/microsoft/iomt-fhir/pull/120 + LogAggregateException(e); + } + else + { + _telemetryClient.TrackException(ex); + } } public virtual void LogTrace(string message) @@ -41,5 +49,18 @@ public void LogMetricWithDimensions(Common.Telemetry.Metric metric, double metri EnsureArg.IsNotNull(metric); metric.LogMetric(_telemetryClient, metricValue); } + + private void LogAggregateException(AggregateException e) + { + if (e.InnerException != null) + { + _telemetryClient.TrackException(e.InnerException); + } + + foreach (var exception in e.InnerExceptions) + { + _telemetryClient.TrackException(exception); + } + } } }