Metric name disambiguation

src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs

@@ -37,20 +37,64 @@ internal record struct ProviderAndCounter(string ProviderName, string CounterNam
     internal static class TraceEventExtensions
     {
         private static Dictionary<ProviderAndCounter, CounterMetadata> counterMetadataByName = new();
+        private static Dictionary<int, CounterMetadata> counterMetadataById = new();
         private static HashSet<string> inactiveSharedSessions = new(StringComparer.OrdinalIgnoreCase);
 
-        // This assumes uniqueness of provider/counter combinations;
-        // this is currently a limitation (see https://github.com/dotnet/runtime/issues/93097 and https://github.com/dotnet/runtime/issues/93767)
-        public static CounterMetadata GetCounterMetadata(string providerName, string counterName, string meterTags = null, string instrumentTags = null, string scopeHash = null)
+        private static CounterMetadata AddCounterMetadata(string providerName, string counterName, int? id, string meterTags, string instrumentTags, string scopeHash)
         {
+            CounterMetadata metadata;
+            if (id.HasValue && counterMetadataById.TryGetValue(id.Value, out metadata))
+            {
+                return metadata;
+            }
+
+            // Its possible that we previously indexed this counter by name but it didn't have an ID at that point because we weren't
+            // listening to it then.
+            // Its also possible that we previously indexed a counter with the same name as this one but with different tags or scope hash.
             ProviderAndCounter providerAndCounter = new(providerName, counterName);
-            if (counterMetadataByName.TryGetValue(providerAndCounter, out CounterMetadata provider))
+            if (counterMetadataByName.TryGetValue(providerAndCounter, out metadata))
             {
-                return provider;
+                // we found a counter that matches the name, but it might not match everything
+                if (metadata.MeterTags == meterTags && metadata.InstrumentTags == instrumentTags && metadata.ScopeHash == scopeHash)
+                {
+                    // add the ID index if it didn't exist before
+                    if (id.HasValue)
+                    {
+                        counterMetadataById.TryAdd(id.Value, metadata);
+                    }
+                    return metadata;
+                }
             }
 
-            counterMetadataByName.Add(providerAndCounter, new CounterMetadata(providerName, counterName, meterTags, instrumentTags, scopeHash));
-            return counterMetadataByName[providerAndCounter];
+            // no pre-existing counter metadata was found, create a new one
+            metadata = new CounterMetadata(providerName, counterName, meterTags, instrumentTags, scopeHash);
+            if (id.HasValue)
+            {
+                counterMetadataById.TryAdd(id.Value, metadata);
+            }
+            counterMetadataByName.TryAdd(providerAndCounter, metadata);
+            return metadata;
+        }
+
+        private static CounterMetadata GetCounterMetadata(string providerName, string counterName, int? id)
+        {
+            // Lookup by ID is preferred because it eliminates ambiguity in the case of duplicate provider/counter names.
+            // IDs are present starting in MetricsEventSource 9.0.
+            // Duplicate named providers/counters might still have different tags or scope hashes
+            CounterMetadata metadata;
+            if (id.HasValue && counterMetadataById.TryGetValue(id.Value, out metadata))
+            {
+                return metadata;
+            }
+            ProviderAndCounter providerAndCounter = new(providerName, counterName);
+            if (counterMetadataByName.TryGetValue(providerAndCounter, out metadata))
+            {
+                return metadata;
+            }
+
+            // For EventCounter based events we expect to fall through here the first time a new counter is observed
+            // For MetricsEventSource events we should never reach here unless the BeginInstrumentRecording event was dropped.
+            return AddCounterMetadata(providerName, counterName, id, null, null, null);
         }
 
         public static bool TryGetCounterPayload(this TraceEvent traceEvent, CounterConfiguration counterConfiguration, out ICounterPayload payload)
@@ -185,22 +229,29 @@ private static void HandleGauge(TraceEvent obj, CounterConfiguration counterConf
             string unit = (string)obj.PayloadValue(4);
             string tags = (string)obj.PayloadValue(5);
             string lastValueText = (string)obj.PayloadValue(6);
+            int? id = null;
+
+            if (obj.Version >= 2)
+            {
+                id = (int)obj.PayloadValue(7);
+            }
 
             if (!counterConfiguration.CounterFilter.IsIncluded(meterName, instrumentName))
             {
                 return;
             }
 
+            CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
             // the value might be an empty string indicating no measurement was provided this collection interval
             if (double.TryParse(lastValueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double lastValue))
             {
-                payload = new GaugePayload(GetCounterMetadata(meterName, instrumentName), null, unit, tags, lastValue, obj.TimeStamp);
+                payload = new GaugePayload(metadata, null, unit, tags, lastValue, obj.TimeStamp);
             }
             else
             {
                 // for observable instruments we assume the lack of data is meaningful and remove it from the UI
                 // this happens when the Gauge callback function throws an exception.
-                payload = new CounterEndedPayload(GetCounterMetadata(meterName, instrumentName), obj.TimeStamp);
+                payload = new CounterEndedPayload(metadata, obj.TimeStamp);
             }
         }
 
@@ -223,18 +274,25 @@ private static void HandleBeginInstrumentReporting(TraceEvent traceEvent, Counte
                 return;
             }
 
-            if (traceEvent.Version < 1)
+            string instrumentTags = null;
+            string meterTags = null;
+            string meterScopeHash = null;
+            int? instrumentID = null;
+
+            if (traceEvent.Version >= 1)
             {
-                payload = new BeginInstrumentReportingPayload(GetCounterMetadata(meterName, instrumentName), traceEvent.TimeStamp);
+                instrumentTags = (string)traceEvent.PayloadValue(7);
+                meterTags = (string)traceEvent.PayloadValue(8);
+                meterScopeHash = (string)traceEvent.PayloadValue(9);
             }
-            else
+            if (traceEvent.Version >= 2)
             {
-                string instrumentTags = (string)traceEvent.PayloadValue(7);
-                string meterTags = (string)traceEvent.PayloadValue(8);
-                string meterScopeHash = (string)traceEvent.PayloadValue(9);
-
-                payload = new BeginInstrumentReportingPayload(GetCounterMetadata(meterName, instrumentName, meterTags, instrumentTags, meterScopeHash), traceEvent.TimeStamp);
+                int id = (int)traceEvent.PayloadValue(10);
+                // ID zero is a sentinel value for MetricsEventSource events indicating no ID was provided because the instrument was not being listened to.
+                // Many different instruments may all share ID zero we don't want to index them by that ID.
+                instrumentID = (id != 0) ? id : null;
             }
+            payload = new BeginInstrumentReportingPayload(AddCounterMetadata(meterName, instrumentName, instrumentID, meterTags, instrumentTags, meterScopeHash), traceEvent.TimeStamp);
         }
 
         private static void HandleCounterRate(TraceEvent traceEvent, CounterConfiguration counterConfiguration, out ICounterPayload payload)
@@ -256,35 +314,40 @@ private static void HandleCounterRate(TraceEvent traceEvent, CounterConfiguratio
             string rateText = (string)traceEvent.PayloadValue(6);
             //Starting in .NET 8 we also publish the absolute value of these counters
             string absoluteValueText = null;
+            int? id = null;
             if (traceEvent.Version >= 1)
             {
                 absoluteValueText = (string)traceEvent.PayloadValue(7);
             }
+            if (traceEvent.Version >= 2)
+            {
+                id = (int)traceEvent.PayloadValue(8);
+            }
 
             if (!counterConfiguration.CounterFilter.IsIncluded(meterName, instrumentName))
             {
                 return;
             }
-
+            CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
             if (double.TryParse(rateText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double rate))
             {
                 if (absoluteValueText != null &&
                     counterConfiguration.UseCounterRateAndValuePayload &&
                     double.TryParse(absoluteValueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double value))
                 {
-                    payload = new CounterRateAndValuePayload(GetCounterMetadata(meterName, instrumentName), null, unit, tags, rate, value, traceEvent.TimeStamp);
+                    payload = new CounterRateAndValuePayload(metadata, null, unit, tags, rate, value, traceEvent.TimeStamp);
                 }
                 else
                 {
-                    payload = new RatePayload(GetCounterMetadata(meterName, instrumentName), null, unit, tags, rate, counterConfiguration.CounterFilter.DefaultIntervalSeconds, traceEvent.TimeStamp);
+                    payload = new RatePayload(metadata, null, unit, tags, rate, counterConfiguration.CounterFilter.DefaultIntervalSeconds, traceEvent.TimeStamp);
                 }
             }
             else
             {
                 // for observable instruments we assume the lack of data is meaningful and remove it from the UI
                 // this happens when the ObservableCounter callback function throws an exception
                 // or when the ObservableCounter doesn't include a measurement for a particular set of tag values.
-                payload = new CounterEndedPayload(GetCounterMetadata(meterName, instrumentName), traceEvent.TimeStamp);
+                payload = new CounterEndedPayload(metadata, traceEvent.TimeStamp);
             }
         }
 
@@ -306,24 +369,30 @@ private static void HandleUpDownCounterValue(TraceEvent traceEvent, CounterConfi
             string tags = (string)traceEvent.PayloadValue(5);
             //string rateText = (string)traceEvent.PayloadValue(6); // Not currently using rate for UpDownCounters.
             string valueText = (string)traceEvent.PayloadValue(7);
+            int? id = null;
+            if (traceEvent.Version >= 2)
+            {
+                id = (int)traceEvent.PayloadValue(8);
+            }
 
             if (!configuration.CounterFilter.IsIncluded(meterName, instrumentName))
             {
                 return;
             }
 
+            CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
             if (double.TryParse(valueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double value))
             {
                 // UpDownCounter reports the value, not the rate - this is different than how Counter behaves.
-                payload = new UpDownCounterPayload(GetCounterMetadata(meterName, instrumentName), null, unit, tags, value, traceEvent.TimeStamp);
+                payload = new UpDownCounterPayload(metadata, null, unit, tags, value, traceEvent.TimeStamp);
 
             }
             else
             {
                 // for observable instruments we assume the lack of data is meaningful and remove it from the UI
                 // this happens when the ObservableUpDownCounter callback function throws an exception
                 // or when the ObservableUpDownCounter doesn't include a measurement for a particular set of tag values.
-                payload = new CounterEndedPayload(GetCounterMetadata(meterName, instrumentName), traceEvent.TimeStamp);
+                payload = new CounterEndedPayload(metadata, traceEvent.TimeStamp);
             }
         }
 
@@ -344,6 +413,13 @@ private static void HandleHistogram(TraceEvent obj, CounterConfiguration configu
             string unit = (string)obj.PayloadValue(4);
             string tags = (string)obj.PayloadValue(5);
             string quantilesText = (string)obj.PayloadValue(6);
+            //int count - unused arg 7
+            //double sum - unused arg 8
+            int? id = null;
+            if (obj.Version >= 2)
+            {
+                id = (int)obj.PayloadValue(9);
+            }
 
             if (!configuration.CounterFilter.IsIncluded(meterName, instrumentName))
             {
@@ -352,8 +428,8 @@ private static void HandleHistogram(TraceEvent obj, CounterConfiguration configu
 
             //Note quantiles can be empty.
             IList<Quantile> quantiles = ParseQuantiles(quantilesText);
-
-            payload = new AggregatePercentilePayload(GetCounterMetadata(meterName, instrumentName), null, unit, tags, quantiles, obj.TimeStamp);
+            CounterMetadata metadata = GetCounterMetadata(meterName, instrumentName, id);
+            payload = new AggregatePercentilePayload(metadata, null, unit, tags, quantiles, obj.TimeStamp);
         }
 
         private static void HandleHistogramLimitReached(TraceEvent obj, CounterConfiguration configuration, out ICounterPayload payload)

src/tests/EventPipeTracee/DuplicateNameMetrics.cs

@@ -0,0 +1,50 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.Metrics;
+
+
+
+namespace EventPipeTracee
+{
+    // The constructor overloads for Meter and Counter that take tags were added in .NET 8.0
+    // so this class is only available when built against 8.0 and later
+#if NET8_0_OR_GREATER
+    internal sealed class DuplicateNameMetrics : IDisposable
+    {
+        private Meter _meter1;
+        private Meter _meter2;
+        private Counter<int> _counter1a;
+        private Counter<int> _counter1b;
+        private Counter<int> _counter2a;
+        private Counter<int> _counter2b;
+
+        public DuplicateNameMetrics()
+        {
+            _meter1 = new Meter("AmbiguousNameMeter", "", [new("MeterTag", "one")]);
+            _meter2 = new Meter("AmbiguousNameMeter", "", [new("MeterTag", "two")]);
+            _counter1a = _meter1.CreateCounter<int>("AmbiguousNameCounter", "", "", [new("InstrumentTag", "A")]);
+            _counter1b = _meter1.CreateCounter<int>("AmbiguousNameCounter", "", "", [new("InstrumentTag", "B")]);
+            _counter2a = _meter2.CreateCounter<int>("AmbiguousNameCounter", "", "", [new("InstrumentTag", "A")]);
+            _counter2b = _meter2.CreateCounter<int>("AmbiguousNameCounter", "", "", [new("InstrumentTag", "B")]);
+        }
+
+
+        public void IncrementCounter(int v = 1)
+        {
+            _counter1a.Add(v);
+            _counter1b.Add(v + 1);
+            _counter2a.Add(v + 2);
+            _counter2b.Add(v + 3);
+        }
+
+        public void Dispose()
+        {
+            _meter1?.Dispose();
+            _meter2?.Dispose();
+        }
+    }
+#endif
+}

src/tests/EventPipeTracee/EventPipeTracee.csproj

@@ -3,6 +3,7 @@
     <OutputType>Exe</OutputType>
     <TargetFramework Condition="'$(BuildProjectFramework)' != ''">$(BuildProjectFramework)</TargetFramework>
     <TargetFrameworks Condition="'$(BuildProjectFramework)' == ''">$(BuildTargetFrameworks)</TargetFrameworks>
+    <DefineConstants Condition="'$(TargetFramework)'!='net6.0' and '$(TargetFramework)'!='net7.0'">NET8_0_OR_GREATER</DefineConstants>
   </PropertyGroup>
 
   <ItemGroup>

src/tests/EventPipeTracee/Program.cs

@@ -32,8 +32,10 @@ public static async Task<int> Main(string[] args)
             string loggerCategory = args[1];
 
             bool diagMetrics = args.Any("DiagMetrics".Equals);
+            bool duplicateNameMetrics = args.Any("DuplicateNameMetrics".Equals);
 
             Console.WriteLine($"{pid} EventPipeTracee: DiagMetrics {diagMetrics}");
+            Console.WriteLine($"{pid} EventPipeTracee: DuplicateNameMetrics {duplicateNameMetrics}");
 
             Console.WriteLine($"{pid} EventPipeTracee: start process");
             Console.Out.Flush();
@@ -60,8 +62,6 @@ public static async Task<int> Main(string[] args)
             Console.WriteLine($"{pid} EventPipeTracee: {DateTime.UtcNow} Awaiting start");
             Console.Out.Flush();
 
-            using CustomMetrics metrics = diagMetrics ? new CustomMetrics() : null;
-
             // Wait for server to send something
             int input = pipeStream.ReadByte();
 
@@ -70,19 +70,26 @@ public static async Task<int> Main(string[] args)
 
             CancellationTokenSource recordMetricsCancellationTokenSource = new();
 
-            if (diagMetrics)
+            if (diagMetrics || duplicateNameMetrics)
             {
                 _ = Task.Run(async () => {
 
+                    using CustomMetrics metrics = diagMetrics ? new CustomMetrics() : null;
+#if NET8_0_OR_GREATER
+                    using DuplicateNameMetrics dupMetrics = duplicateNameMetrics ? new DuplicateNameMetrics() : null;
+#endif
                     // Recording a single value appeared to cause test flakiness due to a race
                     // condition with the timing of when dotnet-counters starts collecting and
                     // when these values are published. Publishing values repeatedly bypasses this problem.
                     while (!recordMetricsCancellationTokenSource.Token.IsCancellationRequested)
                     {
                         recordMetricsCancellationTokenSource.Token.ThrowIfCancellationRequested();
 
-                        metrics.IncrementCounter();
-                        metrics.RecordHistogram(10.0f);
+                        metrics?.IncrementCounter();
+                        metrics?.RecordHistogram(10.0f);
+#if NET8_0_OR_GREATER
+                        dupMetrics?.IncrementCounter();
+#endif
                         await Task.Delay(1000).ConfigureAwait(true);
                     }