Merge Correlation Branch into Dev

[davidmrdavid]

Since Azure/durabletask#422 is now merged into DTFx's master branch and is soon to released, it makes sense for us to start updating the correlation branch of the durable-extension in preparation for that dependency.

I'm opening this PR for two reasons:
(1) As an excuse to bring it's contents up to date with dev's latest changes
(2) To start getting reviews out of the way so we can merge this feature to the master branch as soon as the upcoming DTFx release is out.

Note that the code here was not written by me, so I too will be reviewing it for the first time and proposing changes.

[davidmrdavid]

Left some preliminary thoughts about the code. Mostly ignored going over the READMEs and dependency updates. Please let me know y'alls thoughts as well. Overall, this is already in pretty good shape, it just needs a few touch-ups before merge!

[davidmrdavid]

I also expect the tests to be failing at this point, as there are missing dependencies

[ConnorMcMahon]

Made it through the samples, will go through the extension source/test code changes tomorrow.

[ConnorMcMahon]

Some more comments of things to address.

[davidmrdavid]

This is the leftover ToDos issue, for follow-up tasks after merging this: #1576

[ConnorMcMahon]

Getting closer, just a few nits and new things I found + some reminders.

Let me know if you need help investigating timeouts and the emulator tests.

[ConnorMcMahon]

This would be a bit cleaner as the following:

if (CorrelationTraceContext.Current is W3CTraceContext correlationContext)
{
    var trace = new TraceTelemetry(
        $"Activity Id: {correlationContext.TraceParent} ParentSpanId: {correlationContext.ParentSpanId}");
    trace.Context.Operation.Id = correlationContext.TelemetryContextOperationId;
    trace.Context.Operation.ParentId = correlationContext.TelemetryContextOperationParentId;
    this.telemetryClient.Track(trace);
}

[davidmrdavid]

I'm not convinced that's clearer 😄 .

Judging by the names of this demo, the correlation context is always meant to be a W3CTraceContext so I don't see why we should replace the as coercion to an if-check.

[ConnorMcMahon]

I don't love null coalescing after an as conversion. It means that if for some reason the customer switched the CorrelationContext away from W3CTraceContext after we added support for a new protocol, then this would start failing in weird ways (specifically, a bunch of fields would silently go null.

I guess in that case, a preferable piece of code would look like this:

if (!CorrelationTraceContext.Current is W3CTraceContext correlationContext)
{
    throw new InvalidOperationException($"This sample expects a correlation trace context of {nameof(W3CTraceContext)}, but the context isof type {CorrelationTraceContext.Current.GetType()}");
}

var trace = new TraceTelemetry(
    $"Activity Id: {correlationContext.TraceParent} ParentSpanId: {correlationContext.ParentSpanId}");
trace.Context.Operation.Id = correlationContext.TelemetryContextOperationId;
trace.Context.Operation.ParentId = correlationContext.TelemetryContextOperationParentId;
this.telemetryClient.Track(trace);

[davidmrdavid]

Ah yes, I see what you mean. I wasn't thinking of the full implications of the coalescing operation. I'll go for your new suggestion. Thanks!

[ConnorMcMahon]

We don't need to ever let customers inject their own custom implementation of this interface, right?

[davidmrdavid]

@TsuyoshiUshio , could you please review connor's question above ^.

As for my own guess, think the answer is no. I don't see the use-case.

[TsuyoshiUshio]

No. We don't need to let customer inject their won custom implementation.
Some customer want open telemetry however, in this case, it should be done by PR, since there are other strategy classes are there e.g. W3CTraceContext ... and HttpCorrelationProtocol...

[davidmrdavid]

Fair enough. Sounds to me then like there's merit in leaving the possibility for customers to provide their customer implementations then.

[ConnorMcMahon]

Let's leave it as AddSingleton for now, and if customers need to provide their own implementation later, we can open that up by converting this to TryAddSingleton

[ConnorMcMahon]

From what I can tell this is never not provided, at least in production, because we inject it via Dependency Injection. If it is null in our test code, let's just feed in a no-op version of the interface.

[davidmrdavid]

By "this" did you mean the telemetryActivator or the errorSerializerSettingsFactory ?

[davidmrdavid]

I've also been playing with how to simplify this conditional compilation bit. It looks like it could be simplified since errorSerializerSettingsFactory is always present, but there's no clean way to add it without violating linter-rules. There are also a few places where this constructor's optional parameters are called without explicitly specifying the parameter's names, so the order of params needs to remain intact.

I would personally vote to keep this as-is, for now. But perhaps you have a perspective I'm missing here, what do you think?

[ConnorMcMahon]

I was referring to telemetryActivator, but I think I am fine leaving it for now.

[davidmrdavid]

With the latest commit, we're finally passing the tests. I've also updated the ToDos issue with the latest findings. I believe this is pretty much ready to merge, but happy to take on other suggestions. Let's aim to merge this before noon PDT tomorrow.

[ConnorMcMahon]

LGTM!