Skip to content

Address the feedback left on the Introducing Metrics APIs PR #53324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 28, 2021
Merged

Address the feedback left on the Introducing Metrics APIs PR #53324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c16a441
Address @stephentoub Feedback on Metrics APIs PR
tarekgh May 27, 2021
01f2ae4
Address @ViktorHofer feedback
tarekgh May 27, 2021
ba90a56
Merge branch 'main' into AddressMetricsPRFeedback
tarekgh May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,6 @@
<value>Unable to initialize all required reflection objects</value>
</data>
<data name="UnsupportedType" xml:space="preserve">
<value>{0} is unsupported type for this operation. The only supported types are byte, short, int, long, float, double and decimal.</value>
<value>{0} is unsupported type for this operation. The only supported types are byte, short, int, long, float, double, and decimal. </value>
</data>
</root>
2 changes: 1 addition & 1 deletion ...raries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
<Compile Include="System\Diagnostics\ActivityListener.cs" />
<Compile Include="System\Diagnostics\ActivitySource.cs" />
<Compile Include="System\Diagnostics\DiagnosticSourceActivity.cs" />
<Compile Include="System\Diagnostics\LinkedList.cs" />
<Compile Include="System\Diagnostics\DiagLinkedList.cs" />
<Compile Include="System\Diagnostics\RandomNumberGenerator.cs" />
<Compile Include="System\Diagnostics\Metrics\Counter.cs" />
<Compile Include="System\Diagnostics\Metrics\Histogram.cs" />
Expand Down
54 changes: 27 additions & 27 deletions src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ public partial class Activity : IDisposable

