Merge Correlation branch into master #422
Conversation
Minor code cleanup
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 422 in repo Azure/durabletask |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
|
||
| ### [TelemetryActivator](../TelemetryActivator.cs) | ||
|
|
||
| TelemetryActivator has a responsibility to track telemetry. Work with CorrelationTraceClient with giving Lambda to activate Application Insights. This class is a client-side implementation. So this class is **NOT** included in DurableTask namespaces. |
There was a problem hiding this comment.
not sure what is meant by with giving Lambda to activate Application Insights?
There was a problem hiding this comment.
CorrelationTraceClient is initialized by Setup method. https://github.com/Azure/durabletask/blob/correlation/src/DurableTask.Core/CorrelationTraceClient.cs#L42 The implementation is hidden on the Action . They don't have any dependency on Application Insights.
For the dependency of the Application Insights, configure the TelemetryActivator to enable Correlation feature.
https://github.com/Azure/durabletask/blob/correlation/samples/Correlation.Samples/TelemetryActivator.cs#L34-L51
|
Aha! So the test that's failing is not quite a bug I introduced. The test passes after Tsuyoshi's fix branch was merged here, but it fails after we pulled the latest master, which must have automerged some things incorrectly. I'll review the diff tomorrow and hopefully get this fixed! |
|
Another update: We appear to have a Heisenbug in our hands. When git-checking the commit that merges Tsuyoshi's Fix PR into this branch, the test will succeed when invoked normally but will fail when invoked with a debugger. I'll consult with Tsuyoshi tomorrow to try to get to the bottom of this :) |
…, currently with dummy test for CI sanity check
|
I think this branch is ready to merge now. We've fixed the failing tests and bypassed the test-discovery exception we were seeing in the CI. I'll create a ticket for us to investigate the CI test discovery config, as it took a lot of time to diagnose and circumvent. |
|
Thanks for the hard work on this! I guess our CI is pretty sensitive to changes in test projects. Looking forward to the result of that investigation. I'll go ahead and merge this PR. |
|
Tracking issue: #473 |
The private preview was a success. Let's take the next step and get this merged into the main branch so it can be shipped with our nuget releases.
We'll want to do another pass on these changes to make sure we're not taking any changes that we don't want shipping.
FYI @TsuyoshiUshio