Distributed Tracing #1270

TsuyoshiUshio · 2020-03-11T18:37:07Z

The Distributed Tracing Implementation

I create this PR for share/review/discuss the architecture.
This branch includes references for non-published NuGet package (DurableTask correlation branch:https://github.com/Azure/durabletask/tree/correlation), so DO NOT MERGE
Also, I need to resolve conflict against the correlation branch.

Overview

Support HttpCorrelationProtocol and W3C TraceContext
Use the Distributed Tracing enabled DurableTask (https://github.com/Azure/durabletask/tree/correlation/samples/Correlation.Samples)
Update Testing Framework to support E2E tracing testing
Add Configuration for enabling/disabling distributed Tracing
Add Configuration for specifying Distributed Tracing Protocol

Supported

End to End Tracing of Orchestration, SubOrchestration, Activity, and Exception, Retry, Fan-Out-Fan-In, ContinueAsNew

Not Supported yet

Durable Entity Tracing
Http Activity
JavaScript

TODO

Telemetry Suppression (Currently, Durable Functions and Azure Functions Host send duplicate telemetry with different Id.)

Related Issue:

#939

amdeel · 2020-03-13T16:19:57Z

/AzurePipelines run

azure-pipelines · 2020-03-13T16:20:02Z

Pull request contains merge conflicts.

TsuyoshiUshio · 2020-03-14T17:52:03Z

@amdeel Thank you for the trigger! I'm busy state until Monday, I'll fix the conflict after that, then ping you.

…cally

TsuyoshiUshio · 2020-03-20T16:45:20Z

The test fails, we need to push DurableTaskCore.Telemetry package first.

TsuyoshiUshio · 2020-04-08T04:48:46Z

Ready for review and merge. However, the CI fails because:

DurableTask.Core.Telemetry and DurableTask.AzureStrorage.Telemetry is reside on the myget.
TestV1 is not supported. (Distributed Tracing is V2 only feature)

Is there any good ways to run the appveyor CI under this situation?

samples/correlation-csharp/FunctionAppCorrelation/local.settings.json

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs

reyang · 2020-04-20T17:51:42Z

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs

+        {
+            if (this.IsSuppressedTelemetry(telemetry))
+            {
+                this.SuppressTelemetry(telemetry);


This would suppress the telemetry within the durable task.
Additional work is required to communicate back to the host and suppress the telemetry there:

Durable task puts the suppression flag in the payload before returning back to the Functions host.

Once Functions host received the suppression flag, respect the flag and stop emitting telemetry events (context propagation is still required).

Hi @reyang
Thank you for the notice! Very good point.

This issue might be related.
#593

Maybe this TelemetryInitializer might be the one fix on the functions host side.
https://github.com/Azure/azure-webjobs-sdk/blob/dev/src/Microsoft.Azure.WebJobs.Logging.ApplicationInsights/Initializers/WebJobsTelemetryInitializer.cs

samples/correlation-csharp/FunctionAppCorrelation/local.settings.json

reyang · 2020-04-20T17:54:52Z

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs

+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT License. See LICENSE in the project root for license information.
+
+using DurableTask.Core.Settings;


Not related to this PR though, the namespace seems to be against the naming convention.

Do you mean <Company>.<Technology>* rule?

Sure. Then it might be a problem of the DurableTask framework side. Thank you for pointing out.

reyang

LGTM overall, I've left my suggestions on how to make the experience work by suppressing telemetry events on both sides (durable function + functions host). I've also left some minor comments that are not specific about this PR.

ConnorMcMahon

There is a lot of conditional compilation I want to see cleaned up before we merge this I think. I think only compiling your new files in non-Functions V1 will substantially simplify a lot of our conditional compilation.

As mentioned in one of the comments, we should discuss how we want to handle Functions V1 for this release while it is it's own package. I think we should actively discourage Functions V1 customers from using this alternative package, since they won't get any benefit from this.

That being said, it may be lest to leave the net461 bits in there. I think we use net461 vs netstandard2.0 to diferentiate features that are only available in Functions V2+, and if we get rid of net461 bits, then Functions V1 customers who avoid our recommendation and use the custom package will use the netstandard2.0 bits, which will likely break their app.

samples/correlation-csharp/FunctionAppCorrelation/local.settings.json

src/WebJobs.Extensions.DurableTask/DurableTaskJobHostConfigurationExtensions.cs

src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs

src/WebJobs.Extensions.DurableTask/Correlation/TraceContextBaseExtensions.cs

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs

src/WebJobs.Extensions.DurableTask/Options/TraceOptions.cs

ConnorMcMahon · 2020-04-24T17:22:36Z

src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj

  </ItemGroup>

  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
    <PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.14" />
-    <PackageReference Include="Microsoft.AspNetCore.Mvc.WebApiCompatShim" Version="2.2.0" />
+    <PackageReference Include="Microsoft.AspNetCore.Mvc.WebApiCompatShim" Version="2.1.0" />


I vaguely remember some weirdness here, where we had to use a specific version to avoid breaking a build. I'll see if I can dig up the issue.

src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj

… with some nit fixes

TsuyoshiUshio · 2020-05-03T19:58:43Z

/AzurePipelines run

azure-pipelines · 2020-05-03T19:58:48Z

Commenter does not have sufficient privileges for PR 1270 in repo Azure/azure-functions-durable-extension

TsuyoshiUshio · 2020-05-03T21:46:10Z

Hi @ConnorMcMahon

I fix all of the issue that you point out! Could you run the AzDO pipeline?
I seem don't have a permission.
Once it passed. Let's get merge this.

…to DistributedTracingEnabled

TsuyoshiUshio · 2020-05-04T07:17:28Z

According to the request of renaming from DistributedTracingDisabled to DistributedTracingEnabled, I change the PR. #1298

ConnorMcMahon

It's getting very close. Mainly a few nits remaining!

I think we can merge this once you address this last round of feedback.

ConnorMcMahon · 2020-05-07T00:28:38Z

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs

+    /// 2. Avoid to be overriden when it is RequestTelemetry
+    /// Original Source is here https://github.com/microsoft/ApplicationInsights-dotnet-server/blob/2.8.0/Src/Common/W3C/W3COperationCorrelationTelemetryInitializer.cs.
+    /// </summary>
+    [Obsolete("Not ready for public consumption.")]


Is there a reason we are marking this as obsolete? In general, since this is internal, I don't see much benefit to this.

I think you may want to mark the interface ITelemetryInitializer with this attribute, as that is public. I wish it didn't have to be, but I haven't found a good way to have non-public interfaces be injected via dependency injection yet.

I remove the Obsolute attribute. The reason it was there is, I took this code from https://github.com/microsoft/ApplicationInsights-dotnet-server/blob/2.8.0/Src/Common/W3C/W3COperationCorrelationTelemetryInitializer.cs as I leave the comment on it. After that I modify it. So the first time, I just keep the same structure, however, since I modify it, it means nothing. I removed it and remove warning suppression at the TelemetryActivator.

src/WebJobs.Extensions.DurableTask/Correlation/StringExtensions.cs

src/WebJobs.Extensions.DurableTask/Correlation/TraceContextBaseExtensions.cs

src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs

src/WebJobs.Extensions.DurableTask/Options/TraceOptions.cs

TsuyoshiUshio · 2020-05-08T20:21:51Z

@ConnorMcMahon I've done the all fix. I appreciate your review! I learned a lot!

ConnorMcMahon

LGTM!

Demo implementation of Distributed Tracing with change for running lo…

65bffc0

…cally

TsuyoshiUshio force-pushed the feature/tracing branch from 6d25781 to 65bffc0 Compare March 20, 2020 16:41

TsuyoshiUshio added 3 commits March 30, 2020 20:05

Bump the version for alpha

ee08628

fix: duplicated automatic dependency tracking

ddf4495

remove warning for the release

1db794d

cgillum mentioned this pull request Apr 3, 2020

End-to-end distributed tracing support #939

Closed

TsuyoshiUshio changed the title ~~[Do Not Merge] Distributed Tracing~~ Distributed Tracing Apr 8, 2020

TsuyoshiUshio added 2 commits April 8, 2020 15:04

change version of DurableTask.AzureStorage for v1 test

8860955

fix V1 test can't compile

45aa7b1

cgillum requested a review from ConnorMcMahon April 8, 2020 22:53

cgillum added this to the v2.3.0 milestone Apr 8, 2020

change #if for consistency

6a11b5e

reyang reviewed Apr 16, 2020

View reviewed changes

samples/correlation-csharp/FunctionAppCorrelation/local.settings.json Show resolved Hide resolved

reyang reviewed Apr 20, 2020

View reviewed changes

src/WebJobs.Extensions.DurableTask/Correlation/DurableTaskCorrelationTelemetryInitializer.cs Outdated Show resolved Hide resolved

reyang reviewed Apr 20, 2020

View reviewed changes

samples/correlation-csharp/FunctionAppCorrelation/local.settings.json Show resolved Hide resolved

reyang reviewed Apr 20, 2020

View reviewed changes

reyang approved these changes Apr 20, 2020

View reviewed changes

add blank line for the before EOF

70f13c9

ConnorMcMahon reviewed Apr 24, 2020

View reviewed changes

olitomlinson mentioned this pull request Apr 24, 2020

Durable Orchestration and Activities correlation in Application Insights. #1334

Closed

TsuyoshiUshio added 4 commits May 2, 2020 11:25

Remove unnecessary string

6b78e79

Remove condition for V1 Functions

426dc85

change to ignore Correlation for FunctionV1 with upgrading V1 version…

05bb6d7

… with some nit fixes

upgrade Functions V1 test library to fit main source

100ee00

change the name of the configuration from DistributedTracingDisabled …

f2cca30

…to DistributedTracingEnabled

ConnorMcMahon approved these changes May 7, 2020

View reviewed changes

TsuyoshiUshio added 6 commits May 8, 2020 11:24

remove unnecessary Attributes

94e067c

Add explanation for the exception message

9288ff7

Remove StringExensions

3139635

forget to add to remove StringExtensions

d06d17e

remove unnecessary TODO

4fbed6f

Remove the warning and white space for stylecop

60b3298

ConnorMcMahon approved these changes May 13, 2020

View reviewed changes

ConnorMcMahon merged commit 49aac11 into Azure:correlation May 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distributed Tracing #1270

Distributed Tracing #1270

TsuyoshiUshio commented Mar 11, 2020 •

edited

Loading

amdeel commented Mar 13, 2020

azure-pipelines bot commented Mar 13, 2020

TsuyoshiUshio commented Mar 14, 2020

TsuyoshiUshio commented Mar 20, 2020

TsuyoshiUshio commented Apr 8, 2020

reyang Apr 20, 2020

TsuyoshiUshio Apr 24, 2020

reyang Apr 20, 2020

TsuyoshiUshio Apr 24, 2020

TsuyoshiUshio Apr 24, 2020 •

edited

Loading

reyang left a comment

ConnorMcMahon left a comment

ConnorMcMahon Apr 24, 2020

TsuyoshiUshio commented May 3, 2020

azure-pipelines bot commented May 3, 2020

TsuyoshiUshio commented May 3, 2020

TsuyoshiUshio commented May 4, 2020

ConnorMcMahon left a comment

ConnorMcMahon May 7, 2020

ConnorMcMahon May 7, 2020

TsuyoshiUshio May 8, 2020

TsuyoshiUshio commented May 8, 2020 •

edited

Loading

ConnorMcMahon left a comment

Distributed Tracing #1270

Distributed Tracing #1270

Conversation

TsuyoshiUshio commented Mar 11, 2020 • edited Loading

The Distributed Tracing Implementation

Overview

Supported

Not Supported yet

TODO

Related Issue:

amdeel commented Mar 13, 2020

azure-pipelines bot commented Mar 13, 2020

TsuyoshiUshio commented Mar 14, 2020

TsuyoshiUshio commented Mar 20, 2020

TsuyoshiUshio commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TsuyoshiUshio Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

ConnorMcMahon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TsuyoshiUshio commented May 3, 2020

azure-pipelines bot commented May 3, 2020

TsuyoshiUshio commented May 3, 2020

TsuyoshiUshio commented May 4, 2020

ConnorMcMahon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TsuyoshiUshio commented May 8, 2020 • edited Loading

ConnorMcMahon left a comment

Choose a reason for hiding this comment

TsuyoshiUshio commented Mar 11, 2020 •

edited

Loading

TsuyoshiUshio Apr 24, 2020 •

edited

Loading

TsuyoshiUshio commented May 8, 2020 •

edited

Loading