From 2765e0779d7073b24e446d31befc5c3b3a616cc1 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 28 Apr 2020 11:57:43 -0700 Subject: [PATCH 1/6] Address the System.Diagnostics Feedback --- ...em.Diagnostics.DiagnosticSourceActivity.cs | 2 +- ...System.Diagnostics.DiagnosticSource.csproj | 6 + .../src/System/Diagnostics/Activity.cs | 193 ++++++++---------- .../src/System/Diagnostics/ActivityContext.cs | 23 +-- .../Diagnostics/ActivityContext.netcoreapp.cs | 21 ++ .../Diagnostics/ActivityContext.netfx.cs | 30 +++ .../src/System/Diagnostics/ActivityEvent.cs | 13 +- .../src/System/Diagnostics/ActivityLink.cs | 27 +-- .../Diagnostics/ActivityLink.netcoreapp.cs | 38 ++++ .../System/Diagnostics/ActivityLink.netfx.cs | 41 ++++ .../System/Diagnostics/ActivityListener.cs | 1 - .../src/System/Diagnostics/ActivitySource.cs | 10 +- .../tests/ActivityTests.cs | 16 +- 13 files changed, 247 insertions(+), 174 deletions(-) create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netfx.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs create mode 100644 src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netfx.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 bb08e3f5d30f02..6428488b65f0d2 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs @@ -169,7 +169,7 @@ public readonly struct ActivityEvent public ActivityEvent(string name, System.Collections.Generic.IEnumerable>? attributes) { throw null; } public string Name { get { throw null; } } public System.DateTimeOffset Timestamp { get { throw null; } } - public System.Collections.Generic.IEnumerable>? Attributes { get { throw null; } } + public System.Collections.Generic.IEnumerable> Attributes { get { throw null; } } } public readonly struct ActivityContext : System.IEquatable { 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 7ef0b1f1db8f8d..2db9da18322a9f 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -53,12 +53,17 @@ + + + + + @@ -80,5 +85,6 @@ + \ No newline at end of file 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 07f717e9256635..150d3e15d1d3a5 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -9,6 +9,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; +using System.Linq; using System.Threading; namespace System.Diagnostics @@ -37,7 +38,7 @@ public partial class Activity : IDisposable private static readonly IEnumerable s_emptyLinks = new ActivityLink[0]; private static readonly IEnumerable s_emptyEvents = new ActivityEvent[0]; #pragma warning restore CA1825 - private static ActivitySource s_defaultSource = new ActivitySource(string.Empty); + private static readonly ActivitySource s_defaultSource = new ActivitySource(string.Empty); private const byte ActivityTraceFlagsIsSet = 0b_1_0000000; // Internal flag to indicate if flags have been set private const int RequestIdMaxLength = 1024; @@ -74,15 +75,15 @@ public partial class Activity : IDisposable private byte _w3CIdFlags; - private LinkedListNode>? _tags; - private LinkedListNode>? _baggage; - private LinkedListNode? _links; - private LinkedListNode? _events; + private LinkedList>? _tags; + private LinkedList>? _baggage; + private LinkedList? _links; + private LinkedList? _events; private ConcurrentDictionary? _customProperties; private string? _displayName; /// - /// Kind describes the relationship between the Activity, its parents, and its children in a Trace. + /// Gets or sets the relationship between the Activity, its parents, and its children in a Trace. /// public ActivityKind Kind { get; private set; } = ActivityKind.Internal; @@ -94,21 +95,21 @@ public partial class Activity : IDisposable /// public string OperationName { get; } = null!; - /// - /// DisplayName is name mainly intended to be used in the UI and not necessary has to be - /// same as OperationName. - /// + /// Gets or sets the display name of the Activity + /// + /// DisplayName is intended to be used in a user interface and need not be the same as OperationName. + /// public string DisplayName { get => _displayName ?? OperationName; - set => _displayName = value; + set => _displayName = value ?? throw new ArgumentNullException(nameof(value)); } - /// - /// Get the ActivitySource object associated with this Activity. - /// All Activities created from public constructors will have a singleton source where the source name is empty string. - /// Otherwise, the source will be holding the object that created the Activity through ActivitySource.StartActivity. - /// + /// Get the ActivitySource object associated with this Activity. + /// + /// All Activities created from public constructors will have a singleton source where the source name is an empty string. + /// Otherwise, the source will hold the object that created the Activity through ActivitySource.StartActivity. + /// public ActivitySource Source { get; private set; } /// @@ -238,29 +239,13 @@ public string? RootId /// /// Tags are string-string key-value pairs that represent information that will - /// be logged along with the Activity to the logging system. This information + /// be logged along with the Activity to the logging system. This information /// however is NOT passed on to the children of this activity. /// /// public IEnumerable> Tags { - get - { - LinkedListNode>? tags = _tags; - return tags != null ? - Iterate(tags) : - s_emptyBaggageTags; - - static IEnumerable> Iterate(LinkedListNode>? tags) - { - do - { - yield return tags!.Value; - tags = tags.Next; - } - while (tags != null); - } - } + get => _tags != null ? _tags.Enumerate() : s_emptyBaggageTags; } /// @@ -269,21 +254,7 @@ public string? RootId /// public IEnumerable Events { - get - { - LinkedListNode? events = _events; - return events != null ? Iterate(events) : s_emptyEvents; - - static IEnumerable Iterate(LinkedListNode? events) - { - do - { - yield return events!.Value; - events = events.Next; - } - while (events != null); - } - } + get => _events != null ? _events.Enumerate() : s_emptyEvents; } /// @@ -292,21 +263,7 @@ static IEnumerable Iterate(LinkedListNode? events) /// public IEnumerable Links { - get - { - LinkedListNode? links = _links; - return links != null ? Iterate(links) : s_emptyLinks; - - static IEnumerable Iterate(LinkedListNode? links) - { - do - { - yield return links!.Value; - links = links.Next; - } - while (links != null); - } - } + get => _links != null ? _links.Enumerate() : s_emptyLinks; } /// @@ -336,14 +293,16 @@ static IEnumerable Iterate(LinkedListNode? links) Debug.Assert(activity != null); do { - for (LinkedListNode>? baggage = activity._baggage; baggage != null; baggage = baggage.Next) + if (activity._baggage != null) { - yield return baggage.Value; + for (LinkedListNode>? current = activity._baggage.First; current != null; current = current.Next) + { + yield return current.Value; + } } activity = activity.Parent; - } - while (activity != null); + } while (activity != null); } } } @@ -390,13 +349,12 @@ public Activity(string operationName) /// 'this' for convenient chaining public Activity AddTag(string key, string? value) { - LinkedListNode>? currentTags = _tags; - LinkedListNode> newTags = new LinkedListNode>(new KeyValuePair(key, value)); - do + KeyValuePair kvp = new KeyValuePair(key, value); + + if (_tags != null || Interlocked.CompareExchange(ref _tags, new LinkedList>(kvp), null) != null) { - newTags.Next = currentTags; - currentTags = Interlocked.CompareExchange(ref _tags, newTags, currentTags); - } while (!ReferenceEquals(newTags.Next, currentTags)); + _tags.Add(kvp); + } return this; } @@ -408,13 +366,10 @@ public Activity AddTag(string key, string? value) /// 'this' for convenient chaining public Activity AddEvent(ActivityEvent e) { - LinkedListNode? currentEvents = _events; - LinkedListNode newEvents = new LinkedListNode(e); - do + if (_events != null || Interlocked.CompareExchange(ref _events, new LinkedList(e), null) != null) { - newEvents.Next = currentEvents; - currentEvents = Interlocked.CompareExchange(ref _events, newEvents, currentEvents); - } while (!ReferenceEquals(newEvents.Next, currentEvents)); + _events.Add(e); + } return this; } @@ -430,14 +385,12 @@ public Activity AddEvent(ActivityEvent e) /// 'this' for convenient chaining public Activity AddBaggage(string key, string? value) { - LinkedListNode>? currentBaggage = _baggage; - LinkedListNode> newBaggage = new LinkedListNode>(new KeyValuePair(key, value)); + KeyValuePair kvp = new KeyValuePair(key, value); - do + if (_baggage != null || Interlocked.CompareExchange(ref _baggage, new LinkedList>(kvp), null) != null) { - newBaggage.Next = currentBaggage; - currentBaggage = Interlocked.CompareExchange(ref _baggage, newBaggage, currentBaggage); - } while (!ReferenceEquals(newBaggage.Next, currentBaggage)); + _baggage.Add(kvp); + } return this; } @@ -936,20 +889,14 @@ internal static Activity CreateAndStart(ActivitySource source, string name, Acti else activity._id = activity.GenerateHierarchicalId(); - if (links != null) + if (links != null && links.Any()) { - foreach (ActivityLink link in links) - { - activity._links = new LinkedListNode(link, activity._links); - } + activity._links = new LinkedList(links); } - if (tags != null) + if (tags != null && tags.Any()) { - foreach (KeyValuePair tag in tags) - { - activity._tags = new LinkedListNode>(tag, activity._tags); - } + activity._tags = new LinkedList>(tags); } activity.StartTimeUtc = startTime == default ? DateTime.UtcNow : startTime.DateTime; @@ -1191,21 +1138,59 @@ public ActivityIdFormat IdFormat private set => _state = (_state & ~State.FormatFlags) | (State)((byte)value & (byte)State.FormatFlags); } - /// - /// Having our own key-value linked list allows us to be more efficient - /// private partial class LinkedListNode { public LinkedListNode(T value) => Value = value; - public LinkedListNode(T value, LinkedListNode? next) - { - Value = value; - Next = next; - } public T Value; public LinkedListNode? Next; } + private class LinkedList + { + private LinkedListNode _first; + private LinkedListNode _last; + + public LinkedList(T firstValue) => _last =_first = new LinkedListNode(firstValue); + + public LinkedList(IEnumerable list) + { + IEnumerator enumerator = list.GetEnumerator(); + Debug.Assert(enumerator.MoveNext()); + _last =_first = new LinkedListNode(enumerator.Current); + + while (enumerator.MoveNext()) + { + _last.Next = new LinkedListNode(enumerator.Current); + _last = _last.Next; + } + } + + public LinkedListNode First => _first; + + public void Add(T value) + { + LinkedListNode newNode = new LinkedListNode(value); + LinkedListNode last; + + do + { + last = _last; + } while (!object.ReferenceEquals(Interlocked.CompareExchange(ref _last, newNode, last), last)); + + last.Next = newNode; + } + + public IEnumerable Enumerate() + { + LinkedListNode? current = _first; + do + { + yield return current.Value; + current = current.Next; + } while (current != null); + } + } + [Flags] private enum State : byte { diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs index e01cf46f852e2a..9c242fd87e334f 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.cs @@ -10,7 +10,7 @@ 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 + public readonly partial struct ActivityContext : IEquatable { /// /// Construct a new object of ActivityContext. @@ -44,12 +44,12 @@ public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityT public ActivitySpanId SpanId { get; } /// - /// The flags for the details about the trace. + /// These flags are defined by the W3C standard along with the ID for the activity. /// public ActivityTraceFlags TraceFlags { get; } /// - /// system-specific configuration data. + /// Holds the W3C 'tracestate' header as a string. /// public string? TraceState { get; } @@ -58,22 +58,5 @@ public ActivityContext(ActivityTraceId traceId, ActivitySpanId spanId, ActivityT public override bool Equals(object? obj) => (obj is ActivityContext context) ? Equals(context) : false; public static bool operator ==(ActivityContext left, ActivityContext right) => left.Equals(right); public static bool operator !=(ActivityContext left, ActivityContext right) => !(left == right); - - 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 much, 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; - } } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs new file mode 100644 index 00000000000000..aaa2f213042a18 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs @@ -0,0 +1,21 @@ +// 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 +{ + /// + /// 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 partial struct ActivityContext : IEquatable + { + public override int GetHashCode() + { + if (this == default) + return 0; + + return HashCode.Combine(TraceId, SpanId, TraceFlags, TraceState == null ? "" : TraceState); + } + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netfx.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netfx.cs new file mode 100644 index 00000000000000..a84bad6f3cc743 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netfx.cs @@ -0,0 +1,30 @@ +// 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 +{ + /// + /// 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 partial struct ActivityContext : IEquatable + { + 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 much, 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/ActivityEvent.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityEvent.cs index 12836bfc30ee3b..27bc3b0ab2367a 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityEvent.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityEvent.cs @@ -36,11 +36,8 @@ public ActivityEvent(string name, DateTimeOffset timestamp) : this(name, timesta /// /// Event name. /// Event attributes. - public ActivityEvent(string name, IEnumerable>? attributes) + public ActivityEvent(string name, IEnumerable>? attributes) : this(name, default, attributes) { - this.Name = name ?? string.Empty; - this.Attributes = attributes ?? s_emptyAttributes; - this.Timestamp = DateTimeOffset.UtcNow; } /// @@ -51,9 +48,9 @@ public ActivityEvent(string name, IEnumerable>? att /// Event attributes. public ActivityEvent(string name, DateTimeOffset timestamp, IEnumerable>? attributes) { - this.Name = name ?? string.Empty; - this.Attributes = attributes ?? s_emptyAttributes; - this.Timestamp = timestamp != default ? timestamp : DateTimeOffset.UtcNow; + Name = name ?? string.Empty; + Attributes = attributes ?? s_emptyAttributes; + Timestamp = timestamp != default ? timestamp : DateTimeOffset.UtcNow; } /// @@ -69,6 +66,6 @@ public ActivityEvent(string name, DateTimeOffset timestamp, IEnumerable /// Gets the collection of attributes associated with the event. /// - public IEnumerable>? Attributes { get; } + public IEnumerable> Attributes { get; } } } \ 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 index bea45b55bd1343..920459e5083548 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.cs @@ -12,7 +12,7 @@ namespace System.Diagnostics /// 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 : IEquatable + public readonly partial struct ActivityLink : IEquatable { /// /// Construct a new object which can be linked to an Activity object. @@ -46,30 +46,5 @@ public ActivityLink(ActivityContext context, IEnumerable Context == value.Context && value.Attributes == Attributes; public static bool operator ==(ActivityLink left, ActivityLink right) => left.Equals(right); public static bool operator !=(ActivityLink left, ActivityLink right) => !left.Equals(right); - - 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) + this.Context.GetHashCode(); - if (Attributes != null) - { - foreach (KeyValuePair kvp in Attributes) - { - hash = ((hash << 5) + hash) + kvp.Key.GetHashCode(); - if (kvp.Value != null) - { - hash = ((hash << 5) + hash) + kvp.Value.GetHashCode(); - } - } - } - - return hash; - } } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs new file mode 100644 index 00000000000000..a74ea25714bbb4 --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs @@ -0,0 +1,38 @@ +// 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 partial struct ActivityLink : IEquatable + { + public override int GetHashCode() + { + if (this == default) + return 0; + + HashCode hashCode = default; + hashCode.Add(Context); + if (Attributes != null) + { + foreach (KeyValuePair kvp in Attributes) + { + hashCode.Add(kvp.Key); + if (kvp.Value != null) + { + hashCode.Add(kvp.Value); + } + } + } + return hashCode.ToHashCode(); + } + } +} \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netfx.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netfx.cs new file mode 100644 index 00000000000000..678e1d73a66beb --- /dev/null +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netfx.cs @@ -0,0 +1,41 @@ +// 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 partial struct ActivityLink : IEquatable + { + 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) + this.Context.GetHashCode(); + if (Attributes != null) + { + foreach (KeyValuePair kvp in Attributes) + { + hash = ((hash << 5) + hash) + kvp.Key.GetHashCode(); + if (kvp.Value != null) + { + hash = ((hash << 5) + hash) + kvp.Value.GetHashCode(); + } + } + } + return hash; + } + } +} \ No newline at end of file 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 47703a5e33bc09..5c07128a2d0c14 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityListener.cs @@ -21,7 +21,6 @@ public sealed class ActivityListener : IDisposable /// public ActivityListener() { - } /// 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 673111b2855222..dcf750c32b0785 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -9,12 +9,10 @@ namespace System.Diagnostics { public sealed class ActivitySource : IDisposable { - private static SynchronizedList s_activeSources = new SynchronizedList(); - private static SynchronizedList s_allListeners = new SynchronizedList(); + private static readonly SynchronizedList s_activeSources = new SynchronizedList(); + private static readonly SynchronizedList s_allListeners = new SynchronizedList(); private SynchronizedList? _listeners; - private ActivitySource() { throw new InvalidOperationException(); } - /// /// Construct an ActivitySource object with the input name /// @@ -250,10 +248,10 @@ internal void NotifyActivityStop(Activity 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 the action executed on the same item more than once which is ok in our scenarios. + // Caution: We can have the action executed on the same item more than once which is ok in our scenarios. internal class SynchronizedList { - private List _list; + private readonly List _list; private uint _version; public SynchronizedList() => _list = new List(); diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs index 4151e6ba16ffd5..11dd57028538f4 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs @@ -85,9 +85,9 @@ public void Baggage() Assert.Equal("world", anotherActivity.GetBaggageItem("hello")); Assert.Equal(4, anotherActivity.Baggage.Count()); Assert.Equal(new KeyValuePair("hello", "world"), anotherActivity.Baggage.First()); - Assert.Equal(new KeyValuePair(Key + 2, Value + 2), anotherActivity.Baggage.Skip(1).First()); + Assert.Equal(new KeyValuePair(Key + 0, Value + 0), anotherActivity.Baggage.Skip(1).First()); Assert.Equal(new KeyValuePair(Key + 1, Value + 1), anotherActivity.Baggage.Skip(2).First()); - Assert.Equal(new KeyValuePair(Key + 0, Value + 0), anotherActivity.Baggage.Skip(3).First()); + Assert.Equal(new KeyValuePair(Key + 2, Value + 2), anotherActivity.Baggage.Skip(3).First()); } finally { @@ -116,8 +116,8 @@ public void Tags() Assert.Equal(activity, activity.AddTag(Key + i, Value + i)); List> tags = activity.Tags.ToList(); Assert.Equal(i + 1, tags.Count); - Assert.Equal(tags[tags.Count - i - 1].Key, Key + i); - Assert.Equal(tags[tags.Count - i - 1].Value, Value + i); + Assert.Equal(tags[i].Key, Key + i); + Assert.Equal(tags[i].Value, Value + i); } } @@ -1411,12 +1411,12 @@ public void TestEvent() Assert.True(object.ReferenceEquals(activity, activity.AddEvent(new ActivityEvent("Event2", ts2)))); Assert.Equal(2, activity.Events.Count()); - Assert.Equal("Event2", activity.Events.ElementAt(0).Name); - Assert.Equal(ts2, activity.Events.ElementAt(0).Timestamp); + Assert.Equal("Event1", activity.Events.ElementAt(0).Name); + Assert.Equal(ts1, activity.Events.ElementAt(0).Timestamp); Assert.Equal(0, activity.Events.ElementAt(0).Attributes.Count()); - Assert.Equal("Event1", activity.Events.ElementAt(1).Name); - Assert.Equal(ts1, activity.Events.ElementAt(1).Timestamp); + Assert.Equal("Event2", activity.Events.ElementAt(1).Name); + Assert.Equal(ts2, activity.Events.ElementAt(1).Timestamp); Assert.Equal(0, activity.Events.ElementAt(1).Attributes.Count()); } From 4ef59ad997bd9739f1b416c470a7b6ba40f2ef6d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 29 Apr 2020 12:57:35 -0700 Subject: [PATCH 2/6] Address the feedback of the feedback :-) --- ...System.Diagnostics.DiagnosticSource.csproj | 2 - .../src/System/Diagnostics/Activity.cs | 45 ++++++++++++------- 2 files changed, 28 insertions(+), 19 deletions(-) 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 2db9da18322a9f..4d78e61dd4f24f 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -53,7 +53,6 @@ - @@ -85,6 +84,5 @@ - \ No newline at end of file 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 150d3e15d1d3a5..0bdab053b03582 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -83,7 +83,7 @@ public partial class Activity : IDisposable private string? _displayName; /// - /// Gets or sets the relationship between the Activity, its parents, and its children in a Trace. + /// Gets the relationship between the Activity, its parents, and its children in a Trace. /// public ActivityKind Kind { get; private set; } = ActivityKind.Internal; @@ -889,14 +889,26 @@ internal static Activity CreateAndStart(ActivitySource source, string name, Acti else activity._id = activity.GenerateHierarchicalId(); - if (links != null && links.Any()) + if (links != null) { - activity._links = new LinkedList(links); + using (IEnumerator enumerator = links.GetEnumerator()) + { + if (enumerator.MoveNext()) + { + activity._links = new LinkedList(enumerator); + } + } } - if (tags != null && tags.Any()) + if (tags != null) { - activity._tags = new LinkedList>(tags); + using (IEnumerator> enumerator = tags.GetEnumerator()) + { + if (enumerator.MoveNext()) + { + activity._tags = new LinkedList>(enumerator); + } + } } activity.StartTimeUtc = startTime == default ? DateTime.UtcNow : startTime.DateTime; @@ -1145,22 +1157,21 @@ private partial class LinkedListNode public LinkedListNode? Next; } + // We are not using the public LinkedList because we need to ensure thread safety operation on the list. private class LinkedList { private LinkedListNode _first; - private LinkedListNode _last; + private volatile LinkedListNode _last; public LinkedList(T firstValue) => _last =_first = new LinkedListNode(firstValue); - public LinkedList(IEnumerable list) + public LinkedList(IEnumerator e) { - IEnumerator enumerator = list.GetEnumerator(); - Debug.Assert(enumerator.MoveNext()); - _last =_first = new LinkedListNode(enumerator.Current); + _last =_first = new LinkedListNode(e.Current); - while (enumerator.MoveNext()) + while (e.MoveNext()) { - _last.Next = new LinkedListNode(enumerator.Current); + _last.Next = new LinkedListNode(e.Current); _last = _last.Next; } } @@ -1170,14 +1181,14 @@ public LinkedList(IEnumerable list) public void Add(T value) { LinkedListNode newNode = new LinkedListNode(value); - LinkedListNode last; + SpinWait sw = default; - do + while (Interlocked.CompareExchange(ref _last.Next, newNode, null) != null) { - last = _last; - } while (!object.ReferenceEquals(Interlocked.CompareExchange(ref _last, newNode, last), last)); + sw.SpinOnce(); + } - last.Next = newNode; + _last = newNode; } public IEnumerable Enumerate() From 8c117ff54b0e3515958384f37ae0a0693e4c68e0 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 29 Apr 2020 14:00:49 -0700 Subject: [PATCH 3/6] Use lock instead of InterlockedExchange+SpinWait --- .../src/System/Diagnostics/Activity.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 0bdab053b03582..5b0e52b2ba79e4 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -1161,7 +1161,7 @@ private partial class LinkedListNode private class LinkedList { private LinkedListNode _first; - private volatile LinkedListNode _last; + private LinkedListNode _last; public LinkedList(T firstValue) => _last =_first = new LinkedListNode(firstValue); @@ -1181,14 +1181,12 @@ public LinkedList(IEnumerator e) public void Add(T value) { LinkedListNode newNode = new LinkedListNode(value); - SpinWait sw = default; - while (Interlocked.CompareExchange(ref _last.Next, newNode, null) != null) + lock (_first) { - sw.SpinOnce(); + _last.Next = newNode; + _last = newNode; } - - _last = newNode; } public IEnumerable Enumerate() From c1664e10814af4407eb7054db14c27f943e638b2 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 29 Apr 2020 15:29:34 -0700 Subject: [PATCH 4/6] Remove System.Linq usage --- .../src/System/Diagnostics/Activity.cs | 1 - 1 file changed, 1 deletion(-) 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 5b0e52b2ba79e4..a6ba3862aebc28 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -9,7 +9,6 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; -using System.Linq; using System.Threading; namespace System.Diagnostics From 6c25b6fa74041b546d244bd51ada09f1fc5cddbd Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 1 May 2020 10:24:46 -0700 Subject: [PATCH 5/6] More feedback addressing --- .../src/System/Diagnostics/Activity.cs | 4 ++-- .../src/System/Diagnostics/ActivityContext.netcoreapp.cs | 8 +------- .../src/System/Diagnostics/ActivityLink.netcoreapp.cs | 9 +-------- 3 files changed, 4 insertions(+), 17 deletions(-) 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 a6ba3862aebc28..926d629f286ea1 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs @@ -1162,11 +1162,11 @@ private class LinkedList private LinkedListNode _first; private LinkedListNode _last; - public LinkedList(T firstValue) => _last =_first = new LinkedListNode(firstValue); + public LinkedList(T firstValue) => _last = _first = new LinkedListNode(firstValue); public LinkedList(IEnumerator e) { - _last =_first = new LinkedListNode(e.Current); + _last = _first = new LinkedListNode(e.Current); while (e.MoveNext()) { diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs index aaa2f213042a18..ea94205c0001e6 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityContext.netcoreapp.cs @@ -10,12 +10,6 @@ namespace System.Diagnostics /// public readonly partial struct ActivityContext : IEquatable { - public override int GetHashCode() - { - if (this == default) - return 0; - - return HashCode.Combine(TraceId, SpanId, TraceFlags, TraceState == null ? "" : TraceState); - } + public override int GetHashCode() => HashCode.Combine(TraceId, SpanId, TraceFlags, TraceState); } } \ No newline at end of file diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs index a74ea25714bbb4..d19a6bdfd36cc0 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs @@ -16,20 +16,13 @@ namespace System.Diagnostics { public override int GetHashCode() { - if (this == default) - return 0; - HashCode hashCode = default; hashCode.Add(Context); if (Attributes != null) { foreach (KeyValuePair kvp in Attributes) { - hashCode.Add(kvp.Key); - if (kvp.Value != null) - { - hashCode.Add(kvp.Value); - } + hashCode.Add(kvp.Key, kvp.Value); } } return hashCode.ToHashCode(); From 13e3cb67eff2232a7268cfbb3e9633ba29e60abf Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 1 May 2020 11:27:15 -0700 Subject: [PATCH 6/6] Fix compilation --- .../src/System/Diagnostics/ActivityLink.netcoreapp.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs index d19a6bdfd36cc0..62f1af752e4dc4 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivityLink.netcoreapp.cs @@ -22,7 +22,8 @@ public override int GetHashCode() { foreach (KeyValuePair kvp in Attributes) { - hashCode.Add(kvp.Key, kvp.Value); + hashCode.Add(kvp.Key); + hashCode.Add(kvp.Value); } } return hashCode.ToHashCode();