From d2512294fdc95208e8c294b968060b2e044c8081 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 24 May 2023 18:21:29 -0700 Subject: [PATCH 1/6] Address Metrics APIs issues --- .../System/Diagnostics/DiagnosticsHelper.cs | 219 ++++++++++++++++-- ...oft.Extensions.Diagnostics.Abstractions.cs | 4 + .../src/Metrics/MeterFactoryExtensions.cs | 9 +- .../ref/Microsoft.Extensions.Diagnostics.cs | 4 - .../src/Metrics/DefaultMeterFactory.cs | 36 ++- .../src/Resources/Strings.resx | 63 +++++ .../tests/MetricsTests.cs | 76 ++++++ ...System.Diagnostics.DiagnosticSource.csproj | 1 - .../System/Diagnostics/Metrics/Instrument.cs | 6 +- .../src/System/Diagnostics/Metrics/Meter.cs | 8 +- .../tests/MetricsTests.cs | 57 ++++- 11 files changed, 434 insertions(+), 49 deletions(-) rename src/libraries/{Microsoft.Extensions.Diagnostics => Microsoft.Extensions.Diagnostics.Abstractions}/src/Metrics/MeterFactoryExtensions.cs (77%) create mode 100644 src/libraries/Microsoft.Extensions.Diagnostics/src/Resources/Strings.resx diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index 6d018effde82ae..47de977e7bad29 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -7,35 +7,118 @@ namespace System.Diagnostics { internal static class DiagnosticsHelper { - internal static bool CompareTags(IEnumerable>? tags1, IEnumerable>? tags2) + // This is similar to System.Linq ToArray. We are not using System.Linq here to avoid the dependency. + internal static KeyValuePair[]? ToArray(IEnumerable>? tags) { - if (tags1 == tags2) + if (tags is null) + { + return null; + } + + KeyValuePair[]? array = null; + if (tags is ICollection> tagsCol) + { + array = new KeyValuePair[tagsCol.Count]; + if (tagsCol is IList> secondList) + { + for (int i = 0; i < tagsCol.Count; i++) + { + array[i] = secondList[i]; + } + + return array; + } + } + + if (array is null) + { + int count = 0; + using (IEnumerator> enumerator = tags.GetEnumerator()) + { + while (enumerator.MoveNext()) + { + count++; + } + } + + array = new KeyValuePair[count]; + } + + Debug.Assert(array is not null); + + int index = 0; + using (IEnumerator> enumerator = tags.GetEnumerator()) + { + while (enumerator.MoveNext()) + { + array[index++] = enumerator.Current; + } + } + + return array; + } + + /// + /// Compares two tag collections for equality. + /// + /// The first collection of tags. it has to be a sorted array + /// The second collection of tags. This one doesn't have to be sorted nor be specific collection type + /// True if the two collections are equal, false otherwise + /// + /// This method is used to compare two collections of tags for equality. The first collection is expected to be a sorted array + /// of tags. The second collection can be any collection of tags. + /// we avoid the allocation of a new array by using the second collection as is and not converting it to an array. the reason + /// is we call this every time we try to create a meter or instrument and we don't want to allocate a new array every time. + /// + internal static bool CompareTags(KeyValuePair[]? sortedTags, IEnumerable>? tags2) + { + if (sortedTags == tags2) { return true; } - if (tags1 is null || tags2 is null) + if (sortedTags is null || tags2 is null) { return false; } - if (tags1 is ICollection> firstCol && tags2 is ICollection> secondCol) + // creating with 2 longs which can initially handle 2 * 64 = 128 tags + BitMapper bitMapper = new BitMapper(stackalloc ulong[2], true); + + int count = sortedTags.Length; + if (tags2 is ICollection> tagsCol) { - int count = firstCol.Count; - if (count != secondCol.Count) + if (tagsCol.Count != count) { return false; } - if (firstCol is IList> firstList && secondCol is IList> secondList) + if (tagsCol is IList> secondList) { for (int i = 0; i < count; i++) { - KeyValuePair pair1 = firstList[i]; - KeyValuePair pair2 = secondList[i]; - if (pair1.Key != pair2.Key || !object.Equals(pair1.Value, pair2.Value)) + KeyValuePair pair = secondList[i]; + + for (int j = 0; j < count; j++) { - return false; + if (bitMapper.IsSet(j)) + { + continue; + } + + KeyValuePair pair1 = sortedTags[j]; + + int compareResult = string.CompareOrdinal(pair.Key, pair1.Key); + if (compareResult == 0 && object.Equals(pair.Value, pair1.Value)) + { + bitMapper.SetBit(j); + break; + } + + if (compareResult < 0 || j == count - 1) + { + return false; + } } } @@ -43,26 +126,122 @@ internal static bool CompareTags(IEnumerable>? tag } } - using (IEnumerator> e1 = tags1.GetEnumerator()) - using (IEnumerator> e2 = tags2.GetEnumerator()) + int listCount = 0; + using (IEnumerator> enumerator = tags2.GetEnumerator()) { - while (e1.MoveNext()) + while (enumerator.MoveNext()) { - KeyValuePair pair1 = e1.Current; - if (!e2.MoveNext()) + listCount++; + if (listCount > sortedTags.Length) { return false; } - KeyValuePair pair2 = e2.Current; - if (pair1.Key != pair2.Key || !object.Equals(pair1.Value, pair2.Value)) + KeyValuePair pair = enumerator.Current; + for (int j = 0; j < count; j++) { - return false; + if (bitMapper.IsSet(j)) + { + continue; + } + + KeyValuePair pair1 = sortedTags[j]; + + int compareResult = string.CompareOrdinal(pair.Key, pair1.Key); + if (compareResult == 0 && object.Equals(pair.Value, pair1.Value)) + { + bitMapper.SetBit(j); + break; + } + + if (compareResult < 0 || j == count - 1) + { + return false; + } } } - return !e2.MoveNext(); + return listCount == sortedTags.Length; + } + } + } + + internal ref struct BitMapper + { + private int _maxIndex; + private Span _bitMap; + + public BitMapper(Span bitMap, bool zeroInitialize = false) + { + _bitMap = bitMap; + _maxIndex = bitMap.Length * sizeof(long) * 8; + + if (zeroInitialize) + { + for (int i = 0; i < _bitMap.Length; i++) + { + _bitMap[i] = 0; + } + } + } + + public int MaxIndex => _maxIndex; + + private void Expand(int index) + { + if (_maxIndex > index) + { + return; } + + int newMax = (index / sizeof(long)) + 10; + + Span newBitMap = new ulong[newMax]; + _bitMap.CopyTo(newBitMap); + _bitMap = newBitMap; + _maxIndex = newMax * sizeof(long); + } + + private static void GetIndexAndMask(int index, out int bitIndex, out ulong mask) + { + bitIndex = index >> sizeof(long); + int bit = index & (sizeof(long) - 1); + mask = 1UL << bit; + } + + public bool SetBit(int index) + { + if (index < 0) + { + throw new OutOfMemoryException(nameof(index)); + } + + if (index >= _maxIndex) + { + Expand(index); + } + + GetIndexAndMask(index, out int bitIndex, out ulong mask); + ulong value = _bitMap[bitIndex]; + _bitMap[bitIndex] = value | mask; + return true; + } + + public bool IsSet(int index) + { + if (index < 0) + { + throw new OutOfMemoryException(nameof(index)); + } + + if (index >= _maxIndex) + { + return false; + } + + GetIndexAndMask(index, out int bitIndex, out ulong mask); + ulong value = _bitMap[bitIndex]; + return ((value & mask) != 0); } } } diff --git a/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/ref/Microsoft.Extensions.Diagnostics.Abstractions.cs b/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/ref/Microsoft.Extensions.Diagnostics.Abstractions.cs index 121f454cc12653..38530051d412f6 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/ref/Microsoft.Extensions.Diagnostics.Abstractions.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/ref/Microsoft.Extensions.Diagnostics.Abstractions.cs @@ -7,4 +7,8 @@ public interface IMeterFactory : System.IDisposable { System.Diagnostics.Metrics.Meter Create(System.Diagnostics.Metrics.MeterOptions options); } + public static class MeterFactoryExtensions + { + public static System.Diagnostics.Metrics.Meter Create(this Microsoft.Extensions.Diagnostics.Metrics.IMeterFactory meterFactory, string name, string? version = null, System.Collections.Generic.IEnumerable>? tags = null) { return null!; } + } } \ No newline at end of file diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/MeterFactoryExtensions.cs b/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/src/Metrics/MeterFactoryExtensions.cs similarity index 77% rename from src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/MeterFactoryExtensions.cs rename to src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/src/Metrics/MeterFactoryExtensions.cs index ad1e82e6f22210..5d744f92e6dc92 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/MeterFactoryExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/src/Metrics/MeterFactoryExtensions.cs @@ -13,15 +13,14 @@ namespace Microsoft.Extensions.Diagnostics.Metrics public static class MeterFactoryExtensions { /// - /// Creates a with the specified , , , and . + /// Creates a with the specified , , and . /// /// The to use to create the . /// The name of the . /// The version of the . /// The tags to associate with the . - /// The scope to associate with the . - /// A with the specified , , , and . - public static Meter Create(this IMeterFactory meterFactory, string name, string? version = null, IEnumerable>? tags = null, object? scope = null) + /// A with the specified , , and . + public static Meter Create(this IMeterFactory meterFactory, string name, string? version = null, IEnumerable>? tags = null) { if (meterFactory is null) { @@ -32,7 +31,7 @@ public static Meter Create(this IMeterFactory meterFactory, string name, string? { Version = version, Tags = tags, - Scope = scope + Scope = meterFactory }); } } diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/ref/Microsoft.Extensions.Diagnostics.cs b/src/libraries/Microsoft.Extensions.Diagnostics/ref/Microsoft.Extensions.Diagnostics.cs index 6e034a5ba4e1e9..52fa2fcf89e115 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/ref/Microsoft.Extensions.Diagnostics.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics/ref/Microsoft.Extensions.Diagnostics.cs @@ -7,8 +7,4 @@ public static class MetricsServiceExtensions { public static Microsoft.Extensions.DependencyInjection.IServiceCollection AddMetrics(this Microsoft.Extensions.DependencyInjection.IServiceCollection services) { return null!; } } - public static class MeterFactoryExtensions - { - public static System.Diagnostics.Metrics.Meter Create(this Microsoft.Extensions.Diagnostics.Metrics.IMeterFactory meterFactory, string name, string? version = null, System.Collections.Generic.IEnumerable>? tags = null, object? scope = null) { return null!; } - } } \ No newline at end of file diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs index c6ceaeeae9e2eb..2d4f929e0d6232 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs @@ -10,7 +10,7 @@ namespace Microsoft.Extensions.Diagnostics.Metrics { internal sealed class DefaultMeterFactory : IMeterFactory { - private readonly Dictionary> _cachedMeters = new(); + private readonly Dictionary> _cachedMeters = new(); private bool _disposed; public DefaultMeterFactory() { } @@ -22,6 +22,11 @@ public Meter Create(MeterOptions options) throw new ArgumentNullException(nameof(options)); } + if (options.Scope is not null && !object.ReferenceEquals(options.Scope, this)) + { + throw new InvalidOperationException(SR.InvalidScope); + } + Debug.Assert(options.Name is not null); lock (_cachedMeters) @@ -31,11 +36,11 @@ public Meter Create(MeterOptions options) throw new ObjectDisposedException(nameof(DefaultMeterFactory)); } - if (_cachedMeters.TryGetValue(options.Name, out List? meterList)) + if (_cachedMeters.TryGetValue(options.Name, out List? meterList)) { foreach (Meter meter in meterList) { - if (meter.Version == options.Version && DiagnosticsHelper.CompareTags(meter.Tags, options.Tags)) + if (meter.Version == options.Version && DiagnosticsHelper.CompareTags(meter.Tags as KeyValuePair[], options.Tags)) { return meter; } @@ -43,11 +48,11 @@ public Meter Create(MeterOptions options) } else { - meterList = new List(); + meterList = new List(); _cachedMeters.Add(options.Name, meterList); } - Meter m = new Meter(options.Name, options.Version, options.Tags, scope: this); + FactoryMeter m = new FactoryMeter(options.Name, options.Version, options.Tags, scope: this); meterList.Add(m); return m; } @@ -64,11 +69,11 @@ public void Dispose() _disposed = true; - foreach (List meterList in _cachedMeters.Values) + foreach (List meterList in _cachedMeters.Values) { - foreach (Meter meter in meterList) + foreach (FactoryMeter meter in meterList) { - meter.Dispose(); + meter.Release(); } } @@ -76,4 +81,19 @@ public void Dispose() } } } + + internal sealed class FactoryMeter : Meter + { + public FactoryMeter(string name, string? version, IEnumerable>? tags, object? scope) + : base(name, version, tags, scope) + { + } + + public void Release() => base.Dispose(true); // call the protected Dispose(bool) + + protected override void Dispose(bool disposing) + { + // no-op, disallow users from disposing of the meters created from the factory. + } + } } diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/src/Resources/Strings.resx b/src/libraries/Microsoft.Extensions.Diagnostics/src/Resources/Strings.resx new file mode 100644 index 00000000000000..ab0d5ca5a461b3 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Diagnostics/src/Resources/Strings.resx @@ -0,0 +1,63 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + text/microsoft-resx + + + 2.0 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + The meter factory does not allow a custom scope value when creating a meter. + + diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/tests/MetricsTests.cs b/src/libraries/Microsoft.Extensions.Diagnostics/tests/MetricsTests.cs index bb9e91d26cb673..cfda92c4388da9 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/tests/MetricsTests.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics/tests/MetricsTests.cs @@ -49,6 +49,47 @@ public void FactoryDITest() Assert.Same(meterFactory, meter3.Scope); Assert.NotSame(meter1, meter3); + + // + // Test meter creation with unordered tags + // + + object o = new object(); + TagList l1 = new TagList() { { "z", "a" }, { "y", "b" }, { "x", "c" }, { "w", o }, { "N", null }, { "c", "d" }, { "q", "d" }, { "c", null } }; + List> l2 = new List>() + { + new KeyValuePair("y", "b"), new KeyValuePair("c", null), new KeyValuePair("N", null), + new KeyValuePair("x", "c"), new KeyValuePair("w", o), new KeyValuePair("z", "a"), + new KeyValuePair("c", "d"), new KeyValuePair("q", "d") + }; + HashSet> l3 = new HashSet>() + { + new KeyValuePair("q", "d"), new KeyValuePair("c", null), new KeyValuePair("N", null), + new KeyValuePair("w", o), new KeyValuePair("c", "d"), new KeyValuePair("x", "c"), + new KeyValuePair("z", "a"), new KeyValuePair("y", "b") + }; + + Meter meter4 = meterFactory.Create("name4", "4", l1); + Meter meter5 = meterFactory.Create("name4", "4", l2); + Meter meter6 = meterFactory.Create("name4", "4", l3); + + Assert.Same(meter4, meter5); + Assert.Same(meter4, meter6); + + KeyValuePair[] t1 = meter4.Tags.ToArray(); + Assert.Equal(l1.Count, t1.Length); + t1[0] = new KeyValuePair(t1[0].Key, "newValue"); // change value of one item; + Meter meter7 = meterFactory.Create("name4", "4", t1); + Assert.NotSame(meter4, meter7); + + // + // Ensure the tags in the meter are sorted + // + t1 = meter4.Tags.ToArray(); + for (int i = 0; i < t1.Length - 1; i++) + { + Assert.True(string.Compare(t1[i].Key, t1[i + 1].Key, StringComparison.Ordinal) <= 0); + } } [Fact] @@ -60,6 +101,41 @@ public void NegativeTest() using IMeterFactory meterFactory = sp.GetRequiredService(); Assert.Throws(() => meterFactory.Create(name: null)); + Assert.Throws(() => meterFactory.Create(new MeterOptions("name") { Name = "SomeName", Scope = new object() })); + + Meter meter = meterFactory.Create(new MeterOptions("name") { Name = "SomeName", Scope = meterFactory }); + Assert.Equal(meterFactory, meter.Scope); + } + + [Fact] + public void MeterDisposeTest() + { + ServiceCollection services = new ServiceCollection(); + services.AddMetrics(); + var sp = services.BuildServiceProvider(); + IMeterFactory meterFactory = sp.GetRequiredService(); + + Meter meter = meterFactory.Create("DisposableMeter"); + + Counter counter = meter.CreateCounter("MyCounter"); + InstrumentRecorder recorder = new InstrumentRecorder(counter); + counter.Add(10); + Assert.Equal(1, recorder.GetMeasurements().Count()); + Assert.Equal(10, recorder.GetMeasurements().ElementAt(0).Value); + meter.Dispose(); // should be no-op + counter.Add(20); + Assert.Equal(2, recorder.GetMeasurements().Count()); + Assert.Equal(20, recorder.GetMeasurements().ElementAt(1).Value); + + meter.Dispose(); // dispose again, should be no-op too + counter.Add(30); + Assert.Equal(3, recorder.GetMeasurements().Count()); + Assert.Equal(30, recorder.GetMeasurements().ElementAt(2).Value); + + // Now dispose the factory, the meter should be disposed too + meterFactory.Dispose(); + counter.Add(40); // recorder shouldn't observe this value as the meter created this instrument is disposed + Assert.Equal(3, recorder.GetMeasurements().Count()); } [Fact] 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 17d2489283e0f4..235c490dda5516 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -126,7 +126,6 @@ System.Diagnostics.DiagnosticSource - diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs index be33619291e466..5a33933f324488 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; -using System.Linq; namespace System.Diagnostics.Metrics { @@ -54,8 +53,9 @@ protected Instrument(Meter meter, string name, string? unit, string? description Unit = unit; if (tags is not null) { - var tagsArray = tags.ToArray(); - // Array.Sort(tagsArray, (left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); + var tagsArray = DiagnosticsHelper.ToArray(tags); + Debug.Assert(tagsArray is not null); + Array.Sort(tagsArray, (left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); Tags = tagsArray; } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs index 6fe7ac17c93a70..5b28d833ef77ff 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Threading; namespace System.Diagnostics.Metrics @@ -72,8 +71,9 @@ private void Initialize(string name, string? version, IEnumerable string.Compare(left.Key, right.Key, StringComparison.Ordinal)); + var tagsArray = DiagnosticsHelper.ToArray(tags); + Debug.Assert(tagsArray is not null); + Array.Sort(tagsArray, (left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); Tags = tagsArray; } Scope = scope; @@ -462,7 +462,7 @@ protected virtual void Dispose(bool disposing) foreach (Instrument instrument in instrumentList) { if (instrument.GetType() == instrumentType && instrument.Unit == unit && - instrument.Description == description && DiagnosticsHelper.CompareTags(instrument.Tags, tags)) + instrument.Description == description && DiagnosticsHelper.CompareTags(instrument.Tags as KeyValuePair[], tags)) { return instrument; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs index aafbbedf2f8f4e..f6d86efe0f7727 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs @@ -1221,6 +1221,16 @@ public void TestMeterCreationWithOptions() Assert.Equal("8.0", meter8.Version); Assert.Equal(new[] { new KeyValuePair("Key8", "Value8") }, meter8.Tags); Assert.Equal("Scope8", meter8.Scope); + + // Test tags sorting order + TagList l = new TagList() { { "f", "a" }, { "d", "b" }, { "w", "b" }, { "h", new object() }, { "N", null }, { "a", "b" }, { "a", null } }; + using Meter meter9 = new Meter(new MeterOptions("TestMeterCreationWithOptions9") { Version = "8.0", Tags = l, Scope = "Scope8" }); + var insArray = meter9.Tags.ToArray(); + Assert.Equal(l.Count, insArray.Length); + for (int i = 0; i < insArray.Length - 1; i++) + { + Assert.True(string.Compare(insArray[i].Key, insArray[i + 1].Key, StringComparison.Ordinal) <= 0); + } }).Dispose(); } @@ -1297,6 +1307,38 @@ public void TestCachedInstruments() UpDownCounter upDownCounter3 = meter.CreateUpDownCounter("name", null, null, list3); Assert.False(object.ReferenceEquals(upDownCounter3, upDownCounter1)); + + // + // Test instrument creation with unordered tags + // + + object o = new object(); + TagList l1 = new TagList() { { "f", "a" }, { "d", "b" }, { "w", "b" }, { "h", o}, { "N", null }, { "a", "b" }, { "a", null } }; + List> l2 = new List>() + { + new KeyValuePair("w", "b"), new KeyValuePair("h", o), new KeyValuePair("a", null), + new KeyValuePair("d", "b"), new KeyValuePair("f", "a"), new KeyValuePair("N", null), + new KeyValuePair("a", "b") + }; + HashSet> l3 = new HashSet>() + { + new KeyValuePair("d", "b"), new KeyValuePair("f", "a"), new KeyValuePair("a", null), + new KeyValuePair("w", "b"), new KeyValuePair("h", o), new KeyValuePair("a", "b"), + new KeyValuePair("N", null) + }; + + Counter counter9 = meter.CreateCounter("name9", null, null, l1); + Counter counter10 = meter.CreateCounter("name9", null, null, l2); + Counter counter11 = meter.CreateCounter("name9", null, null, l3); + Assert.Same(counter9, counter10); + Assert.Same(counter9, counter11); + + KeyValuePair[] t1 = counter9.Tags.ToArray(); + Assert.Equal(l1.Count, t1.Length); + t1[0] = new KeyValuePair(t1[0].Key, "newValue"); // change value of one item; + Counter counter12 = meter.CreateCounter("name9", null, null, t1); + Assert.NotSame(counter9, counter12); + }).Dispose(); } @@ -1342,6 +1384,17 @@ public void TestInstrumentCreationWithTags() Instrument ins12 = meter.CreateObservableGauge("ObservableUpDownCounter3", () => new Measurement[] { new Measurement(3) }, null, null, new TagList() { { "oudc3", "oudc-v3" } }); Assert.Equal(new[] { new KeyValuePair("oudc3", "oudc-v3") }, ins12.Tags); + // Test tags sorting order + + TagList l = new TagList() { { "z", "a" }, { "y", "b" }, { "x", "b" }, { "m", new object() }, { "N", null }, { "a", "b" }, { "a", null } }; + Instrument ins13 = meter.CreateCounter("counter", null, null, l); + var insArray = ins13.Tags.ToArray(); + Assert.Equal(l.Count, insArray.Length); + for (int i = 0; i < insArray.Length - 1; i++) + { + Assert.True(string.Compare(insArray[i].Key, insArray[i + 1].Key, StringComparison.Ordinal) <= 0); + } + }).Dispose(); } @@ -1454,10 +1507,6 @@ public void TestInstrumentRecorder() Assert.True(recorder7.GetMeasurements().Same(new Measurement[] { measurementWith10Value, measurementWith10Value })); Assert.True(recorder7.GetMeasurements(true).Same(new Measurement[] { measurementWith10Value, measurementWith10Value, measurementWith10Value })); Assert.True(recorder7.GetMeasurements().Same(new Measurement[] { measurementWith10Value })); - // Assert.Equal(new Measurement[] { measurementWith10Value, measurementWith10Value }, recorder7.GetMeasurements()); - // Assert.Equal(new Measurement[] { measurementWith10Value, measurementWith10Value, measurementWith10Value }, recorder7.GetMeasurements(clear: true)); - // Assert.Equal(new Measurement[] { measurementWith10Value }, recorder7.GetMeasurements()); - }).Dispose(); } From f47737442bacb56c996ae6fca8a87c7d34081262 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 24 May 2023 19:47:09 -0700 Subject: [PATCH 2/6] Address the feedback --- .../System/Diagnostics/DiagnosticsHelper.cs | 61 ++----------------- .../src/Metrics/DefaultMeterFactory.cs | 2 +- .../System/Diagnostics/Metrics/Instrument.cs | 7 +-- .../src/System/Diagnostics/Metrics/Meter.cs | 9 ++- 4 files changed, 13 insertions(+), 66 deletions(-) diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index 47de977e7bad29..dd1767f5b43b54 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -7,61 +7,10 @@ namespace System.Diagnostics { internal static class DiagnosticsHelper { - // This is similar to System.Linq ToArray. We are not using System.Linq here to avoid the dependency. - internal static KeyValuePair[]? ToArray(IEnumerable>? tags) - { - if (tags is null) - { - return null; - } - - KeyValuePair[]? array = null; - if (tags is ICollection> tagsCol) - { - array = new KeyValuePair[tagsCol.Count]; - if (tagsCol is IList> secondList) - { - for (int i = 0; i < tagsCol.Count; i++) - { - array[i] = secondList[i]; - } - - return array; - } - } - - if (array is null) - { - int count = 0; - using (IEnumerator> enumerator = tags.GetEnumerator()) - { - while (enumerator.MoveNext()) - { - count++; - } - } - - array = new KeyValuePair[count]; - } - - Debug.Assert(array is not null); - - int index = 0; - using (IEnumerator> enumerator = tags.GetEnumerator()) - { - while (enumerator.MoveNext()) - { - array[index++] = enumerator.Current; - } - } - - return array; - } - /// /// Compares two tag collections for equality. /// - /// The first collection of tags. it has to be a sorted array + /// The first collection of tags. it has to be a sorted List /// The second collection of tags. This one doesn't have to be sorted nor be specific collection type /// True if the two collections are equal, false otherwise /// @@ -70,7 +19,7 @@ internal static class DiagnosticsHelper /// we avoid the allocation of a new array by using the second collection as is and not converting it to an array. the reason /// is we call this every time we try to create a meter or instrument and we don't want to allocate a new array every time. /// - internal static bool CompareTags(KeyValuePair[]? sortedTags, IEnumerable>? tags2) + internal static bool CompareTags(List>? sortedTags, IEnumerable>? tags2) { if (sortedTags == tags2) { @@ -85,7 +34,7 @@ internal static bool CompareTags(KeyValuePair[]? sortedTags, IE // creating with 2 longs which can initially handle 2 * 64 = 128 tags BitMapper bitMapper = new BitMapper(stackalloc ulong[2], true); - int count = sortedTags.Length; + int count = sortedTags.Count; if (tags2 is ICollection> tagsCol) { if (tagsCol.Count != count) @@ -132,7 +81,7 @@ internal static bool CompareTags(KeyValuePair[]? sortedTags, IE while (enumerator.MoveNext()) { listCount++; - if (listCount > sortedTags.Length) + if (listCount > sortedTags.Count) { return false; } @@ -161,7 +110,7 @@ internal static bool CompareTags(KeyValuePair[]? sortedTags, IE } } - return listCount == sortedTags.Length; + return listCount == sortedTags.Count; } } } diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs index 2d4f929e0d6232..c631e54c641215 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs @@ -40,7 +40,7 @@ public Meter Create(MeterOptions options) { foreach (Meter meter in meterList) { - if (meter.Version == options.Version && DiagnosticsHelper.CompareTags(meter.Tags as KeyValuePair[], options.Tags)) + if (meter.Version == options.Version && DiagnosticsHelper.CompareTags(meter.Tags as List>, options.Tags)) { return meter; } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs index 5a33933f324488..0cb50f523f0282 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs @@ -53,10 +53,9 @@ protected Instrument(Meter meter, string name, string? unit, string? description Unit = unit; if (tags is not null) { - var tagsArray = DiagnosticsHelper.ToArray(tags); - Debug.Assert(tagsArray is not null); - Array.Sort(tagsArray, (left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); - Tags = tagsArray; + var tagList = new List>(tags); + tagList.Sort((left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); + Tags = tagList; } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs index 5b28d833ef77ff..a5722aa7b6c4dc 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Meter.cs @@ -71,10 +71,9 @@ private void Initialize(string name, string? version, IEnumerable string.Compare(left.Key, right.Key, StringComparison.Ordinal)); - Tags = tagsArray; + var tagList = new List>(tags); + tagList.Sort((left, right) => string.Compare(left.Key, right.Key, StringComparison.Ordinal)); + Tags = tagList; } Scope = scope; @@ -462,7 +461,7 @@ protected virtual void Dispose(bool disposing) foreach (Instrument instrument in instrumentList) { if (instrument.GetType() == instrumentType && instrument.Unit == unit && - instrument.Description == description && DiagnosticsHelper.CompareTags(instrument.Tags as KeyValuePair[], tags)) + instrument.Description == description && DiagnosticsHelper.CompareTags(instrument.Tags as List>, tags)) { return instrument; } From 69e5efebe9b4a29f78224df68676f085701b21a9 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 25 May 2023 11:48:45 -0700 Subject: [PATCH 3/6] Fix the correct thrown exception name. --- .../Common/src/System/Diagnostics/DiagnosticsHelper.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index dd1767f5b43b54..39e75abb466f97 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -162,7 +162,7 @@ public bool SetBit(int index) { if (index < 0) { - throw new OutOfMemoryException(nameof(index)); + throw new ArgumentOutOfRangeException (nameof(index)); } if (index >= _maxIndex) @@ -180,7 +180,7 @@ public bool IsSet(int index) { if (index < 0) { - throw new OutOfMemoryException(nameof(index)); + throw new ArgumentOutOfRangeException(nameof(index)); } if (index >= _maxIndex) From ea3b81d90b0878bba99a8938fdffcfb1adcce38a Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 26 May 2023 11:00:30 -0700 Subject: [PATCH 4/6] Address the feedback --- .../System/Diagnostics/DiagnosticsHelper.cs | 47 ++++--------------- .../Tasks/TimeProviderTaskExtensions.cs | 2 +- .../src/Metrics/DefaultMeterFactory.cs | 4 ++ 3 files changed, 14 insertions(+), 39 deletions(-) diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index 39e75abb466f97..9c4c9b2cc16d3d 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -31,10 +31,10 @@ internal static bool CompareTags(List>? sortedTags return false; } - // creating with 2 longs which can initially handle 2 * 64 = 128 tags - BitMapper bitMapper = new BitMapper(stackalloc ulong[2], true); - int count = sortedTags.Count; + int size = count / (sizeof(long) * 8) + 1; + BitMapper bitMapper = new BitMapper(size <= 100 ? stackalloc ulong[size] : new ulong[size]); + if (tags2 is ICollection> tagsCol) { if (tagsCol.Count != count) @@ -120,55 +120,26 @@ internal ref struct BitMapper private int _maxIndex; private Span _bitMap; - public BitMapper(Span bitMap, bool zeroInitialize = false) + public BitMapper(Span bitMap) { _bitMap = bitMap; + _bitMap.Clear(); _maxIndex = bitMap.Length * sizeof(long) * 8; - - if (zeroInitialize) - { - for (int i = 0; i < _bitMap.Length; i++) - { - _bitMap[i] = 0; - } - } } public int MaxIndex => _maxIndex; - private void Expand(int index) - { - if (_maxIndex > index) - { - return; - } - - int newMax = (index / sizeof(long)) + 10; - - Span newBitMap = new ulong[newMax]; - _bitMap.CopyTo(newBitMap); - _bitMap = newBitMap; - _maxIndex = newMax * sizeof(long); - } - private static void GetIndexAndMask(int index, out int bitIndex, out ulong mask) { - bitIndex = index >> sizeof(long); - int bit = index & (sizeof(long) - 1); + bitIndex = index >> 6; + int bit = index & (sizeof(long) * 8 - 1); mask = 1UL << bit; } public bool SetBit(int index) { - if (index < 0) - { - throw new ArgumentOutOfRangeException (nameof(index)); - } - - if (index >= _maxIndex) - { - Expand(index); - } + Debug.Assert(index >= 0); + Debug.Assert(index < _maxIndex); GetIndexAndMask(index, out int bitIndex, out ulong mask); ulong value = _bitMap[bitIndex]; diff --git a/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs b/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs index 9f8ba325f1d143..e410f4a5c9d32b 100644 --- a/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs +++ b/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs @@ -96,7 +96,7 @@ public static Task Delay(this TimeProvider timeProvider, TimeSpan delay, Cancell // There are race conditions where the timer fires after we have attached the cancellation callback but before the // registration is stored in state.Registration, or where cancellation is requested prior to the registration being - // stored into state.Registration, or where the timer could fire after it's been createdbut before it's been stored + // stored into state.Registration, or where the timer could fire after it's been created but before it's been stored // in state.Timer. In such cases, the cancellation registration and/or the Timer might be stored into state after the // callbacks and thus left undisposed. So, we do a subsequent check here. If the task isn't completed by this point, // then the callbacks won't have called TrySetResult (the callbacks invoke TrySetResult before disposing of the fields), diff --git a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs index c631e54c641215..dd1be2d1512d3a 100644 --- a/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs +++ b/src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DefaultMeterFactory.cs @@ -52,7 +52,11 @@ public Meter Create(MeterOptions options) _cachedMeters.Add(options.Name, meterList); } + object? scope = options.Scope; + options.Scope = this; FactoryMeter m = new FactoryMeter(options.Name, options.Version, options.Tags, scope: this); + options.Scope = scope; + meterList.Add(m); return m; } From 587408c3a510cac3cd9d6b1c1e14fcf8cc9bffc6 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 26 May 2023 11:03:46 -0700 Subject: [PATCH 5/6] Address the feedback --- .../Common/src/System/Diagnostics/DiagnosticsHelper.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index 9c4c9b2cc16d3d..c2088cafbea0f4 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -149,15 +149,9 @@ public bool SetBit(int index) public bool IsSet(int index) { - if (index < 0) - { - throw new ArgumentOutOfRangeException(nameof(index)); - } + Debug.Assert(index >= 0); + Debug.Assert(index < _maxIndex); - if (index >= _maxIndex) - { - return false; - } GetIndexAndMask(index, out int bitIndex, out ulong mask); ulong value = _bitMap[bitIndex]; From 14e3331c046864352b47171ab68ae654b3f87d6f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 26 May 2023 11:04:25 -0700 Subject: [PATCH 6/6] remove empty line --- src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs index c2088cafbea0f4..18f10c0d520d87 100644 --- a/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs +++ b/src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs @@ -152,7 +152,6 @@ public bool IsSet(int index) Debug.Assert(index >= 0); Debug.Assert(index < _maxIndex); - GetIndexAndMask(index, out int bitIndex, out ulong mask); ulong value = _bitMap[bitIndex]; return ((value & mask) != 0);