From 0033a4262669e3344929a0b00b68889197be50ab Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 21 Feb 2020 10:26:01 -0800 Subject: [PATCH 1/4] Proposed new System.Diagnostics APIs --- ...em.Diagnostics.DiagnosticSourceActivity.cs | 59 +++++- .../src/Resources/Strings.resx | 60 +++--- ...System.Diagnostics.DiagnosticSource.csproj | 5 + .../src/System/Diagnostics/Activity.cs | 196 ++++++++++++++++-- .../src/System/Diagnostics/ActivityContext.cs | 94 +++++++++ .../src/System/Diagnostics/ActivityKind.cs | 47 +++++ .../src/System/Diagnostics/ActivityLink.cs | 44 ++++ .../src/System/Diagnostics/ActivitySource.cs | 122 +++++++++++ .../Diagnostics/ActivitySourceEventArgs.cs | 51 +++++ .../System/Diagnostics/DiagnosticListener.cs | 2 +- .../tests/ActivitySourceTests.cs | 150 ++++++++++++++ .../tests/ActivityTests.cs | 66 ++++++ ....Diagnostics.DiagnosticSource.Tests.csproj | 1 + 13 files changed, 855 insertions(+), 42 deletions(-) create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityKind.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs index 8de9ac09bb0a7e..345a114e19b8d9 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs @@ -7,7 +7,7 @@ namespace System.Diagnostics { - public partial class Activity + public partial class Activity : IDisposable { public Activity(string operationName) { } public System.Diagnostics.ActivityTraceFlags ActivityTraceFlags { get { throw null; } set { } } @@ -34,6 +34,7 @@ public string? Id get { throw null; } } public System.Diagnostics.ActivityIdFormat IdFormat { get { throw null; } } + public System.Diagnostics.ActivityKind Kind { get; set; } public string OperationName { get { throw null; } } public System.Diagnostics.Activity? Parent { get { throw null; } } public string? ParentId { get { throw null; } } @@ -43,9 +44,11 @@ public string? Id public System.Diagnostics.ActivitySpanId SpanId { get { throw null; } } public System.DateTime StartTimeUtc { get { throw null; } } public System.Collections.Generic.IEnumerable> Tags { get { throw null; } } + public System.Collections.Generic.IEnumerable Links { get { throw null; } } public System.Diagnostics.ActivityTraceId TraceId { get { throw null; } } public string? TraceStateString { get { throw null; } set { } } public System.Diagnostics.Activity AddBaggage(string key, string? value) { throw null; } + public System.Diagnostics.Activity AddLink(ActivityLink link) { throw null; } public System.Diagnostics.Activity AddTag(string key, string? value) { throw null; } public string? GetBaggageItem(string key) { throw null; } public System.Diagnostics.Activity SetEndTime(System.DateTime endTimeUtc) { throw null; } @@ -55,6 +58,9 @@ public string? Id public System.Diagnostics.Activity SetStartTime(System.DateTime startTimeUtc) { throw null; } public System.Diagnostics.Activity Start() { throw null; } public void Stop() { } + public void Dispose() { } + public void SetCustomProperty(string propertyName, object? propertyValue) { } + public object? GetCustomProperty(string propertyName) { throw null; } } public enum ActivityIdFormat { @@ -121,4 +127,55 @@ public virtual void OnActivityImport(System.Diagnostics.Activity activity, objec public System.Diagnostics.Activity StartActivity(System.Diagnostics.Activity activity, object? args) { throw null; } public void StopActivity(System.Diagnostics.Activity activity, object? args) { } } + public enum ActivitySourceEventOperation + { + SourceCreated = 0, + ActivityStarted = 1, + ActivityStopped = 2 + } + public enum ActivityKind + { + Internal = 1, + Server = 2, + Client = 3, + Producer = 4, + Consumer = 5, + } + public sealed class ActivitySourceEventArgs : System.EventArgs + { + private ActivitySourceEventArgs() { throw null; } + public System.Diagnostics.ActivitySourceEventOperation Operation { get; } + } + public readonly struct ActivityContext : IEquatable + { + public ActivityContext(System.Diagnostics.ActivityTraceId traceId, System.Diagnostics.ActivitySpanId spanId, System.Diagnostics.ActivityTraceFlags traceOptions, string? traceState = null) { throw null; } + public System.Diagnostics.ActivityTraceId TraceId { get; } + public System.Diagnostics.ActivitySpanId SpanId { get; } + public System.Diagnostics.ActivityTraceFlags TraceFlags { get; } + public string? TraceState { get; } + public static bool operator ==(System.Diagnostics.ActivityContext context1, System.Diagnostics.ActivityContext context2) { throw null; } + public static bool operator !=(System.Diagnostics.ActivityContext context1, System.Diagnostics.ActivityContext context2) { throw null; } + public bool Equals(System.Diagnostics.ActivityContext context) { throw null; } + public override bool Equals(object? obj) { throw null; } + public override int GetHashCode() { throw null; } + } + public readonly struct ActivityLink + { + public ActivityLink(System.Diagnostics.ActivityContext context) { throw null; } + public ActivityLink(System.Diagnostics.ActivityContext context, System.Collections.Generic.IDictionary? attributes) { throw null; } + public System.Diagnostics.ActivityContext Context { get; } + public System.Collections.Generic.IDictionary? Attributes { get; } + } + public sealed class ActivitySource : IDisposable + { + private ActivitySource() { throw null; } + public ActivitySource(string name) { throw null; } + public static event EventHandler? OperationEvent { add { } remove { } } + public event EventHandler? ActivityEvent { add { } remove { } } + public static System.Collections.Generic.IEnumerable ActiveList => throw null; + public string Name { get; } + public Activity? CreateActivity() { throw null; } + public Activity? CreateActivity(System.Diagnostics.ActivityContext context, System.Collections.Generic.IEnumerable? links = null, System.DateTimeOffset startTime = default) { throw null; } + public void Dispose() { } + } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx b/src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx index 724068b2dc9e40..18036744326ffb 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx @@ -1,17 +1,17 @@  - @@ -144,9 +144,15 @@ "Can not change format for an activity that was already started" + + "Can not add link to activity after it has been started" + "Can not set ParentId on activity which has parent" + + "Invalid SpanId or TraceId" + "StartTime is not UTC" diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj index eb0dc38fc393ab..e3ea414b161f67 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -32,6 +32,11 @@ Common\System\HexConverter.cs + + + + + diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index b98571feffb22d..a2ce37afaf5613 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -5,6 +5,7 @@ using System.Buffers.Binary; using System.Buffers.Text; using System.Collections.Generic; +using System.Collections.Concurrent; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; @@ -29,10 +30,11 @@ namespace System.Diagnostics /// but the exception is suppressed, and the operation does something reasonable (typically /// doing nothing). /// - public partial class Activity + public partial class Activity : IDisposable { #pragma warning disable CA1825 // Array.Empty() doesn't exist in all configurations private static readonly IEnumerable> s_emptyBaggageTags = new KeyValuePair[0]; + private static readonly IEnumerable s_emptyLinks = new ActivityLink[0]; #pragma warning restore CA1825 private const byte ActivityTraceFlagsIsSet = 0b_1_0000000; // Internal flag to indicate if flags have been set @@ -70,8 +72,17 @@ public partial class Activity private byte _w3CIdFlags; - private KeyValueListNode? _tags; - private KeyValueListNode? _baggage; + private KeyValueListNode>? _tags; + private KeyValueListNode>? _baggage; + private KeyValueListNode? _links; + + private ActivitySource? _source; + private ConcurrentDictionary? _customProperties; + + /// + /// Kind describes the relationship between the Activity, its parents, and its children in a Trace. + /// + public ActivityKind Kind { get; set; } = ActivityKind.Internal; /// /// An operation name is a COARSEST name that is useful grouping/filtering. @@ -216,12 +227,12 @@ public string? RootId { get { - KeyValueListNode? tags = _tags; + KeyValueListNode>? tags = _tags; return tags != null ? Iterate(tags) : s_emptyBaggageTags; - static IEnumerable> Iterate(KeyValueListNode? tags) + static IEnumerable> Iterate(KeyValueListNode>? tags) { do { @@ -233,6 +244,29 @@ public string? RootId } } + /// + /// Links is the list of all objects which contain the attached to this Activity object. + /// If there is no any object attached to the Activity object, Links will return empty list. + /// + public IEnumerable Links + { + get + { + KeyValueListNode? links = _links; + return links != null ? Iterate(links) : s_emptyLinks; + + static IEnumerable Iterate(KeyValueListNode? links) + { + do + { + yield return links!.keyValue; + links = links.Next; + } + while (links != null); + } + } + } + /// /// Baggage is string-string key-value pairs that represent information that will /// be passed along to children of this activity. Baggage is serialized @@ -260,7 +294,7 @@ public string? RootId Debug.Assert(activity != null); do { - for (KeyValueListNode? baggage = activity._baggage; baggage != null; baggage = baggage.Next) + for (KeyValueListNode>? baggage = activity._baggage; baggage != null; baggage = baggage.Next) { yield return baggage.keyValue; } @@ -310,8 +344,8 @@ public Activity(string operationName) /// 'this' for convenient chaining public Activity AddTag(string key, string? value) { - KeyValueListNode? currentTags = _tags; - KeyValueListNode newTags = new KeyValueListNode() { keyValue = new KeyValuePair(key, value) }; + KeyValueListNode>? currentTags = _tags; + KeyValueListNode> newTags = new KeyValueListNode>(new KeyValuePair(key, value)); do { newTags.Next = currentTags; @@ -321,6 +355,32 @@ public Activity AddTag(string key, string? value) return this; } + /// + /// Add object to the list. + /// + /// object of to add to the attached links list. + /// 'this' for convenient chaining + public Activity AddLink(ActivityLink link) + { + // Cannot set Link after starting the Activity. + if (_id != null || _spanId != null) + { + NotifyError(new InvalidOperationException(SR.SetLinkInvalid)); + } + else + { + KeyValueListNode? currentLinks = _links; + KeyValueListNode newLinks = new KeyValueListNode(link); + do + { + newLinks.Next = currentLinks; + currentLinks = Interlocked.CompareExchange(ref _links, newLinks, currentLinks); + } while (!ReferenceEquals(newLinks.Next, currentLinks)); + } + + return this; + } + /// /// Update the Activity to have baggage with an additional 'key' and value 'value'. /// This shows up in the enumeration as well as the @@ -332,8 +392,8 @@ public Activity AddTag(string key, string? value) /// 'this' for convenient chaining public Activity AddBaggage(string key, string? value) { - KeyValueListNode? currentBaggage = _baggage; - KeyValueListNode newBaggage = new KeyValueListNode() { keyValue = new KeyValuePair(key, value) }; + KeyValueListNode>? currentBaggage = _baggage; + KeyValueListNode> newBaggage = new KeyValueListNode>(new KeyValuePair(key, value)); do { @@ -718,6 +778,110 @@ private static bool IsW3CId(string id) ('0' <= id[1] && id[1] <= '9' || 'a' <= id[1] && id[1] <= 'e'); } + /// + /// Dispose will stop the Activity if it is already started and notify any event listeners. Nothing will happen otherwise. + /// + public void Dispose() + { + if (!IsFinished) + { + Stop(); + _source?.RaiseActivityEvent(this, ActivitySourceEventArgs.s_activityStopped); + } + } + + /// + /// SetCustomProperty allow attaching any custom object to this Activity object. + /// If the property name was previously associated with other object, SetCustomProperty will update to use the new propert value instead. + /// + /// The name to associate the value with. + /// The object to attach and map to the property name. + public void SetCustomProperty(string propertyName, object? propertyValue) + { + // We don't check null name here as the dictionary is performing this check anyway. + + if (_customProperties == null) + { + Interlocked.CompareExchange(ref _customProperties, new ConcurrentDictionary(), null); + } + + _customProperties[propertyName] = propertyValue!; + } + + /// + /// GetCustomProperty retrieve previously attached object mapped to the property name. + /// + /// The name to get the associated object with. + /// The object mapped to the property name. Or null if there is no mapping previously done with this property name. + public object? GetCustomProperty(string propertyName) + { + // We don't check null name here as the dictionary is performing this check anyway. + + if (_customProperties == null) + { + return null; + } + + return _customProperties.TryGetValue(propertyName, out object? o) ? o! : null; + } + + internal static Activity CreateAndStart(ActivitySource source, ActivityContext context, IEnumerable? links, DateTimeOffset startTime) + { + Debug.Assert(source != null); + + Activity activity = new Activity(source.Name); + + activity._source = source; + + if (context != default) + { + activity._traceId = context.TraceId.ToString(); + activity._parentSpanId = context.SpanId.ToString(); + activity.ActivityTraceFlags = context.TraceFlags; + activity._traceState = context.TraceState; + } + else + { + Activity? parent = Current; + if (parent != null) + { + // The parent change should not form a loop. We are actually guaranteed this because + // 1. Un-started activities can't be 'Current' (thus can't be 'parent'), we throw if you try. + // 2. All started activities have a finite parent change (by inductive reasoning). + activity.Parent = parent; + } + } + + activity.IdFormat = + ForceDefaultIdFormat ? DefaultIdFormat : + activity.Parent != null ? activity.Parent.IdFormat : + activity._parentSpanId != null ? ActivityIdFormat.W3C : + activity._parentId == null ? DefaultIdFormat : + IsW3CId(activity._parentId) ? ActivityIdFormat.W3C : + ActivityIdFormat.Hierarchical; + + if (activity.IdFormat == ActivityIdFormat.W3C) + activity.GenerateW3CId(); + else + activity._id = activity.GenerateHierarchicalId(); + + if (links != null) + { + foreach (ActivityLink link in links) + { + activity._links = new KeyValueListNode(link, activity._links); + } + } + + activity.StartTimeUtc = startTime == default ? GetUtcNow() : startTime.DateTime; + + SetCurrent(activity); + + source.RaiseActivityEvent(activity, ActivitySourceEventArgs.s_activityStarted); + + return activity; + } + /// /// Set the ID (lazily, avoiding strings if possible) to a W3C ID (using the /// traceId from the parent if possible @@ -946,10 +1110,16 @@ public ActivityIdFormat IdFormat /// /// Having our own key-value linked list allows us to be more efficient /// - private partial class KeyValueListNode + private partial class KeyValueListNode { - public KeyValuePair keyValue; - public KeyValueListNode? Next; + public KeyValueListNode(T value) => keyValue = value; + public KeyValueListNode(T value, KeyValueListNode? next) + { + keyValue = value; + Next = next; + } + public T keyValue; + public KeyValueListNode? Next; } [Flags] diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs new file mode 100644 index 00000000000000..d6f69e37a50eef --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs @@ -0,0 +1,94 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +namespace System.Diagnostics +{ + /// + /// ActivityContext representation conforms to the w3c TraceContext specification. It contains two identifiers + /// a TraceId and a SpanId - along with a set of common TraceFlags and system-specific TraceState values. + /// + public readonly struct ActivityContext : IEquatable + { + /// + /// Construct a new object of ActivityContext. + /// + /// A trace identifier. + /// A span identifier + /// Contain details about the trace. + /// Carries system-specific configuration data. + public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityTraceFlags traceFlags, string? traceState = null) + { + if (traceId == default || spanId == default) + { + throw new ArgumentException(SR.SpanIdOrTraceIdInvalid, traceId == default ? nameof(traceId) : nameof(spanId)); + } + + TraceId = traceId; + SpanId = spanId; + TraceFlags = traceFlags; + TraceState = traceState; + } + + /// + /// The trace identifier + /// + public ActivityTraceId TraceId { get; } + + /// + /// The span identifier + /// + public ActivitySpanId SpanId { get; } + + /// + /// The flags for the details about the trace. + /// + public ActivityTraceFlags TraceFlags { get; } + + /// + /// system-specific configuration data. + /// + public string? TraceState { get; } + + public static bool operator ==(ActivityContext context1, ActivityContext context2) + { + return context1.SpanId == context2.SpanId && + context1.TraceId == context2.TraceId && + context1.TraceFlags == context2.TraceFlags && + context1.TraceState == context2.TraceState; + } + + public static bool operator !=(ActivityContext context1, ActivityContext context2) => !(context1 == context2); + + public bool Equals(ActivityContext context) + { + return this == context; + } + + public override bool Equals(object? obj) + { + if (obj is ActivityContext context) + return this == context; + return false; + } + + public override int GetHashCode() + { + if (this == default) + return 0; + + // HashCode.Combine would be the best but we need to compile for the full framework which require adding dependency + // on the extensions package. Considering this simple type and hashing is not expected to be used, we are implementing + // the hashing manually. + int hash = 5381; + hash = ((hash << 5) + hash) + TraceId.GetHashCode(); + hash = ((hash << 5) + hash) + SpanId.GetHashCode(); + hash = ((hash << 5) + hash) + (int) TraceFlags; + hash = ((hash << 5) + hash) + (TraceState == null ? 0 : TraceState.GetHashCode()); + + return hash; + } + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityKind.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityKind.cs new file mode 100644 index 00000000000000..ce1486adf9b296 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityKind.cs @@ -0,0 +1,47 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace System.Diagnostics +{ + /// + /// Kind describes the relationship between the Activity, its parents, and its children in a Trace. + /// -------------------------------------------------------------------------------- + /// ActivityKind Synchronous Asynchronous Remote Incoming Remote Outgoing + /// -------------------------------------------------------------------------------- + /// Internal + /// Client yes yes + /// Server yes yes + /// Producer yes maybe + /// Consumer yes maybe + /// -------------------------------------------------------------------------------- + /// + public enum ActivityKind + { + /// + /// Default value. + /// Indicates that the Activity represents an internal operation within an application, as opposed to an operations with remote parents or children. + /// + Internal = 1, + + /// + /// Server activity represents request incoming from external component. + /// + Server = 2, + + /// + /// Client activity represents outgoing request to the external component. + /// + Client = 3, + + /// + /// Producer activity represents output provided to external components. + /// + Producer = 4, + + /// + /// Consumer activity represents output received from an external component. + /// + Consumer = 5, + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs new file mode 100644 index 00000000000000..00f7de4f307a1b --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs @@ -0,0 +1,44 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +namespace System.Diagnostics +{ + /// + /// Activity may be linked to zero or more other that are causally related. + /// Links can point to ActivityContexts inside a single Trace or across different Traces. + /// Links can be used to represent batched operations where a Activity was initiated by multiple initiating Activities, + /// each representing a single incoming item being processed in the batch. + /// + public readonly struct ActivityLink + { + /// + /// Construct a new object which can be linked to an Activity object through method. + /// + /// The trace Activity context + public ActivityLink(ActivityContext context) : this(context, null) {} + + /// + /// Construct a new object which can be linked to an Activity object through method. + /// + /// The trace Activity context + /// The key-value pair list of attributes which associated to the + public ActivityLink(ActivityContext context, IDictionary? attributes) + { + Context = context; + Attributes = attributes; + } + + /// + /// Retrieve the object inside this object. + /// + public ActivityContext Context { get; } + + /// + /// Retrieve the key-value pair list of attributes attached with the . + /// + public IDictionary? Attributes { get; } + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs new file mode 100644 index 00000000000000..5dd2e319847353 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -0,0 +1,122 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +namespace System.Diagnostics +{ + public sealed class ActivitySource : IDisposable + { + private static object s_syncObject = new object(); + private static List s_activeSources = new List(); + private ActivitySource() { throw new InvalidOperationException(); } + + /// + /// Event to subscribe to get the notification when any ActivitySource object get created or released. + /// + public static event EventHandler? OperationEvent; + + /// + /// Event to subscribe to get the notification when any Activity object get created or disposed. + /// + public event EventHandler? ActivityEvent; + + /// + /// Construct an ActivitySource object with the input name + /// + /// The name of the ActivitySource object + public ActivitySource(string name) + { + if (name == null) + { + throw new ArgumentNullException(nameof(name)); + } + + lock (s_syncObject) + { + Name = name; + s_activeSources.Add(this); + } + + EventHandler? handlers = OperationEvent; + if (handlers != null) + { + handlers(this, ActivitySourceEventArgs.s_sourceCreated); + } + } + + /// + /// Returns the list of all created ActivitySource objects. + /// + public static IEnumerable ActiveList + { + get + { + lock (s_syncObject) + { + return s_activeSources.ToArray(); + } + } + } + + /// + /// Returns the ActivitySource name. + /// + public string Name { get; } + + /// + /// Creates a new object if there is any listener to the Activity creation event, returns null otherwise. + /// + /// The created object or null if there is no any event listener. + public Activity? CreateActivity() + { + if (ActivityEvent == null) + { + return null; + } + + return Activity.CreateAndStart(this, default, null, default); + } + + /// + /// Creates a new object if there is any listener to the Activity events, returns null otherwise. + /// + /// The object to initialize the created Activity object with. + /// The optional list to initialize the created Activity object with. + /// The optional start timestamp to set on the created Activity object. + /// The created object or null if there is no any event listener. + public Activity? CreateActivity(ActivityContext context, IEnumerable? links = null, DateTimeOffset startTime = default) + { + if (ActivityEvent == null) + { + return null; + } + + return Activity.CreateAndStart(this, context, links, startTime); + } + + /// + /// Dispose the ActivitySource object and remove the current instance from the global list. + /// + public void Dispose() + { + lock (s_syncObject) + { + if (s_activeSources.Remove(this)) + { + ActivityEvent = null; + } + } + } + + internal void RaiseActivityEvent(Activity activity, ActivitySourceEventArgs eventArgs) + { + EventHandler? handlers = ActivityEvent; + if (handlers != null) + { + handlers(activity, eventArgs); + } + } + } +} diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs new file mode 100644 index 00000000000000..20324fc654e39e --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs @@ -0,0 +1,51 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +namespace System.Diagnostics +{ + /// + /// ActivitySourceEventOperation define a possible values included inside the object when firing + /// different events. + /// + public enum ActivitySourceEventOperation + { + /// + /// SourceCreated is used when a new object is created. + /// + SourceCreated = 0, + + /// + /// ActivityStarted is used when a new object is created. + /// + ActivityStarted = 1, + + /// + /// ActivityStopped is used when dispossing object. + /// + ActivityStopped = 2 + } + + /// + /// ActivitySourceEventArgs represent the event argument used when notifying any listener + /// about and objects creation and disposing. + /// + public sealed class ActivitySourceEventArgs : EventArgs + { + internal static readonly ActivitySourceEventArgs s_sourceCreated = new ActivitySourceEventArgs(ActivitySourceEventOperation.SourceCreated); + internal static readonly ActivitySourceEventArgs s_activityStarted = new ActivitySourceEventArgs(ActivitySourceEventOperation.ActivityStarted); + internal static readonly ActivitySourceEventArgs s_activityStopped = new ActivitySourceEventArgs(ActivitySourceEventOperation.ActivityStopped); + + /// + /// Operation tells about the event operation. + /// + public ActivitySourceEventOperation Operation { get; } + + internal ActivitySourceEventArgs(ActivitySourceEventOperation operation) + { + Operation = operation; + } + + private ActivitySourceEventArgs() { throw new InvalidOperationException(); } + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs index bfaeb3f406fbc2..9710efe818ea4b 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs @@ -279,7 +279,7 @@ private class DiagnosticSubscription : IDisposable // - IsEnabled1Arg invoked for DiagnosticSource.IsEnabled(string) // - IsEnabled3Arg invoked for DiagnosticSource.IsEnabled(string, obj, obj) // Subscriber MUST set both IsEnabled1Arg and IsEnabled3Arg or none of them: - // when Predicate is provided in DiagosticListener.Subscribe, + // when Predicate is provided in DiagnosticListener.Subscribe, // - IsEnabled1Arg is set to predicate // - IsEnabled3Arg falls back to predicate ignoring extra arguments. // similarly, when Func is provided, diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs new file mode 100644 index 00000000000000..8e939cdf0fe4f2 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs @@ -0,0 +1,150 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Linq; +using Xunit; + +namespace System.Diagnostics.Tests +{ + public class ActivitySourceTests + { + [Fact] + public void CreateTest() + { + using (ActivitySource source = new ActivitySource("Source1")) + { + Assert.Equal("Source1", source.Name); + Assert.True(ActivitySource.ActiveList.Count() > 0); + Assert.Same(source, ActivitySource.ActiveList.FirstOrDefault(c => c.Name == source.Name)); + + using (ActivitySource source1 = new ActivitySource("Source1")) + { + Assert.NotSame(source, source1); + } + } + + Assert.Null(ActivitySource.ActiveList.FirstOrDefault(c => c.Name == "Source1")); + } + + [Fact] + public void CreateActivityTest() + { + using (ActivitySource source = new ActivitySource("Source2")) + { + using (Activity activity = source.CreateActivity()) + { + // No listener, activity should be null + Assert.Null(activity); + } + } + } + + [Fact] + public void SourceListenerTests() + { + int count = 0; + + EventHandler eventHandler = (o, a) => + { + Assert.Equal(ActivitySourceEventOperation.SourceCreated, a.Operation); + count++; + }; + + ActivitySource.OperationEvent += eventHandler; + + using (ActivitySource source = new ActivitySource("Source3")) + { + Assert.Equal(1, count); + using (ActivitySource source1 = new ActivitySource("Source4")) + { + Assert.Equal(2, count); + using (ActivitySource source2 = new ActivitySource("Source5")) + { + Assert.Equal(3, count); + } + } + } + + using (ActivitySource source = new ActivitySource("Source6")) + { + Assert.Equal(4, count); + + ActivitySource.OperationEvent -= eventHandler; + + using (ActivitySource source1 = new ActivitySource("Source7")) + { + Assert.Equal(4, count); + } + } + } + + [Fact] + public void ActivityListenerTests() + { + int count = 0; + + EventHandler eventHandler = (o, a) => { + if (a.Operation == ActivitySourceEventOperation.ActivityStarted) + { + count++; + } + else if (a.Operation == ActivitySourceEventOperation.ActivityStopped) + { + count--; + } + else + { + Assert.True(false, "Shouldn't get Operation value different than ActivityStarted or ActivityStopped"); + } + }; + + using (ActivitySource source = new ActivitySource("Source8")) + { + source.ActivityEvent += eventHandler; + + using (Activity activity = source.CreateActivity()) + { + Assert.NotNull(activity); + Assert.Equal(1, count); + using (Activity activity1 = source.CreateActivity()) + { + Assert.NotNull(activity1); + Assert.Equal(2, count); + } + Assert.Equal(1, count); + } + Assert.Equal(0, count); + + source.ActivityEvent -= eventHandler; + + using (Activity activity = source.CreateActivity()) + { + Assert.Null(activity); + Assert.Equal(0, count); + } + } + } + + [Fact] + public void CreateActivityFromContextTests() + { + EventHandler eventHandler = (o, a) => { }; + + using (ActivitySource source = new ActivitySource("Source9")) + { + source.ActivityEvent += eventHandler; // to ensure creating non-null activity. + + ActivityContext context = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None, "Key=Value"); + using (Activity activity = source.CreateActivity(context)) + { + Assert.NotNull(activity); + Assert.Equal(context.TraceId, activity.TraceId); + Assert.Equal(context.SpanId, activity.ParentSpanId); + Assert.Equal(context.TraceFlags, activity.ActivityTraceFlags); + Assert.Equal(context.TraceState, activity.TraceStateString); + } + } + } + } +} diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index 153990a6ee49b0..abe5b8885e2dfa 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -1347,6 +1347,72 @@ public void ActivityCurrentNotSetToStopped() Assert.Same(started, Activity.Current); } + [Fact] + public void TestDispose() + { + Activity current = Activity.Current; + using (Activity activity = new Activity("Mine").Start()) + { + Assert.Same(activity, Activity.Current); + Assert.Same(current, activity.Parent); + } + + Assert.Same(current, Activity.Current); + } + + [Fact] + public void TestCustomProperties() + { + Activity activity = new Activity("Custom"); + activity.SetCustomProperty("P1", "Prop1"); + activity.SetCustomProperty("P2", "Prop2"); + activity.SetCustomProperty("P3", null); + + Assert.Equal("Prop1", activity.GetCustomProperty("P1")); + Assert.Equal("Prop2", activity.GetCustomProperty("P2")); + Assert.Null(activity.GetCustomProperty("P3")); + Assert.Null(activity.GetCustomProperty("P4")); + + activity.SetCustomProperty("P1", "Prop5"); + Assert.Equal("Prop5", activity.GetCustomProperty("P1")); + + } + + [Fact] + public void TestLinks() + { + Activity activity = new Activity("Links"); + IEnumerable links = activity.Links; + Assert.Equal(0, links.Count()); + + activity.AddLink(new ActivityLink(new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None))); + activity.AddLink(new ActivityLink(new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded))); + + links = activity.Links; + Assert.Equal(2, links.Count()); + + activity.AddLink(new ActivityLink(new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.Recorded, "I=Cool"))); + links = activity.Links; + Assert.Equal(3, links.Count()); + + Assert.Equal(ActivityTraceFlags.None, links.ElementAt(2).Context.TraceFlags); + Assert.Null(links.ElementAt(2).Context.TraceState); + Assert.Equal(ActivityTraceFlags.Recorded, links.ElementAt(1).Context.TraceFlags); + Assert.Null(links.ElementAt(1).Context.TraceState); + Assert.Equal(ActivityTraceFlags.Recorded, links.ElementAt(0).Context.TraceFlags); + Assert.NotNull(links.ElementAt(0).Context.TraceState); + } + + [Fact] + public void TestKind() + { + Activity activity = new Activity("Kind"); + Assert.Equal(ActivityKind.Internal, activity.Kind); + + activity.Kind = ActivityKind.Client; + Assert.Equal(ActivityKind.Client, activity.Kind); + } + public void Dispose() { Activity.Current = null; diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj b/src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj index 09d3a0d5d7f1b9..3d34832a280f1e 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj @@ -9,6 +9,7 @@ + From 38e13dfb4127b8df4be4496ac50b6192b990ee02 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 27 Feb 2020 13:13:22 -0800 Subject: [PATCH 2/4] Noah's Feedback --- ...em.Diagnostics.DiagnosticSourceActivity.cs | 30 ++- ...System.Diagnostics.DiagnosticSource.csproj | 2 +- .../src/System/Diagnostics/Activity.cs | 5 +- .../System/Diagnostics/ActivityListener.cs | 59 +++++ .../src/System/Diagnostics/ActivitySource.cs | 213 +++++++++++++----- .../Diagnostics/ActivitySourceEventArgs.cs | 51 ----- .../tests/ActivitySourceTests.cs | 175 +++++++------- 7 files changed, 329 insertions(+), 206 deletions(-) create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs delete mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs index 345a114e19b8d9..babcaa61ef904c 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs @@ -5,6 +5,8 @@ // Changes to this file must follow the https://aka.ms/api-review process. // ------------------------------------------------------------------------------ +using System.Collections.Generic; + namespace System.Diagnostics { public partial class Activity : IDisposable @@ -127,12 +129,6 @@ public virtual void OnActivityImport(System.Diagnostics.Activity activity, objec public System.Diagnostics.Activity StartActivity(System.Diagnostics.Activity activity, object? args) { throw null; } public void StopActivity(System.Diagnostics.Activity activity, object? args) { } } - public enum ActivitySourceEventOperation - { - SourceCreated = 0, - ActivityStarted = 1, - ActivityStopped = 2 - } public enum ActivityKind { Internal = 1, @@ -141,11 +137,6 @@ public enum ActivityKind Producer = 4, Consumer = 5, } - public sealed class ActivitySourceEventArgs : System.EventArgs - { - private ActivitySourceEventArgs() { throw null; } - public System.Diagnostics.ActivitySourceEventOperation Operation { get; } - } public readonly struct ActivityContext : IEquatable { public ActivityContext(System.Diagnostics.ActivityTraceId traceId, System.Diagnostics.ActivitySpanId spanId, System.Diagnostics.ActivityTraceFlags traceOptions, string? traceState = null) { throw null; } @@ -170,12 +161,19 @@ public sealed class ActivitySource : IDisposable { private ActivitySource() { throw null; } public ActivitySource(string name) { throw null; } - public static event EventHandler? OperationEvent { add { } remove { } } - public event EventHandler? ActivityEvent { add { } remove { } } - public static System.Collections.Generic.IEnumerable ActiveList => throw null; public string Name { get; } - public Activity? CreateActivity() { throw null; } - public Activity? CreateActivity(System.Diagnostics.ActivityContext context, System.Collections.Generic.IEnumerable? links = null, System.DateTimeOffset startTime = default) { throw null; } + public Activity? StartActivity() { throw null; } + public Activity? StartActivity(System.Diagnostics.ActivityContext context, System.Collections.Generic.IEnumerable? links = null, System.DateTimeOffset startTime = default) { throw null; } public void Dispose() { } + public static void AddListener(ActivityListener listener) {} } + public abstract class ActivityListener : IDisposable + { + public virtual bool EnableListening(string activitySourceName) { throw null; } + public virtual bool ShouldCreateActivity(string activitySourceName, ActivityContext context, IEnumerable? links) { throw null; } + public virtual void OnActivityStarted(Activity a) { throw null; } + public virtual void OnActivityStopped(Activity a) { throw null; } + public void Dispose() { throw null; } + protected virtual void Dispose(bool disposing) { throw null; } + } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj index e3ea414b161f67..b8db5f09128d84 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -36,7 +36,7 @@ - + diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs index a2ce37afaf5613..91a635cd9bb344 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -581,6 +581,8 @@ public void Stop() } SetCurrent(Parent); + + _source?.NotifyActivityStop(this); } } @@ -786,7 +788,6 @@ public void Dispose() if (!IsFinished) { Stop(); - _source?.RaiseActivityEvent(this, ActivitySourceEventArgs.s_activityStopped); } } @@ -877,8 +878,6 @@ internal static Activity CreateAndStart(ActivitySource source, ActivityContext c SetCurrent(activity); - source.RaiseActivityEvent(activity, ActivitySourceEventArgs.s_activityStarted); - return activity; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs new file mode 100644 index 00000000000000..b9613a3573cfce --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs @@ -0,0 +1,59 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +namespace System.Diagnostics +{ + /// + /// ActivityListener allows listening to the ActivitySource creation event + /// and decides to enable listening to the activity objects created from the ActivitySource. + /// + public abstract class ActivityListener : IDisposable + { + /// + /// EnableListening will be called with the object name to decide if interested to + /// listen to this object events. + /// + /// The name of the ActivitySource object to decide if need to listen to. + /// true if want to listen to ActivitySource object with the name activitySourceName. + public virtual bool EnableListening(string activitySourceName) => true; + + /// + /// ShouldCreateActivity allow deciding if should allow create the object. + /// The main scenario for this is when doing sampling and try to avoid creating objects which is not going to be used. + /// + /// The name of the object. + /// The object to get more information about the tracing context. + /// List of objects used with the tracing operation. + /// true if should create the object. + public virtual bool ShouldCreateActivity(string activitySourceName, ActivityContext context, IEnumerable? links) => true; + + /// + /// OnActivityStarted will get called when an object get created and started using an object + /// which the current listener is listening to. + /// + public virtual void OnActivityStarted(Activity a) {} + + /// + /// OnActivityStopped will get called when an object get stopped. + /// + public virtual void OnActivityStopped(Activity a) {} + + /// + /// Dispose this listener and detach it from any ActivitySource listening to. + /// + public void Dispose() + { + Dispose(true); + ActivitySource.DetachListener(this); + GC.SuppressFinalize(this); + } + + /// + /// Allow the implementer of this class to do any cleanup before disposing. + /// + protected virtual void Dispose(bool disposing) {} + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index 5dd2e319847353..7d7873c390fd38 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -2,25 +2,18 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Threading; using System.Collections.Generic; namespace System.Diagnostics { public sealed class ActivitySource : IDisposable { - private static object s_syncObject = new object(); - private static List s_activeSources = new List(); - private ActivitySource() { throw new InvalidOperationException(); } - - /// - /// Event to subscribe to get the notification when any ActivitySource object get created or released. - /// - public static event EventHandler? OperationEvent; + private static SynchronizedList s_activeSources = new SynchronizedList(); + private static SynchronizedList s_listeners = new SynchronizedList(); + private SynchronizedList? _listeners; - /// - /// Event to subscribe to get the notification when any Activity object get created or disposed. - /// - public event EventHandler? ActivityEvent; + private ActivitySource() { throw new InvalidOperationException(); } /// /// Construct an ActivitySource object with the input name @@ -33,31 +26,15 @@ public ActivitySource(string name) throw new ArgumentNullException(nameof(name)); } - lock (s_syncObject) - { - Name = name; - s_activeSources.Add(this); - } - - EventHandler? handlers = OperationEvent; - if (handlers != null) - { - handlers(this, ActivitySourceEventArgs.s_sourceCreated); - } - } + Name = name; + s_activeSources.Add(this); - /// - /// Returns the list of all created ActivitySource objects. - /// - public static IEnumerable ActiveList - { - get - { - lock (s_syncObject) + s_listeners.EnumWithAction(listener => { + if (listener.EnableListening(name)) { - return s_activeSources.ToArray(); + AddActivityListener(listener); } - } + }); } /// @@ -69,15 +46,7 @@ public static IEnumerable ActiveList /// Creates a new object if there is any listener to the Activity creation event, returns null otherwise. /// /// The created object or null if there is no any event listener. - public Activity? CreateActivity() - { - if (ActivityEvent == null) - { - return null; - } - - return Activity.CreateAndStart(this, default, null, default); - } + public Activity? StartActivity() => StartActivity(default, null, default); /// /// Creates a new object if there is any listener to the Activity events, returns null otherwise. @@ -86,14 +55,30 @@ public static IEnumerable ActiveList /// The optional list to initialize the created Activity object with. /// The optional start timestamp to set on the created Activity object. /// The created object or null if there is no any event listener. - public Activity? CreateActivity(ActivityContext context, IEnumerable? links = null, DateTimeOffset startTime = default) + public Activity? StartActivity(ActivityContext context, IEnumerable? links = null, DateTimeOffset startTime = default) { - if (ActivityEvent == null) + // _listeners can get assigned to null in Dispose. + SynchronizedList? listeners = _listeners; + if (listeners == null || listeners.Count == 0) { return null; } - return Activity.CreateAndStart(this, context, links, startTime); + Activity? activity = null; + + listeners.EnumWithAction(listener => { + if (listener.ShouldCreateActivity(Name, context, links)) + { + if (activity == null) + { + activity = Activity.CreateAndStart(this, context, links, startTime); + } + + listener.OnActivityStarted(activity); + } + }); + + return activity; } /// @@ -101,21 +86,143 @@ public static IEnumerable ActiveList /// public void Dispose() { - lock (s_syncObject) + s_activeSources.Remove(this); + _listeners = null; + } + + public static void AddListener(ActivityListener listener) + { + if (listener == null) { - if (s_activeSources.Remove(this)) + throw new ArgumentNullException(nameof(listener)); + } + + s_listeners.AddIfNotExist(listener); + + s_activeSources.EnumWithAction(source => { + if (listener.EnableListening(source.Name)) { - ActivityEvent = null; + source.AddActivityListener(listener); } + }); + } + + internal static void DetachListener(ActivityListener listener) + { + if (s_listeners.Remove(listener)) + { + s_activeSources.EnumWithAction(source => source.RemoveActivityListener(listener)); } } - internal void RaiseActivityEvent(Activity activity, ActivitySourceEventArgs eventArgs) + private void AddActivityListener(ActivityListener listener) { - EventHandler? handlers = ActivityEvent; - if (handlers != null) + if (_listeners == null) { - handlers(activity, eventArgs); + Interlocked.CompareExchange(ref _listeners, new SynchronizedList(), null); + } + + // _listeners can get assigned to null in Dispose. + SynchronizedList listeners = _listeners; + if (listeners != null) + { + listeners.AddIfNotExist(listener); + } + } + + private void RemoveActivityListener(ActivityListener listener) + { + Debug.Assert(listener != null); + + // _listeners can get assigned to null in Dispose. + SynchronizedList? listeners = _listeners; + if (listeners != null) + { + listeners.Remove(listener); + } + } + + internal void NotifyActivityStop(Activity activity) + { + Debug.Assert(activity != null); + + // _listeners can get assigned to null in Dispose. + SynchronizedList? listeners = _listeners; + if (listeners != null) + { + listeners.EnumWithAction(listener => listener.OnActivityStopped(activity)); + } + } + } + + + // SynchronizedList is a helper collection which ensure thread safety on the collection + // and allow enumerating the collection items and execute some action on the enumerated item and can detect any change in the collection + // during the enumeration which force restarting the enumeration again. + // Causion: We can have teh action executed on the same item more than once which is ok in our scenarios. + internal class SynchronizedList + { + private List _list; + private uint _version; + + public SynchronizedList() => _list = new List(); + + public void Add(T item) + { + lock (_list) + { + _list.Add(item); + _version++; + } + } + + public void AddIfNotExist(T item) + { + lock (_list) + { + if (!_list.Contains(item)) + { + _list.Add(item); + _version++; + } + } + } + + public bool Remove(T item) + { + lock (_list) + { + _version++; + return _list.Remove(item); + } + } + + public int Count => _list.Count; + + public void EnumWithAction(Action action) + { + uint version = _version; + int index = 0; + + while (index < _list.Count) + { + T item; + lock (_list) + { + if (version != _version) + { + version = _version; + index = 0; + continue; + } + + item = _list[index]; + index++; + } + + // Important to call the action outside the lock. + // This is the whole point we are having this wrapper class. + action(item); } } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs deleted file mode 100644 index 20324fc654e39e..00000000000000 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySourceEventArgs.cs +++ /dev/null @@ -1,51 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -namespace System.Diagnostics -{ - /// - /// ActivitySourceEventOperation define a possible values included inside the object when firing - /// different events. - /// - public enum ActivitySourceEventOperation - { - /// - /// SourceCreated is used when a new object is created. - /// - SourceCreated = 0, - - /// - /// ActivityStarted is used when a new object is created. - /// - ActivityStarted = 1, - - /// - /// ActivityStopped is used when dispossing object. - /// - ActivityStopped = 2 - } - - /// - /// ActivitySourceEventArgs represent the event argument used when notifying any listener - /// about and objects creation and disposing. - /// - public sealed class ActivitySourceEventArgs : EventArgs - { - internal static readonly ActivitySourceEventArgs s_sourceCreated = new ActivitySourceEventArgs(ActivitySourceEventOperation.SourceCreated); - internal static readonly ActivitySourceEventArgs s_activityStarted = new ActivitySourceEventArgs(ActivitySourceEventOperation.ActivityStarted); - internal static readonly ActivitySourceEventArgs s_activityStopped = new ActivitySourceEventArgs(ActivitySourceEventOperation.ActivityStopped); - - /// - /// Operation tells about the event operation. - /// - public ActivitySourceEventOperation Operation { get; } - - internal ActivitySourceEventArgs(ActivitySourceEventOperation operation) - { - Operation = operation; - } - - private ActivitySourceEventArgs() { throw new InvalidOperationException(); } - } -} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs index 8e939cdf0fe4f2..ef277119dbe7a0 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections.Generic; using System.Linq; using Xunit; @@ -15,24 +16,15 @@ public void CreateTest() using (ActivitySource source = new ActivitySource("Source1")) { Assert.Equal("Source1", source.Name); - Assert.True(ActivitySource.ActiveList.Count() > 0); - Assert.Same(source, ActivitySource.ActiveList.FirstOrDefault(c => c.Name == source.Name)); - - using (ActivitySource source1 = new ActivitySource("Source1")) - { - Assert.NotSame(source, source1); - } } - - Assert.Null(ActivitySource.ActiveList.FirstOrDefault(c => c.Name == "Source1")); } [Fact] - public void CreateActivityTest() + public void StartActivityTest() { using (ActivitySource source = new ActivitySource("Source2")) { - using (Activity activity = source.CreateActivity()) + using (Activity activity = source.StartActivity()) { // No listener, activity should be null Assert.Null(activity); @@ -43,85 +35,67 @@ public void CreateActivityTest() [Fact] public void SourceListenerTests() { - int count = 0; - - EventHandler eventHandler = (o, a) => - { - Assert.Equal(ActivitySourceEventOperation.SourceCreated, a.Operation); - count++; - }; - - ActivitySource.OperationEvent += eventHandler; + Listener listener; - using (ActivitySource source = new ActivitySource("Source3")) + using (listener = new Listener(enableListening: false, createActivities: false)) { - Assert.Equal(1, count); - using (ActivitySource source1 = new ActivitySource("Source4")) + ActivitySource.AddListener(listener); + using (ActivitySource source = new ActivitySource("Source3")) { - Assert.Equal(2, count); - using (ActivitySource source2 = new ActivitySource("Source5")) + using (Activity activity = source.StartActivity()) { - Assert.Equal(3, count); + // There is a listener which will not intereseted to listen to any source. + Assert.Null(activity); + Assert.Equal(0, listener.Count); + Assert.Equal(0, listener.SourceNames.Count()); } } } - using (ActivitySource source = new ActivitySource("Source6")) + using (listener = new Listener(enableListening: true, createActivities: false)) { - Assert.Equal(4, count); - - ActivitySource.OperationEvent -= eventHandler; - - using (ActivitySource source1 = new ActivitySource("Source7")) + ActivitySource.AddListener(listener); + using (ActivitySource source = new ActivitySource("Source4")) { - Assert.Equal(4, count); + using (Activity activity = source.StartActivity()) + { + // There is a listener which is listening but not allowing to create any activity. + Assert.Null(activity); + Assert.Equal(0, listener.Count); + Assert.Equal(1, listener.SourceNames.Count()); + } } } - } - - [Fact] - public void ActivityListenerTests() - { - int count = 0; - EventHandler eventHandler = (o, a) => { - if (a.Operation == ActivitySourceEventOperation.ActivityStarted) - { - count++; - } - else if (a.Operation == ActivitySourceEventOperation.ActivityStopped) - { - count--; - } - else - { - Assert.True(false, "Shouldn't get Operation value different than ActivityStarted or ActivityStopped"); - } - }; - - using (ActivitySource source = new ActivitySource("Source8")) + using (listener = new Listener(enableListening: true, createActivities: true)) { - source.ActivityEvent += eventHandler; - - using (Activity activity = source.CreateActivity()) + ActivitySource.AddListener(listener); + using (ActivitySource source = new ActivitySource("Source5")) { - Assert.NotNull(activity); - Assert.Equal(1, count); - using (Activity activity1 = source.CreateActivity()) + Assert.Equal(0, listener.Count); + Assert.Equal(1, listener.SourceNames.Count()); + + using (Activity activity = source.StartActivity()) { - Assert.NotNull(activity1); - Assert.Equal(2, count); + Assert.NotNull(activity); + + // We should already got Activity start event + Assert.Equal(1, listener.Count); } - Assert.Equal(1, count); - } - Assert.Equal(0, count); - source.ActivityEvent -= eventHandler; + // We should already got Activity stop event + Assert.Equal(0, listener.Count); + + using (ActivitySource source1 = new ActivitySource("Source5")) + { + Assert.Equal(2, listener.SourceNames.Count()); + foreach (string s in listener.SourceNames) + { + // We are listening to 2 sources with the same name. + Assert.Equal("Source5", s); + } + } - using (Activity activity = source.CreateActivity()) - { - Assert.Null(activity); - Assert.Equal(0, count); } } } @@ -129,22 +103,59 @@ public void ActivityListenerTests() [Fact] public void CreateActivityFromContextTests() { - EventHandler eventHandler = (o, a) => { }; - - using (ActivitySource source = new ActivitySource("Source9")) + using (Listener listener = new Listener(enableListening: true, createActivities: true)) { - source.ActivityEvent += eventHandler; // to ensure creating non-null activity. + ActivitySource.AddListener(listener); - ActivityContext context = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None, "Key=Value"); - using (Activity activity = source.CreateActivity(context)) + using (ActivitySource source = new ActivitySource("Source6")) { - Assert.NotNull(activity); - Assert.Equal(context.TraceId, activity.TraceId); - Assert.Equal(context.SpanId, activity.ParentSpanId); - Assert.Equal(context.TraceFlags, activity.ActivityTraceFlags); - Assert.Equal(context.TraceState, activity.TraceStateString); + Assert.Equal(0, listener.Count); + + ActivityContext context = new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None, "Key=Value"); + using (Activity activity = source.StartActivity(context)) + { + Assert.Equal(1, listener.Count); + + Assert.NotNull(activity); + Assert.Equal(context.TraceId, activity.TraceId); + Assert.Equal(context.SpanId, activity.ParentSpanId); + Assert.Equal(context.TraceFlags, activity.ActivityTraceFlags); + Assert.Equal(context.TraceState, activity.TraceStateString); + } } } } } + + public class Listener : ActivityListener + { + private bool _enableListening; + private bool _createActivities; + + private List _sourceNames = new List(); + + public Listener(bool enableListening, bool createActivities) + { + _enableListening = enableListening; + _createActivities = createActivities; + } + + public int Count { get; set; } + + public IEnumerable SourceNames => _sourceNames; + + public override bool EnableListening(string activitySourceName) + { + if (_enableListening) + _sourceNames.Add(activitySourceName); + + return _enableListening; + } + + public override bool ShouldCreateActivity(string activitySourceName, ActivityContext context, IEnumerable links) => _createActivities; + + public override void OnActivityStarted(Activity a) { Count++; } + + public override void OnActivityStopped(Activity a) { Count--;} + } } From 4d401dc394e748e088c2836b4fea865ba1011f0f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 27 Feb 2020 13:43:17 -0800 Subject: [PATCH 3/4] small fix --- .../src/System/Diagnostics/ActivitySource.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index 7d7873c390fd38..7c9e732ab0dadb 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -192,8 +192,12 @@ public bool Remove(T item) { lock (_list) { - _version++; - return _list.Remove(item); + if (_list.Remove(item)) + { + _version++; + return true; + } + return false; } } From fb128519690896628bb56aa1cae8316bf4f6b531 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 28 Feb 2020 08:26:34 -0800 Subject: [PATCH 4/4] Move AddListener() to ActivityListener.Start() --- .../ref/System.Diagnostics.DiagnosticSourceActivity.cs | 2 +- .../src/System/Diagnostics/ActivityListener.cs | 6 ++++++ .../src/System/Diagnostics/ActivitySource.cs | 2 +- .../tests/ActivitySourceTests.cs | 9 +++++---- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs index babcaa61ef904c..99d90f62470294 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs @@ -165,10 +165,10 @@ public sealed class ActivitySource : IDisposable public Activity? StartActivity() { throw null; } public Activity? StartActivity(System.Diagnostics.ActivityContext context, System.Collections.Generic.IEnumerable? links = null, System.DateTimeOffset startTime = default) { throw null; } public void Dispose() { } - public static void AddListener(ActivityListener listener) {} } public abstract class ActivityListener : IDisposable { + public void Start() { throw null; } public virtual bool EnableListening(string activitySourceName) { throw null; } public virtual bool ShouldCreateActivity(string activitySourceName, ActivityContext context, IEnumerable? links) { throw null; } public virtual void OnActivityStarted(Activity a) { throw null; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs index b9613a3573cfce..17a36080127089 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs @@ -12,6 +12,12 @@ namespace System.Diagnostics /// public abstract class ActivityListener : IDisposable { + /// + /// Start will activate this listener to start listeneing to the creation events and + /// to get the notification when objects get started and stopped. + /// + public void Start() => ActivitySource.AddListener(this); + /// /// EnableListening will be called with the object name to decide if interested to /// listen to this object events. diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index 7c9e732ab0dadb..7fd81227ce1241 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -90,7 +90,7 @@ public void Dispose() _listeners = null; } - public static void AddListener(ActivityListener listener) + internal static void AddListener(ActivityListener listener) { if (listener == null) { diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs index ef277119dbe7a0..2a76a8b2d07e01 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs @@ -39,7 +39,7 @@ public void SourceListenerTests() using (listener = new Listener(enableListening: false, createActivities: false)) { - ActivitySource.AddListener(listener); + listener.Start(); using (ActivitySource source = new ActivitySource("Source3")) { using (Activity activity = source.StartActivity()) @@ -54,7 +54,8 @@ public void SourceListenerTests() using (listener = new Listener(enableListening: true, createActivities: false)) { - ActivitySource.AddListener(listener); + listener.Start(); + using (ActivitySource source = new ActivitySource("Source4")) { using (Activity activity = source.StartActivity()) @@ -69,7 +70,7 @@ public void SourceListenerTests() using (listener = new Listener(enableListening: true, createActivities: true)) { - ActivitySource.AddListener(listener); + listener.Start(); using (ActivitySource source = new ActivitySource("Source5")) { Assert.Equal(0, listener.Count); @@ -105,7 +106,7 @@ public void CreateActivityFromContextTests() { using (Listener listener = new Listener(enableListening: true, createActivities: true)) { - ActivitySource.AddListener(listener); + listener.Start(); using (ActivitySource source = new ActivitySource("Source6")) {