Add initial common infrastructure for wiring up distributed tracing #6916

pakrym · 2019-07-12T20:58:18Z

Our goal is to provide a way to emit distributed traces from Azure client libraries. The distributed trace usually contains a set of spans. Span represents a unit of work in a distributed system, it has a start time, duration, name and a set of attached attributes.

We are using Activity (https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.activity?view=netcore-2.2) class to represent spans in .net. Activity.Current is async-local and flows with the async context so child Activities can be attached to parents.

In addition to work in this PR there would be an adapter that would turn Activities we create into OpenTelemetry (https://opentelemetry.io/) spans.

Add enough types to get basic 2 level diagnostic trace going.

2 levels are:

Operation, like ConfigurationClient.Get
HttpRequest

I also updated the ConfigurationClient to create tracing scopes in client methods.

Basic pattern is:

using ClientDiagnosticScope scope = _pipeline.Diagnostics.CreateScope("ConfigurationClient.Get");
// Add you string attributes
scope.AddAttribute("key", key);
// Add attributes that need to be computed lazily to avoid allocations when 
// tracing is disabled
scope.AddAttribute("value", integerValue, i => i.ToString());

scope.Start();

try
{
    // Create and make a request
}
catch (Exception e)
{
    scope.Failed(e);
}

Names of diagnostic sources and events will change in the future.

Sample trace that is produced using a zipkin exporter:

…t-HttpRequest

sdk/appconfiguration/Azure.ApplicationModel.Configuration/src/ConfigurationClient.cs

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

sdk/core/Azure.Core/src/Pipeline/Policies/RequestActivityPolicy.cs

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

lmolkova · 2019-07-15T19:12:11Z

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

+
+        public void AddAttribute<T>(string name, T value)
+        {
+            if (_activity != null)


I'd avoid doing this. Activity tags are always strings. ToString() can be called by caller and it's better to make it clear from API.

This allows us to avoid formatting to string when there is no diagnostic source attached.

there is a special Activity.Recorded flag now - it should be false if there is no listener and if you badly need tags - you should add them only if activity is recorded (i.e. sampled in). OpenTelemetry should take care of setting it to true when needed.

Would there be a point in creating activity at all if it's not being recorded?

yes, for propagation: you don't record it, but service before you and after you did - they can correlate telemetry (but causation is broken)

lmolkova · 2019-07-15T19:13:50Z

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

+        {
+            if (_activity != null && _source.IsEnabled(_name))
+            {
+                _source.StartActivity(_activity, null);


For Http requests we should pass payload. It is more efficient to pass it (and parse it) then create multiple attributes.

This is not for http requests, this activity is for client level operations.

lmolkova · 2019-07-15T19:14:01Z

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

+
+            if (_source != null)
+            {
+                _source.StopActivity(_activity, null);


same here, we should have payload

We don't always have payload at this level and we won't be able to have an adapter library per client SDK anyway.

lmolkova · 2019-07-15T19:15:14Z

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs

+                return;
+            }
+
+            _source?.Write(_activity.OperationName + ".Exception", e);


Most likely it will be ignored by tracing system as it is likely handled and just noise. Unhandled exceptions are taken care of by web framework, So I think we don't need it for now.

Don't we care about setting a status flag on spans?

we do, can we do it better than Exception instance? If not, that's ok, but we need a plan

We can, but using an exception should be an option too.

sdk/core/Azure.Core/src/Pipeline/HttpPipelineDiagnostics.cs

sdk/core/Azure.Core/src/Pipeline/Policies/RequestActivityPolicy.cs

lmolkova · 2019-07-15T19:22:19Z

sdk/core/Azure.Core/src/Azure.Core.csproj

@@ -20,6 +20,7 @@
  <ItemGroup>
    <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" />
    <PackageReference Include="System.Buffers" />
+    <PackageReference Include="System.Diagnostics.DiagnosticSource" />


BTW, we need to use DiagSource 4.6.0 preview, it uses W3C trace-context and we should never use old Id format in Azure SDKs

Isn't the format supposed to be controlled by the app?

Ah, I see, we need to know about TraceStateString too. https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L198

sdk/core/Azure.Core/src/Pipeline/Policies/RequestActivityPolicy.cs

lmolkova · 2019-07-15T19:23:32Z

sdk/appconfiguration/Azure.ApplicationModel.Configuration/src/ConfigurationClient.cs

@@ -113,8 +123,13 @@ public virtual async Task<Response<ConfigurationSetting>> AddAsync(Configuration
        /// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
        public virtual Response<ConfigurationSetting> Add(ConfigurationSetting setting, CancellationToken cancellationToken = default)
        {
-            using (Request request = CreateAddRequest(setting))
+            using DiagnosticScope scope = _pipeline.Diagnostics.CreateScope("ConfigurationClient.Add");
+            scope.AddAttribute("key", setting?.Key);


what is key?

Basically a configuration option name.

lmolkova · 2019-07-15T19:27:43Z

sdk/appconfiguration/Azure.ApplicationModel.Configuration/src/ConfigurationClient.cs

@@ -126,6 +141,11 @@ public virtual Response<ConfigurationSetting> Add(ConfigurationSetting setting,
                        throw response.CreateRequestFailedException();
                }
            }
+            catch (Exception e)
+            {
+                scope.Failed(e);


we need to think more on how to track status of Activity. Exceptions are not perfect (they are likely handled). Will we parse it (with type +something like WebException.Status ) in opentelemetry?

Why do we need to parse it?

I mean from the exception type we can understand if it is let's say timeout or HttpRequestException.
In HttpRequestException, it is useful to know the failure details (like these https://docs.microsoft.com/en-us/dotnet/api/system.net.webexceptionstatus?view=netframework-4.8)

One thing that bothers me with exception is this:

request starts, tracing system doesn't care about Start

exception happens - tracing system now have to store it and map it to what? It didn't event create Span yet. So it'll set something on the Activity tags or will be forced to create Span on start and access current span to attach exception to.

request ends - ok, now tracing system tracks Span and can set status

Other approaches could be:

set status on the Activity.Tags - but it has to be a string

set status in the Stop payload - we need to agree on the contract (prop name, value type)

Let's discuss it tomorrow. Activity kinda has to have status and if we agree that is the long term plan - then I like the Exception approach now.

pakrym · 2019-07-15T21:17:30Z

eng/Packages.Data.props

    <PackageReference Update="System.Threading.Tasks.Extensions" Version="4.5.2" />
    <PackageReference Update="System.ValueTuple" Version="4.5.0" />
    <PackageReference Update="WindowsAzure.ServiceBus" Version="5.1.0" />
    <PackageReference Update="WindowsAzure.Storage" Version="9.3.3" />
    <PackageReference Update="xunit.runner.visualstudio" Version="2.4.1" />
    <PackageReference Update="xunit" Version="2.4.1" />
  </ItemGroup>
+  <!-- Track 2 specific versions -->


I've added a conditional section for Track 2 libraries because I'd like to update some package versions that might be used in track 1 clients too.

I assume that DiagnosticsSource and Unsafe are the only 2 packages that are shared? I'm OK with keeping these separate at least for now while we are consuming preview versions of those packages.

Yes, those were the ones I was worried about.

lmolkova · 2019-07-16T02:03:47Z

sdk/core/Azure.Core/src/Pipeline/Policies/RequestActivityPolicy.cs

+                        }
+                    }
+                }
+                else


I think supporting Request-Id doesn't make sense. Azure Services will never implement Request-Id header so propagating it won't help anything.

So what should we do in case of non-W3C activity?

Two options

Insist on W3C activity (in preview7 there will be an instance method Activity.SetIdFormat()). Then if a user had activity in the old format, the one created by Azure SDK will not correlate to the old one. But anything that happens in and after Azure SDK (e.g. EventHub and message processing or Storage logs and EventGrid notifications followed by Function) will be correlated.

Tolerate old format but not inject headers. Then everything before and including Azure SDK will correlate, anything after will become a new trace.

Looking into this, there is no good solution for format mismatch. But I think this is not Azure SDK responsibility to solve this issue - tracing system should control the format and enforce W3C on the incoming boundary.

In the bright future, there will be nothing else than W3C and we won't have this problem.

So I suggest do 2.

pakrym · 2019-07-16T17:20:34Z

I'm going to merge this as it's mostly fine so I can have fewer changes in flight and start working on storage codegen.

I'll address further comments as part of a new PR.

pakrym added 7 commits July 8, 2019 15:16

Rename Request to HttpRequest

713748e

Generator

3c7c98a

Back to Request

189fb90

Merge remote-tracking branch 'azure/master' into pakrym/rename-Reques…

04d75b9

…t-HttpRequest

Fix merge

ea3015c

Add initial common infrastructure for wiring up distributed tracing

254cfd3

Merge remote-tracking branch 'azure/master' into pakrym/dt-basics

8d9cde0

pakrym requested review from jsquire, tg-msft, maririos and schaabs July 12, 2019 20:58

pakrym added 2 commits July 12, 2019 14:13

Nullref

5b1f9cf

Don't run-em twice

8e289c9

tg-msft approved these changes Jul 12, 2019

View reviewed changes

pakrym added 2 commits July 12, 2019 15:33

PR comments

5ae73d7

Nice overload

8bbb2cf

tg-msft reviewed Jul 12, 2019

View reviewed changes

sdk/core/Azure.Core/src/Pipeline/AzureOperationScope.cs Outdated Show resolved Hide resolved

pakrym added 3 commits July 12, 2019 15:54

Ted is right

0bc381e

New Http fields

09a62cb

Remove extra yield

ecd1c1a

pakrym requested a review from lmolkova July 15, 2019 19:01

Real fix

c48c7d4

lmolkova reviewed Jul 15, 2019

View reviewed changes

Some PR Comments

c63ec21

pakrym requested review from chidozieononiwu, danieljurek, mitchdenny and weshaggard as code owners July 15, 2019 21:16

pakrym commented Jul 15, 2019

View reviewed changes

Pass the message in

5ed43bb

lmolkova reviewed Jul 16, 2019

View reviewed changes

pakrym merged commit 369ea8e into Azure:master Jul 16, 2019

pakrym deleted the pakrym/dt-basics branch July 16, 2019 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial common infrastructure for wiring up distributed tracing #6916

Add initial common infrastructure for wiring up distributed tracing #6916

pakrym commented Jul 12, 2019 •

edited

Loading

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019 •

edited

Loading

pakrym Jul 15, 2019

lmolkova Jul 16, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 16, 2019 •

edited

Loading

pakrym Jul 15, 2019

weshaggard Jul 15, 2019

pakrym Jul 15, 2019

lmolkova Jul 16, 2019

pakrym Jul 16, 2019

lmolkova Jul 16, 2019

pakrym commented Jul 16, 2019

Add initial common infrastructure for wiring up distributed tracing #6916

Add initial common infrastructure for wiring up distributed tracing #6916

Conversation

pakrym commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Jul 15, 2019 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Jul 16, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jul 16, 2019

pakrym commented Jul 12, 2019 •

edited

Loading

lmolkova Jul 15, 2019 •

edited

Loading

lmolkova Jul 16, 2019 •

edited

Loading