Fix: correlation test fail except for external event with extended session

[TsuyoshiUshio]

Hi @cgillum

I found the root cause why the correlation test fails.
TaskHubClient changes the behavior. CorrelationTraceClient.Propagate(() => CreateAndTrackDependencyTelemetry(requestTraceContext)); Should be coming before a method that eventually call the TaskHubQueue.

This is the old code.
https://github.com/TsuyoshiUshio/durabletask/blob/correlation/final/src/DurableTask.Core/TaskHubClient.cs#L497-L501

TODO

Only the External Event with non-extended session will fail. I'm not fully understand the new behavior, however, it is getting very close.

[cgillum]

Thanks for debugging this, and sorry that I broke it when merging changes. :(

[TsuyoshiUshio]

The change I introduced

External Event test was failed. The reason was, the old version as false positive. ExecutionStarted and EventRaiseEvent is coming at the same time, It works. However, ExecutionStated and EventRaiseEvnet coming as different timing, EventRaiseEvent's one can not find the parent trace context. I put the traceContext on ExecutionStarted to trackingStore. If the EventRaiseEvent happens, and the ParentTraceContext is empty, I restored the context from the tracking Store.

• Save the TraceContext to the trackingStore when the orchestration Started
• Restore the TraceContext when the EventRaiseEvent and ParentTraceContext is null
• Add CorrelationTraceClinet.PropageteAsync for the Async execution of the Correlation Logic
• Add MultipleParentMultilayer test for testing ChildOrchestration with RaiseEvent.

Point of the Review

What I'm not sure how an Orchestrator with RaiseEvent find the your orchestrator's ExecutionStarted history?
Currently, I just sort the history and pick up the latest. However, I'm not sure if this logic is proper or not.
https://github.com/TsuyoshiUshio/durabletask/blob/tsushi/fixtest/src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs#L812

[cgillum]

How large do you think this data will be? I think you should also update estimatedBytes here so that we don't overflow the maximum batch size for Azure Tables.

Also, please update the AzureTableTrackingStore.VariableSizeEntityProperties array by adding your new "Correlation" property name so that we can correctly keep track of whether we need to compress the data and upload it to blob storage.

[cgillum]

Actually, since you added the Correlation property to the history event class in DurableTask.Core, I don't think you need to do this at all. Just assign it to ExecutionStartedEvent.Correlation and we will use reflection to put it into table storage. In fact, would it make more sense to assign the Correlation property in the DurableTask.Core project? It seems like you don't actually need to do anything specific in DurableTask.AzureStorage for this.

[TsuyoshiUshio]

Done for estimatedBytes and AzureTableTrackingStore.VariableSizeEntityProperties However, I couldn't figure out where should I updated ExecutionStartedEvent.Correlation It is not here, right? Today is times up, I'll try tomorrow, however, If you know where it is, please let me know.

[TsuyoshiUshio]

Hi @cgillum
I assign the ExecutionStartedEvent.Correlation on the DurableTask.Core side. Do you mean like this?
11725f8
All test has passed.

[TsuyoshiUshio]

What I've done

• Move Correlation from History to EventStartedEvent
• Read History from session
• update estimatedBytes and AzureTableTrackingStore.VariableSizeEntityProperties

TODO

Where should I insert the Correlation. I keep this logic. I need to figure out where to put the assignment code for ExecutionStartedEvent.Correlation. Today is time up. Tomorrow, I'll try again, however, if you already knew it, please let me know.

 historyEntity.Properties["Correlation"] = new EntityProperty(traceContext.SerializableTraceContext);

I might read more code releated estimatedBytes. I just calculate it from the actual string length of Correlation.

[TsuyoshiUshio]

@cgillum
I accept all your change request. And It passes correlation tests.
Let's execute all the test on the ci when you merge correlation branch to master.