Add initial common infrastructure for wiring up distributed tracing #6916
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" /> | ||
| <PackageReference Include="System.Buffers" /> | ||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" /> | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. Isn't the format supposed to be controlled by the app? There was a problem hiding this comment. 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 |
||
| <PackageReference Include="System.Memory" /> | ||
| <PackageReference Include="System.Numerics.Vectors" /> | ||
| <PackageReference Include="System.Runtime.CompilerServices.Unsafe" /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
|
|
||
| namespace Azure.Core.Pipeline | ||
| { | ||
| public struct DiagnosticScope: IDisposable | ||
| { | ||
| private Activity _activity; | ||
|
|
||
| private readonly string _name; | ||
|
|
||
| private readonly DiagnosticListener _source; | ||
|
|
||
| internal DiagnosticScope(string name, DiagnosticListener source) | ||
| { | ||
| _name = name; | ||
| _source = source; | ||
| _activity = _source.IsEnabled() ? new Activity(_name) : null; | ||
| } | ||
|
|
||
| public bool IsEnabled => _activity != null; | ||
|
|
||
| public void AddAttribute(string name, string value) | ||
| { | ||
| _activity?.AddTag(name, value); | ||
| } | ||
|
|
||
| public void AddAttribute<T>(string name, T value) | ||
| { | ||
| if (_activity != null) | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. This allows us to avoid formatting to string when there is no diagnostic source attached. There was a problem hiding this comment. 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. There was a problem hiding this comment. Would there be a point in creating activity at all if it's not being recorded? There was a problem hiding this comment. yes, for propagation: you don't record it, but service before you and after you did - they can correlate telemetry (but causation is broken) |
||
| { | ||
| AddAttribute(name, value.ToString()); | ||
| } | ||
| } | ||
|
|
||
| public void AddAttribute<T>(string name, T value, Func<T, string> format) | ||
pakrym marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| if (_activity != null) | ||
| { | ||
| AddAttribute(name, format(value)); | ||
| } | ||
| } | ||
|
|
||
| public void Start() | ||
| { | ||
| if (_activity != null && _source.IsEnabled(_name)) | ||
| { | ||
| _source.StartActivity(_activity, null); | ||
|
There was a problem hiding this comment. For Http requests we should pass payload. It is more efficient to pass it (and parse it) then create multiple attributes. There was a problem hiding this comment. This is not for http requests, this activity is for client level operations. |
||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| if (_activity == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (_source != null) | ||
| { | ||
| _source.StopActivity(_activity, null); | ||
|
There was a problem hiding this comment. same here, we should have payload There was a problem hiding this comment. We don't always have payload at this level and we won't be able to have an adapter library per client SDK anyway. |
||
| } | ||
| else | ||
| { | ||
| _activity?.Stop(); | ||
| } | ||
| } | ||
|
|
||
| public void Failed(Exception e) | ||
| { | ||
| if (_activity == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _source?.Write(_activity.OperationName + ".Exception", e); | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Don't we care about setting a status flag on spans? There was a problem hiding this comment. we do, can we do it better than Exception instance? If not, that's ok, but we need a plan There was a problem hiding this comment. We can, but using an exception should be an option too. |
||
| _activity?.Stop(); | ||
|
|
||
| _activity = null; | ||
|
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Diagnostics; | ||
|
|
||
| namespace Azure.Core.Pipeline | ||
| { | ||
| public sealed class ClientDiagnostics | ||
| { | ||
| private readonly bool _isActivityEnabled; | ||
|
|
||
| public ClientDiagnostics(bool isActivityEnabled) | ||
| { | ||
| _isActivityEnabled = isActivityEnabled; | ||
| } | ||
|
|
||
| private static readonly DiagnosticListener s_source = new DiagnosticListener("Azure.Clients"); | ||
|
|
||
| public DiagnosticScope CreateScope(string name) | ||
| { | ||
| if (!_isActivityEnabled) | ||
| { | ||
| return default; | ||
| } | ||
| return new DiagnosticScope(name, s_source); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Globalization; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Azure.Core.Pipeline.Policies | ||
| { | ||
| internal class RequestActivityPolicy : HttpPipelinePolicy | ||
| { | ||
| private const string TraceParentHeaderName = "traceparent"; | ||
| private const string TraceStateHeaderName = "tracestate"; | ||
| private const string RequestIdHeaderName = "Request-Id"; | ||
|
|
||
| public static RequestActivityPolicy Shared { get; } = new RequestActivityPolicy(); | ||
|
|
||
| private static readonly DiagnosticListener s_diagnosticSource = new DiagnosticListener("Azure.Pipeline"); | ||
|
|
||
| public override Task ProcessAsync(HttpPipelineMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline) | ||
| { | ||
| return ProcessAsync(message, pipeline, true); | ||
| } | ||
|
|
||
| public override void Process(HttpPipelineMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline) | ||
| { | ||
| ProcessAsync(message, pipeline, false).EnsureCompleted(); | ||
| } | ||
|
|
||
| private static async Task ProcessAsync(HttpPipelineMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline, bool isAsync) | ||
| { | ||
| if (!s_diagnosticSource.IsEnabled()) | ||
| { | ||
| await ProcessNextAsync(message, pipeline, isAsync).ConfigureAwait(false); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| var activity = new Activity("Azure.Core.Http.Request"); | ||
| activity.AddTag("http.method", message.Request.Method.Method); | ||
|
There was a problem hiding this comment. I suggest passing HttpRequest and response objects - and avoid using tags for http pipeline. If you have a strong need to use them to avoid more work in bridge, check if activity is sampled in first, before adding them. We should talk more about how to support it better with Activity.Recorded which comes with next DiagnosticSource 4.6.0-preview), it is propagated from parent and it could be changed in Start events by the listener. There was a problem hiding this comment. There is some friction with updating to DiagnosticSource 4.6.0-preview because of the central version management in the repo. I'll file an issue to follow up. There was a problem hiding this comment.
Isn't checking for There was a problem hiding this comment. if IsEnabled is false, then Activity is not created at all - nothing gets tracked or propagated. Activity.Recorded = false means activity is created, and propagated, but augmenting it with anything does not make sense. You see why we need OpenTelemetry in general - there are some many caveats around Activity... There was a problem hiding this comment.
What is the point of this activity then? It carries no information except ID and operation name. Especially considering AzureSDK does not need to create and incoming request activity like ASP.NET COre does |
||
| activity.AddTag("http.url", message.Request.UriBuilder.ToString()); | ||
| if (message.Request.Headers.TryGetValue("User-Agent", out string userAgent)) | ||
| { | ||
| activity.AddTag("http.user_agent", userAgent); | ||
| } | ||
|
|
||
| var diagnosticSourceActivityEnabled = s_diagnosticSource.IsEnabled(activity.OperationName, message); | ||
|
|
||
| if (diagnosticSourceActivityEnabled) | ||
| { | ||
| s_diagnosticSource.StartActivity(activity, message); | ||
| } | ||
| else | ||
| { | ||
| activity.Start(); | ||
| } | ||
|
|
||
pakrym marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (isAsync) | ||
| { | ||
| await ProcessNextAsync(message, pipeline).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| ProcessNext(message, pipeline); | ||
| } | ||
|
|
||
| activity.AddTag("http.status_code", message.Response.Status.ToString(CultureInfo.InvariantCulture)); | ||
|
|
||
| if (diagnosticSourceActivityEnabled) | ||
| { | ||
| s_diagnosticSource.StopActivity(activity, message); | ||
| } | ||
| else | ||
| { | ||
| activity.Stop(); | ||
| } | ||
| } | ||
|
|
||
| private static async Task ProcessNextAsync(HttpPipelineMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline, bool isAsync) | ||
| { | ||
| var currentActivity = Activity.Current; | ||
|
|
||
| if (currentActivity != null) | ||
| { | ||
| if (currentActivity.IdFormat == ActivityIdFormat.W3C) | ||
| { | ||
| if (!message.Request.Headers.Contains(TraceParentHeaderName)) | ||
| { | ||
| message.Request.Headers.Add(TraceParentHeaderName, currentActivity.Id); | ||
| if (currentActivity.TraceStateString != null) | ||
| { | ||
| message.Request.Headers.Add(TraceStateHeaderName, currentActivity.TraceStateString); | ||
| } | ||
| } | ||
| } | ||
| else | ||
|
There was a problem hiding this comment. I think supporting Request-Id doesn't make sense. Azure Services will never implement Request-Id header so propagating it won't help anything. There was a problem hiding this comment. So what should we do in case of non-W3C activity? There was a problem hiding this comment. Two options
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. |
||
| { | ||
| if (!message.Request.Headers.Contains(RequestIdHeaderName)) | ||
| { | ||
| message.Request.Headers.Add(RequestIdHeaderName, currentActivity.Id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (isAsync) | ||
| { | ||
| await ProcessNextAsync(message, pipeline).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| ProcessNext(message, pipeline); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, those were the ones I was worried about.