-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial common infrastructure for wiring up distributed tracing #6916
Changes from 14 commits
713748e
3c7c98a
189fb90
04d75b9
ea3015c
254cfd3
8d9cde0
5b1f9cf
8e289c9
5ae73d7
8bbb2cf
0bc381e
09a62cb
ecd1c1a
c48c7d4
c63ec21
5ed43bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows us to avoid formatting to string when there is no diagnostic source attached. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would there be a point in creating activity at all if it's not being recorded? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, we should have payload There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we care about setting a status flag on spans? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"); | ||
pakrym marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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,83 @@ | ||
// 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 | ||
{ | ||
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()) | ||
{ | ||
if (isAsync) | ||
{ | ||
await ProcessNextAsync(message, pipeline).ConfigureAwait(false); | ||
} | ||
else | ||
{ | ||
ProcessNext(message, pipeline); | ||
} | ||
|
||
return; | ||
} | ||
|
||
var activity = new Activity("Azure.Core.Http.Request"); | ||
activity.AddTag("http.method", message.Request.Method.Method); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't checking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
|
||
if (diagnosticSourceActivityEnabled) | ||
{ | ||
s_diagnosticSource.StartActivity(activity, null); | ||
pakrym marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
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, null); | ||
} | ||
else | ||
{ | ||
activity.Stop(); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using Azure.Core.Pipeline; | ||
using NUnit.Framework; | ||
|
||
namespace Azure.Core.Tests | ||
{ | ||
public class ClientDiagnosticsTests | ||
{ | ||
[Test] | ||
public void CreatesActivityWithNameAndTags() | ||
{ | ||
|
||
using var testListener = new TestDiagnosticListener("Azure"); | ||
ClientDiagnostics clientDiagnostics = new ClientDiagnostics(true); | ||
|
||
DiagnosticScope scope = clientDiagnostics.CreateScope("ActivityName"); | ||
|
||
scope.AddAttribute("Attribute1", "Value1"); | ||
scope.AddAttribute("Attribute2", 2, i => i.ToString()); | ||
scope.AddAttribute("Attribute3", 3); | ||
|
||
scope.Start(); | ||
|
||
KeyValuePair<string, object> startEvent = testListener.Events.Dequeue(); | ||
|
||
Activity activity = Activity.Current; | ||
|
||
scope.Dispose(); | ||
|
||
KeyValuePair<string, object> stopEvent = testListener.Events.Dequeue(); | ||
|
||
Assert.Null(Activity.Current); | ||
Assert.AreEqual("ActivityName.Start", startEvent.Key); | ||
Assert.AreEqual("ActivityName.Stop", stopEvent.Key); | ||
|
||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("Attribute1", "Value1")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("Attribute2", "2")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("Attribute3", "3")); | ||
} | ||
|
||
[Test] | ||
public void FailedStopsActivityAndWritesExceptionEvent() | ||
{ | ||
using var testListener = new TestDiagnosticListener("Azure"); | ||
ClientDiagnostics clientDiagnostics = new ClientDiagnostics(true); | ||
|
||
DiagnosticScope scope = clientDiagnostics.CreateScope("ActivityName"); | ||
|
||
scope.AddAttribute("Attribute1", "Value1"); | ||
scope.AddAttribute("Attribute2", 2, i => i.ToString()); | ||
|
||
scope.Start(); | ||
|
||
KeyValuePair<string, object> startEvent = testListener.Events.Dequeue(); | ||
|
||
Activity activity = Activity.Current; | ||
|
||
var exception = new Exception(); | ||
scope.Failed(exception); | ||
scope.Dispose(); | ||
|
||
KeyValuePair<string, object> stopEvent = testListener.Events.Dequeue(); | ||
|
||
Assert.Null(Activity.Current); | ||
Assert.AreEqual("ActivityName.Start", startEvent.Key); | ||
Assert.AreEqual("ActivityName.Exception", stopEvent.Key); | ||
Assert.AreEqual(exception, stopEvent.Value); | ||
Assert.AreEqual(0, testListener.Events.Count); | ||
|
||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("Attribute1", "Value1")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("Attribute2", "2")); | ||
} | ||
|
||
[Test] | ||
public void NoopsWhenDisabled() | ||
{ | ||
ClientDiagnostics clientDiagnostics = new ClientDiagnostics(false); | ||
DiagnosticScope scope = clientDiagnostics.CreateScope(""); | ||
|
||
scope.AddAttribute("Attribute1", "Value1"); | ||
scope.AddAttribute("Attribute2", 2, i => i.ToString()); | ||
scope.Failed(new Exception()); | ||
scope.Dispose(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Threading.Tasks; | ||
using Azure.Core.Http; | ||
using Azure.Core.Pipeline; | ||
using Azure.Core.Pipeline.Policies; | ||
using Azure.Core.Testing; | ||
using NUnit.Framework; | ||
|
||
namespace Azure.Core.Tests | ||
{ | ||
public class RequestActivityPolicyTests : SyncAsyncPolicyTestBase | ||
{ | ||
public RequestActivityPolicyTests(bool isAsync) : base(isAsync) | ||
{ | ||
} | ||
|
||
[Test] | ||
[NonParallelizable] | ||
public async Task ActivityIsCreatedForRequest() | ||
{ | ||
Activity activity = null; | ||
KeyValuePair<string, object> startEvent = default; | ||
using var testListener = new TestDiagnosticListener("Azure.Pipeline"); | ||
|
||
MockTransport mockTransport = CreateMockTransport(_ => | ||
{ | ||
activity = Activity.Current; | ||
startEvent = testListener.Events.Dequeue(); | ||
return new MockResponse(201); | ||
}); | ||
|
||
using Request request = mockTransport.CreateRequest(); | ||
request.Method = RequestMethod.Get; | ||
request.UriBuilder.Uri = new Uri("http://example.com"); | ||
request.Headers.Add("User-Agent", "agent"); | ||
|
||
Task<Response> requestTask = SendRequestAsync(mockTransport, request, RequestActivityPolicy.Shared); | ||
|
||
await requestTask; | ||
|
||
KeyValuePair<string, object> stopEvent = testListener.Events.Dequeue(); | ||
|
||
Assert.AreEqual("Azure.Core.Http.Request.Start", startEvent.Key); | ||
Assert.AreEqual("Azure.Core.Http.Request.Stop", stopEvent.Key); | ||
|
||
Assert.AreEqual("Azure.Core.Http.Request", activity.OperationName); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("http.status_code", "201")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("http.url", "http://example.com/")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("http.method", "GET")); | ||
CollectionAssert.Contains(activity.Tags, new KeyValuePair<string, string>("http.user_agent", "agent")); | ||
} | ||
} | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the format supposed to be controlled by the app?
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.
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