private TagsLinkedList? _tags;
private BaggageLinkedList? _baggage;
private LinkedList<ActivityLink>? _links;
private LinkedList<ActivityEvent>? _events;
private DiagLinkedList<ActivityLink>? _links;
private DiagLinkedList<ActivityEvent>? _events;
private Dictionary<string, object>? _customProperties;
private string? _displayName;
private ActivityStatusCode _statusCode;
Expand Down Expand Up @@ -341,7 +341,7 @@ public IEnumerable<ActivityLink> Links
{
if (activity._baggage != null)
{
for (LinkedListNode<KeyValuePair<string, string?>>? current = activity._baggage.First; current != null; current = current.Next)
for (DiagNode<KeyValuePair<string, string?>>? current = activity._baggage.First; current != null; current = current.Next)
{
yield return current.Value;
}
Expand Down Expand Up @@ -455,7 +455,7 @@ public Activity SetTag(string key, object? value)
/// <returns>'this' for convenient chaining</returns>
public Activity AddEvent(ActivityEvent e)
{
if (_events != null || Interlocked.CompareExchange(ref _events, new LinkedList<ActivityEvent>(e), null) != null)
if (_events != null || Interlocked.CompareExchange(ref _events, new DiagLinkedList<ActivityEvent>(e), null) != null)
{
_events.Add(e);
}
Expand Down Expand Up @@ -1030,7 +1030,7 @@ internal static Activity Create(ActivitySource source, string name, ActivityKind
{
if (enumerator.MoveNext())
{
activity._links = new LinkedList<ActivityLink>(enumerator);
activity._links = new DiagLinkedList<ActivityLink>(enumerator);
}
}
}
Expand Down Expand Up @@ -1325,15 +1325,15 @@ public ActivityIdFormat IdFormat

private sealed class BaggageLinkedList : IEnumerable<KeyValuePair<string, string?>>
{
private LinkedListNode<KeyValuePair<string, string?>>? _first;
private DiagNode<KeyValuePair<string, string?>>? _first;

public BaggageLinkedList(KeyValuePair<string, string?> firstValue, bool set = false) => _first = ((set && firstValue.Value == null) ? null : new LinkedListNode<KeyValuePair<string, string?>>(firstValue));
public BaggageLinkedList(KeyValuePair<string, string?> firstValue, bool set = false) => _first = ((set && firstValue.Value == null) ? null : new DiagNode<KeyValuePair<string, string?>>(firstValue));

public LinkedListNode<KeyValuePair<string, string?>>? First => _first;
public DiagNode<KeyValuePair<string, string?>>? First => _first;

public void Add(KeyValuePair<string, string?> value)
{
LinkedListNode<KeyValuePair<string, string?>> newNode = new LinkedListNode<KeyValuePair<string, string?>>(value);
DiagNode<KeyValuePair<string, string?>> newNode = new DiagNode<KeyValuePair<string, string?>>(value);

lock (this)
{
Expand All @@ -1352,7 +1352,7 @@ public void Set(KeyValuePair<string, string?> value)

lock (this)
{
LinkedListNode<KeyValuePair<string, string?>>? current = _first;
DiagNode<KeyValuePair<string, string?>>? current = _first;
while (current != null)
{
if (current.Value.Key == value.Key)
Expand All @@ -1364,7 +1364,7 @@ public void Set(KeyValuePair<string, string?> value)
current = current.Next;
}

LinkedListNode<KeyValuePair<string, string?>> newNode = new LinkedListNode<KeyValuePair<string, string?>>(value);
DiagNode<KeyValuePair<string, string?>> newNode = new DiagNode<KeyValuePair<string, string?>>(value);
newNode.Next = _first;
_first = newNode;
}
Expand All @@ -1385,7 +1385,7 @@ public void Remove(string key)
return;
}

LinkedListNode<KeyValuePair<string, string?>> previous = _first;
DiagNode<KeyValuePair<string, string?>> previous = _first;

while (previous.Next != null)
{
Expand All @@ -1407,20 +1407,20 @@ public void Remove(string key)

private sealed class TagsLinkedList : IEnumerable<KeyValuePair<string, object?>>
{
private LinkedListNode<KeyValuePair<string, object?>>? _first;
private LinkedListNode<KeyValuePair<string, object?>>? _last;
private DiagNode<KeyValuePair<string, object?>>? _first;
private DiagNode<KeyValuePair<string, object?>>? _last;

private StringBuilder? _stringBuilder;

public TagsLinkedList(KeyValuePair<string, object?> firstValue, bool set = false) => _last = _first = ((set && firstValue.Value == null) ? null : new LinkedListNode<KeyValuePair<string, object?>>(firstValue));
public TagsLinkedList(KeyValuePair<string, object?> firstValue, bool set = false) => _last = _first = ((set && firstValue.Value == null) ? null : new DiagNode<KeyValuePair<string, object?>>(firstValue));

public TagsLinkedList(IEnumerator<KeyValuePair<string, object?>> e)
{
_last = _first = new LinkedListNode<KeyValuePair<string, object?>>(e.Current);
_last = _first = new DiagNode<KeyValuePair<string, object?>>(e.Current);

while (e.MoveNext())
{
_last.Next = new LinkedListNode<KeyValuePair<string, object?>>(e.Current);
_last.Next = new DiagNode<KeyValuePair<string, object?>>(e.Current);
_last = _last.Next;
}
}
Expand All @@ -1438,24 +1438,24 @@ public void Add(IEnumerable<KeyValuePair<string, object?>> list)

if (_first == null)
{
_last = _first = new LinkedListNode<KeyValuePair<string, object?>>(e.Current);
_last = _first = new DiagNode<KeyValuePair<string, object?>>(e.Current);
}
else
{
_last!.Next = new LinkedListNode<KeyValuePair<string, object?>>(e.Current);
_last!.Next = new DiagNode<KeyValuePair<string, object?>>(e.Current);
_last = _last.Next;
}

while (e.MoveNext())
{
_last.Next = new LinkedListNode<KeyValuePair<string, object?>>(e.Current);
_last.Next = new DiagNode<KeyValuePair<string, object?>>(e.Current);
_last = _last.Next;
}
}

public void Add(KeyValuePair<string, object?> value)
{
LinkedListNode<KeyValuePair<string, object?>> newNode = new LinkedListNode<KeyValuePair<string, object?>>(value);
DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);

lock (this)
{
Expand All @@ -1475,7 +1475,7 @@ public void Add(KeyValuePair<string, object?> value)
public object? Get(string key)
{
// We don't take the lock here so it is possible the Add/Remove operations mutate the list during the Get operation.
LinkedListNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = _first;
while (current != null)
{
if (current.Value.Key == key)
Expand Down Expand Up @@ -1504,7 +1504,7 @@ public void Remove(string key)
return;
}

LinkedListNode<KeyValuePair<string, object?>> previous = _first;
DiagNode<KeyValuePair<string, object?>> previous = _first;

while (previous.Next != null)
{
Expand All @@ -1528,7 +1528,7 @@ public void Set(KeyValuePair<string, object?> value)

lock (this)
{
LinkedListNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = _first;
while (current != null)
{
if (current.Value.Key == value.Key)
Expand All @@ -1540,7 +1540,7 @@ public void Set(KeyValuePair<string, object?> value)
current = current.Next;
}

LinkedListNode<KeyValuePair<string, object?>> newNode = new LinkedListNode<KeyValuePair<string, object?>>(value);
DiagNode<KeyValuePair<string, object?>> newNode = new DiagNode<KeyValuePair<string, object?>>(value);
if (_first == null)
{
_first = _last = newNode;
Expand All @@ -1561,7 +1561,7 @@ public void Set(KeyValuePair<string, object?> value)

public IEnumerable<KeyValuePair<string, string?>> EnumerateStringValues()
{
LinkedListNode<KeyValuePair<string, object?>>? current = _first;
DiagNode<KeyValuePair<string, object?>>? current = _first;

while (current != null)
{
Expand All @@ -1588,7 +1588,7 @@ public override string ToString()
_stringBuilder.Append(':');
_stringBuilder.Append(_first.Value.Value);

LinkedListNode<KeyValuePair<string, object?>>? current = _first.Next;
DiagNode<KeyValuePair<string, object?>>? current = _first.Next;
while (current != null)
{
_stringBuilder.Append(", ");
Expand Down
42 changes: 21 additions & 21 deletions ...urce/src/System/Diagnostics/LinkedList.cs → .../src/System/Diagnostics/DiagLinkedList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,36 @@

namespace System.Diagnostics
{
internal sealed partial class LinkedListNode<T>
internal sealed partial class DiagNode<T>
{
public LinkedListNode(T value) => Value = value;
public DiagNode(T value) => Value = value;
public T Value;
public LinkedListNode<T>? Next;
public DiagNode<T>? Next;
}

// We are not using the public LinkedList<T> because we need to ensure thread safety operation on the list.
internal sealed class LinkedList<T> : IEnumerable<T>
internal sealed class DiagLinkedList<T> : IEnumerable<T>
{
private LinkedListNode<T>? _first;
private LinkedListNode<T>? _last;
private DiagNode<T>? _first;
private DiagNode<T>? _last;

public LinkedList() {}
public DiagLinkedList() {}

public LinkedList(T firstValue) => _last = _first = new LinkedListNode<T>(firstValue);
public DiagLinkedList(T firstValue) => _last = _first = new DiagNode<T>(firstValue);

public LinkedList(IEnumerator<T> e)
public DiagLinkedList(IEnumerator<T> e)
{
Debug.Assert(e is not null);
_last = _first = new LinkedListNode<T>(e.Current);
_last = _first = new DiagNode<T>(e.Current);

while (e.MoveNext())
{
_last.Next = new LinkedListNode<T>(e.Current);
_last.Next = new DiagNode<T>(e.Current);
_last = _last.Next;
}
}

public LinkedListNode<T>? First => _first;
public DiagNode<T>? First => _first;

public void Clear()
{
Expand All @@ -46,7 +46,7 @@ public void Clear()
}
}

private void UnsafeAdd(LinkedListNode<T> newNode)
private void UnsafeAdd(DiagNode<T> newNode)
{
if (_first is null)
{
Expand All @@ -63,7 +63,7 @@ private void UnsafeAdd(LinkedListNode<T> newNode)

public void Add(T value)
{
LinkedListNode<T> newNode = new LinkedListNode<T>(value);
DiagNode<T> newNode = new DiagNode<T>(value);

lock (this)
{
Expand All @@ -75,7 +75,7 @@ public bool AddIfNotExist(T value, Func<T, T, bool> compare)
{
lock (this)
{
LinkedListNode<T>? current = _first;
DiagNode<T>? current = _first;
while (current is not null)
{
if (compare(value, current.Value))
Expand All @@ -86,7 +86,7 @@ public bool AddIfNotExist(T value, Func<T, T, bool> compare)
current = current.Next;
}

LinkedListNode<T> newNode = new LinkedListNode<T>(value);
DiagNode<T> newNode = new DiagNode<T>(value);
UnsafeAdd(newNode);

return true;
Expand All @@ -97,7 +97,7 @@ public bool AddIfNotExist(T value, Func<T, T, bool> compare)
{
lock (this)
{
LinkedListNode<T>? previous = _first;
DiagNode<T>? previous = _first;
if (previous is null)
{
return default;
Expand All @@ -113,7 +113,7 @@ public bool AddIfNotExist(T value, Func<T, T, bool> compare)
return previous.Value;
}

LinkedListNode<T>? current = previous.Next;
DiagNode<T>? current = previous.Next;

while (current is not null)
{
Expand All @@ -138,7 +138,7 @@ public bool AddIfNotExist(T value, Func<T, T, bool> compare)

public void AddFront(T value)
{
LinkedListNode<T> newNode = new LinkedListNode<T>(value);
DiagNode<T> newNode = new DiagNode<T>(value);

lock (this)
{
Expand All @@ -156,10 +156,10 @@ public void AddFront(T value)
// Note: Some consumers use this Enumerator dynamically to avoid allocations.
internal struct Enumerator<T> : IEnumerator<T>
{
private LinkedListNode<T>? _nextNode;
private DiagNode<T>? _nextNode;
[AllowNull, MaybeNull] private T _currentItem;

public Enumerator(LinkedListNode<T>? head)
public Enumerator(DiagNode<T>? head)
{
_nextNode = head;
_currentItem = default;
Expand Down
2 changes: 1 addition & 1 deletion ...s/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ protected Instrument(Meter meter, string name, string? unit, string? description
/// <param name="tags">A span of key-value pair tags associated with the measurement.</param>
protected void RecordMeasurement(T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags)
{
LinkedListNode<ListenerSubscription>? current = _subscriptions.First;
DiagNode<ListenerSubscription>? current = _subscriptions.First;
while (current is not null)
{
current.Value.Listener.NotifyMeasurement(this, measurement, tags, current.Value.State);
Expand Down
14 changes: 9 additions & 5 deletions ...ibraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ namespace System.Diagnostics.Metrics
#endif
public abstract class Instrument
{
internal static KeyValuePair<string, object?>[] EmptyTags { get; } = Array.Empty<KeyValuePair<string, object?>>();
#if NO_ARRAY_EMPTY_SUPPORT
internal static KeyValuePair<string, object?>[] EmptyTags { get; } = new KeyValuePair<string, object?>[0];
#else
internal static KeyValuePair<string, object?>[] EmptyTags => Array.Empty<KeyValuePair<string, object?>>();
#endif // NO_ARRAY_EMPTY_SUPPORT

// The SyncObject is used to synchronize the following operations:
// - Instrument.Publish()
Expand All @@ -26,8 +30,8 @@ public abstract class Instrument
internal static object SyncObject { get; } = new object();

// We use LikedList here so we don't have to take any lock while iterating over the list as we always hold on a node which be either valid or null.
// LinkedList is thread safe for Add and Remove operations.
internal LinkedList<ListenerSubscription> _subscriptions = new LinkedList<ListenerSubscription>();
// DiagLinkedList is thread safe for Add and Remove operations.
internal readonly DiagLinkedList<ListenerSubscription> _subscriptions = new DiagLinkedList<ListenerSubscription>();

/// <summary>
/// Protected constructor to initialize the common instrument properties like the meter, name, description, and unit.
Expand Down Expand Up @@ -113,7 +117,7 @@ protected void Publish()
// NotifyForUnpublishedInstrument is called from Meter.Dispose()
internal void NotifyForUnpublishedInstrument()
{
LinkedListNode<ListenerSubscription>? current = _subscriptions.First;
DiagNode<ListenerSubscription>? current = _subscriptions.First;
while (current is not null)
{
current.Value.Listener.DisableMeasurementEvents(this);
Expand Down Expand Up @@ -160,7 +164,7 @@ internal virtual void Observe(MeterListener listener)

internal object? GetSubscriptionState(MeterListener listener)
{
LinkedListNode<ListenerSubscription>? current = _subscriptions.First;
DiagNode<ListenerSubscription>? current = _subscriptions.First;
while (current is not null)
{
if (object.ReferenceEquals(listener, current.Value.Listener))
Expand Down
Loading