-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update how measurements are processed #196
Changes from 5 commits
2629b20
e82da7a
fc505e7
e39b7ca
86adc82
8864425
34f7b7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
@@ -45,7 +46,7 @@ public MeasurementEventNormalizationService( | |
Data.IConverter<EventData, JToken> converter, | ||
IExceptionTelemetryProcessor exceptionTelemetryProcessor, | ||
int maxParallelism, | ||
int asyncCollectorBatchSize = 200) | ||
int asyncCollectorBatchSize = 25) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any data on the impact of this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No hard numbers. I had originally put in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That might be good for now. If we have enough time to compare just this change with out the others and we see a benefit then we can include. |
||
{ | ||
_log = EnsureArg.IsNotNull(log, nameof(log)); | ||
_contentTemplate = EnsureArg.IsNotNull(contentTemplate, nameof(contentTemplate)); | ||
|
@@ -99,7 +100,6 @@ private async Task StartConsumer(ISourceBlock<EventData> producer, IEnumerableAs | |
var transformingConsumer = new TransformManyBlock<EventData, (string, IMeasurement)>( | ||
async evt => | ||
{ | ||
var createdMeasurements = new List<(string, IMeasurement)>(); | ||
try | ||
{ | ||
string partitionId = evt.SystemProperties.PartitionKey; | ||
|
@@ -115,19 +115,7 @@ private async Task StartConsumer(ISourceBlock<EventData> producer, IEnumerableAs | |
|
||
var token = _converter.Convert(evt); | ||
|
||
try | ||
{ | ||
foreach (var measurement in _contentTemplate.GetMeasurements(token)) | ||
{ | ||
measurement.IngestionTimeUtc = evt.SystemProperties.EnqueuedTimeUtc; | ||
createdMeasurements.Add((partitionId, measurement)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we can't just yield return here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately a |
||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Translate all Normalization Mapping exceptions into a common type for easy identification. | ||
throw new NormalizationDataMappingException(ex); | ||
} | ||
return CreateMeasurementIterator(token, partitionId, exceptions, errorConsumer, evt); | ||
} | ||
catch (Exception ex) | ||
{ | ||
|
@@ -137,7 +125,7 @@ private async Task StartConsumer(ISourceBlock<EventData> producer, IEnumerableAs | |
} | ||
} | ||
|
||
return createdMeasurements; | ||
return Enumerable.Empty<(string, IMeasurement)>(); | ||
}); | ||
|
||
var asyncCollectorConsumer = new ActionBlock<(string, IMeasurement)[]>( | ||
|
@@ -201,5 +189,70 @@ private Task<bool> ProcessErrorAsync(Exception ex, EventData data) | |
var handled = _exceptionTelemetryProcessor.HandleException(ex, _log); | ||
return Task.FromResult(!handled); | ||
} | ||
|
||
private IEnumerable<(string, IMeasurement)> CreateMeasurementIterator(JToken token, string partitionId, ConcurrentBag<Exception> exceptions, Func<Exception, EventData, Task<bool>> errorConsumer, EventData evt) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, we batched all Measurements per DeviceEvent before sending down the processing pipeline. If there was an error with one Measurement we wouldn't send any Measurements for that DeviceEvent. This change streams each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So one consequence of this is we have bad data and spin, we could be now sending more repeat/duplicate messages down stream? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is true if we were to spin. But I had thought that we decided not to spin, and simply discard bad data? Until we could send it over to the EMS? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will continue to spin by default though I did add the ability to change the strategy if desired. |
||
{ | ||
IEnumerator<Measurement> enumerable = null; | ||
Action<Exception> storeNormalizedException = ex => | ||
{ | ||
var normalizationException = new NormalizationDataMappingException(ex); | ||
if (errorConsumer(normalizationException, evt).ConfigureAwait(false).GetAwaiter().GetResult()) | ||
{ | ||
exceptions.Add(normalizationException); | ||
} | ||
}; | ||
|
||
try | ||
{ | ||
enumerable = _contentTemplate.GetMeasurements(token).GetEnumerator(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be we able to remove the lower while loop and just yield return as we iterate through each element here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may also want to look at simplifying this class completely. I don't know if using the TPL adds much value. We can also at switching over to async enumerables https://docs.microsoft.com/en-us/archive/msdn-magazine/2019/november/csharp-iterating-with-async-enumerables-in-csharp-8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching this to an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the other perf PR we are no longer using this class. Thoughts on reverting this change? It will still be used for the Azure Function implementation but we wouldn't be taking the risk of potentially introducing a new bug for that path. The new metric is great, and we can work to port it over to the new implementation. |
||
} | ||
catch (Exception ex) | ||
{ | ||
storeNormalizedException(ex); | ||
yield break; | ||
} | ||
|
||
(string, IMeasurement) currentValue = (partitionId, null); | ||
var shouldLoop = true; | ||
var stopWatch = new Stopwatch(); | ||
|
||
while (shouldLoop) | ||
{ | ||
try | ||
{ | ||
stopWatch.Start(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have some helper classes that simplify this. We may need to port them from PaaS to OSS. |
||
shouldLoop = enumerable.MoveNext(); | ||
stopWatch.Stop(); | ||
|
||
_log.LogMetric( | ||
IomtMetrics.NormalizedEventGenerationTimeMs(partitionId), | ||
stopWatch.ElapsedMilliseconds); | ||
|
||
stopWatch.Reset(); | ||
|
||
if (shouldLoop) | ||
{ | ||
var measurement = enumerable.Current; | ||
measurement.IngestionTimeUtc = evt.SystemProperties.EnqueuedTimeUtc; | ||
currentValue = (partitionId, measurement); | ||
} | ||
else | ||
{ | ||
break; | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Translate all Normalization Mapping exceptions into a common type for easy identification. | ||
storeNormalizedException(ex); | ||
shouldLoop = false; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dustinburson Do we skip processing the remainder of the DeviceEvent now that one Measurement produced an Exception? Or do we simply skip this Measurement, and allow the remaining Measurements to be generated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it we would keep processing but we would need to discuss further. |
||
if (shouldLoop) | ||
{ | ||
yield return currentValue; | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work? We are returning the tokenClone and them updating the same reference to remove the property. I don't think we can do the remove. Anything downstream of this will potentially be missing the MatchedToken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work as intended as this is a synchronous
IEnumerable
. We firstyield
the clonedJToken
and then produce aMeasurement
from it. Finally we return here to remove theMatchedToken
property and start the loop again. The code producing theMeasurement
does not hold onto a reference for the token it was given.We have various unit tests which should be testing out this scenario, such as: https://github.com/microsoft/iomt-fhir/blob/main/test/Microsoft.Health.Fhir.Ingest.Template.UnitTests/CalculatedFunctionContentTemplateTests.cs#L79