[Feature branch changes, no review] Fixup counter apis

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

@@ -8,14 +8,8 @@
 
 namespace Microsoft.Diagnostics.Monitoring.EventPipe
 {
-    public class CounterPayload : ICounterPayload
+    internal class CounterPayload : ICounterPayload
     {
-#if NETSTANDARD
-        private static readonly IReadOnlyDictionary<string, string> Empty = new ReadOnlyDictionary<string, string>(new Dictionary<string, string>(0));
-#else
-        private static readonly IReadOnlyDictionary<string, string> Empty = System.Collections.Immutable.ImmutableDictionary<string, string>.Empty;
-#endif
-
         public CounterPayload(DateTime timestamp,
             string provider,
             string name,
@@ -24,7 +18,7 @@ public CounterPayload(DateTime timestamp,
             double value,
             CounterType counterType,
             float interval,
-            Dictionary<string, string> metadata)
+            string metadata)
         {
             Timestamp = timestamp;
             Name = name;
@@ -34,16 +28,16 @@ public CounterPayload(DateTime timestamp,
             CounterType = counterType;
             Provider = provider;
             Interval = interval;
-            Metadata = metadata ?? Empty;
+            Metadata = metadata;
             EventType = EventType.Gauge;
         }
 
         // Copied from dotnet-counters
-        public CounterPayload(string providerName, string name, string displayName, string displayUnits, Dictionary<string, string> metadata, double value, DateTime timestamp, string type, EventType eventType)
+        public CounterPayload(string providerName, string name, string displayName, string displayUnits, string metadata, double value, DateTime timestamp, string type, EventType eventType)
         {
             Provider = providerName;
             Name = name;
-            Metadata = metadata ?? Empty;
+            Metadata = metadata;
             Value = value;
             Timestamp = timestamp;
             CounterType = (CounterType)Enum.Parse(typeof(CounterType), type);
@@ -68,14 +62,14 @@ public CounterPayload(string providerName, string name, string displayName, stri
 
         public string Provider { get; }
 
-        public IReadOnlyDictionary<string, string> Metadata { get; }
+        public string Metadata { get; }
 
         public EventType EventType { get; set; }
     }
 
-    public class GaugePayload : CounterPayload
+    internal class GaugePayload : CounterPayload
     {
-        public GaugePayload(string providerName, string name, string displayName, string displayUnits, Dictionary<string, string> metadata, double value, DateTime timestamp) :
+        public GaugePayload(string providerName, string name, string displayName, string displayUnits, string metadata, double value, DateTime timestamp) :
             base(providerName, name, displayName, displayUnits, metadata, value, timestamp, "Metric", EventType.Gauge)
         {
             // In case these properties are not provided, set them to appropriate values.
@@ -84,9 +78,18 @@ public GaugePayload(string providerName, string name, string displayName, string
         }
     }
 
-    public class RatePayload : CounterPayload
+    internal class CounterEndedPayload : CounterPayload
+    {
+        public CounterEndedPayload(string providerName, string name, string displayName, DateTime timestamp)
+            : base(providerName, name, displayName, string.Empty, null, 0.0, timestamp, "Metric", EventType.CounterEnded)
+        {
+
+        }
+    }
+
+    internal class RatePayload : CounterPayload
     {
-        public RatePayload(string providerName, string name, string displayName, string displayUnits, Dictionary<string, string> metadata, double value, double intervalSecs, DateTime timestamp) :
+        public RatePayload(string providerName, string name, string displayName, string displayUnits, string metadata, double value, double intervalSecs, DateTime timestamp) :
             base(providerName, name, displayName, displayUnits, metadata, value, timestamp, "Rate", EventType.Rate)
         {
             // In case these properties are not provided, set them to appropriate values.
@@ -97,9 +100,9 @@ public RatePayload(string providerName, string name, string displayName, string
         }
     }
 
-    public class PercentilePayload : CounterPayload
+    internal class PercentilePayload : CounterPayload
     {
-        public PercentilePayload(string providerName, string name, string displayName, string displayUnits, Dictionary<string, string> metadata, double val, DateTime timestamp) :
+        public PercentilePayload(string providerName, string name, string displayName, string displayUnits, string metadata, double val, DateTime timestamp) :
             base(providerName, name, displayName, displayUnits, metadata, val, timestamp, "Metric", EventType.Histogram)
         {
             // In case these properties are not provided, set them to appropriate values.
@@ -108,10 +111,14 @@ public PercentilePayload(string providerName, string name, string displayName, s
         }
     }
 
-    public class ErrorPayload : CounterPayload
+    internal class ErrorPayload : CounterPayload
     {
-        public ErrorPayload(string providerName, string name, string displayName, string displayUnits, Dictionary<string, string> metadata, double val, DateTime timestamp, string errorMessage) :
-            base(providerName, name, displayName, displayUnits, metadata, val, timestamp, "Metric", EventType.Error)
+        public ErrorPayload(string errorMessage) : this(errorMessage, DateTime.UtcNow) 
+        {
+        }
+
+        public ErrorPayload(string errorMessage, DateTime timestamp) :
+            base(string.Empty, string.Empty, string.Empty, string.Empty, null, 0.0, timestamp, "Metric", EventType.Error)
         {
             ErrorMessage = errorMessage;
         }
@@ -120,11 +127,12 @@ public ErrorPayload(string providerName, string name, string displayName, string
     }
 
     // If keep this, should probably put it somewhere else
-    public enum EventType : int
+    internal enum EventType : int
     {
         Rate,
         Gauge,
         Histogram,
-        Error
+        Error,
+        CounterEnded
     }
 }

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

@@ -48,7 +48,7 @@ protected override MonitoringSourceConfiguration CreateConfiguration()
 
         protected override async Task OnEventSourceAvailable(EventPipeEventSource eventSource, Func<Task> stopSessionAsync, CancellationToken token)
         {
-            ExecuteCounterLoggerAction((metricLogger) => metricLogger.PipelineStarted());
+            await ExecuteCounterLoggerActionAsync((metricLogger) => metricLogger.PipelineStarted(token));
 
             eventSource.Dynamic.All += traceEvent =>
             {
@@ -77,7 +77,21 @@ protected override async Task OnEventSourceAvailable(EventPipeEventSource eventS
 
             await sourceCompletedTaskSource.Task;
 
-            ExecuteCounterLoggerAction((metricLogger) => metricLogger.PipelineStopped());
+            await ExecuteCounterLoggerActionAsync((metricLogger) => metricLogger.PipelineStopped(token));
+        }
+
+        private async Task ExecuteCounterLoggerActionAsync(Func<ICountersLogger, Task> action)
+        {
+            foreach (ICountersLogger logger in _loggers)
+            {
+                try
+                {
+                    await action(logger);
+                }
+                catch (ObjectDisposedException)
+                {
+                }
+            }
         }
 
         private void ExecuteCounterLoggerAction(Action<ICountersLogger> action)

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

@@ -17,7 +17,7 @@ public enum CounterType
         Rate
     }
 
-    public interface ICounterPayload
+    internal interface ICounterPayload
     {
         string Name { get; }
 
@@ -35,8 +35,8 @@ public interface ICounterPayload
 
         float Interval { get; }
 
-        IReadOnlyDictionary<string, string> Metadata { get; }
+        string Metadata { get; }
 
-        public EventType EventType { get; set; }
+        EventType EventType { get; set; }
     }
 }

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

@@ -2,14 +2,16 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.Threading;
+using System.Threading.Tasks;
+
 namespace Microsoft.Diagnostics.Monitoring.EventPipe
 {
     internal interface ICountersLogger
     {
-        //TODO Consider making these async.
-
         void Log(ICounterPayload counter);
-        void PipelineStarted();
-        void PipelineStopped();
+
+        Task PipelineStarted(CancellationToken token);
+        Task PipelineStopped(CancellationToken token);
     }
 }

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

@@ -24,7 +24,7 @@ public static bool TryGetCounterPayload(this TraceEvent traceEvent, CounterFilte
                 string series = payloadFields["Series"].ToString();
                 string counterName = payloadFields["Name"].ToString();
 
-                Dictionary<string, string> metadataDict = GetMetadata(payloadFields["Metadata"].ToString());
+                string metadata = payloadFields["Metadata"].ToString();
 
                 //CONSIDER
                 //Concurrent counter sessions do not each get a separate interval. Instead the payload
@@ -67,7 +67,7 @@ public static bool TryGetCounterPayload(this TraceEvent traceEvent, CounterFilte
                     value,
                     counterType,
                     intervalSec,
-                    metadataDict));
+                    metadata));
 
                 return true;
             }
@@ -152,8 +152,6 @@ private static void HandleGauge(TraceEvent obj, CounterFilter filter, string ses
             string tags = (string)obj.PayloadValue(5);
             string lastValueText = (string)obj.PayloadValue(6);
 
-            Dictionary<string, string> metadataDict = GetMetadata(tags);
-
             if (!filter.IsIncluded(meterName, instrumentName))
             {
                 return;
@@ -162,7 +160,16 @@ private static void HandleGauge(TraceEvent obj, CounterFilter filter, string ses
             // the value might be an empty string indicating no measurement was provided this collection interval
             if (double.TryParse(lastValueText, out double lastValue))
             {
-                payload = new GaugePayload(meterName, instrumentName, null, unit, metadataDict, lastValue, obj.TimeStamp);
+                payload = new GaugePayload(meterName, instrumentName, 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.
+
+                //TODO Can this occur for other meter types?
+                payload = new CounterEndedPayload(meterName, instrumentName, null, obj.TimeStamp);
+
             }
         }
 
@@ -184,11 +191,9 @@ private static void HandleCounterRate(TraceEvent traceEvent, CounterFilter filte
             string tags = (string)traceEvent.PayloadValue(5);
             string rateText = (string)traceEvent.PayloadValue(6);
 
-            Dictionary<string, string> metadataDict = GetMetadata(tags);
-
             if (double.TryParse(rateText, out double rate))
             {
-                payload = new RatePayload(meterName, instrumentName, null, unit, metadataDict, rate, 10, traceEvent.TimeStamp);
+                payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, 10, traceEvent.TimeStamp);
             }
         }
 
@@ -218,12 +223,13 @@ private static void HandleHistogram(TraceEvent obj, CounterFilter filter, string
             KeyValuePair<double, double>[] quantiles = ParseQuantiles(quantilesText);
             foreach ((double key, double val) in quantiles)
             {
-                Dictionary<string, string> metadataDict = GetMetadata(tags);
-                metadataDict.Add("quantile", key.ToString());
-                payload.Add(new PercentilePayload(meterName, instrumentName, null, unit, metadataDict, val, obj.TimeStamp));
+                string tagsWithQuantile = AppendQuantile(tags, FormattableString.Invariant($"quantile={key}"));
+                payload.Add(new PercentilePayload(meterName, instrumentName, null, unit, tagsWithQuantile, val, obj.TimeStamp));
             }
         }
 
+        private static string AppendQuantile(string tags, string quantile) => string.IsNullOrEmpty(tags) ? quantile : FormattableString.Invariant($"{tags},{quantile}");
+
         private static void HandleHistogramLimitReached(TraceEvent obj, string sessionId, out ICounterPayload payload)
         {
             payload = null;
@@ -237,7 +243,7 @@ private static void HandleHistogramLimitReached(TraceEvent obj, string sessionId
 
             string errorMessage = $"Warning: Histogram tracking limit reached. Not all data is being shown. The limit can be changed with maxHistograms but will use more memory in the target process.";
 
-            payload = new ErrorPayload(string.Empty, string.Empty, string.Empty, string.Empty, new(), 0, obj.TimeStamp, errorMessage);
+            payload = new ErrorPayload(errorMessage);
         }
 
         private static void HandleTimeSeriesLimitReached(TraceEvent obj, string sessionId, out ICounterPayload payload)
@@ -253,7 +259,7 @@ private static void HandleTimeSeriesLimitReached(TraceEvent obj, string sessionI
 
             string errorMessage = "Warning: Time series tracking limit reached. Not all data is being shown. The limit can be changed with maxTimeSeries but will use more memory in the target process.";
 
-            payload = new ErrorPayload(string.Empty, string.Empty, string.Empty, string.Empty, new(), 0, obj.TimeStamp, errorMessage);
+            payload = new ErrorPayload(errorMessage, obj.TimeStamp);
         }
 
         private static void HandleError(TraceEvent obj, string sessionId, out ICounterPayload payload)
@@ -269,7 +275,7 @@ private static void HandleError(TraceEvent obj, string sessionId, out ICounterPa
 
             string errorMessage = "Error reported from target process:" + Environment.NewLine + error;
 
-            payload = new ErrorPayload(string.Empty, string.Empty, string.Empty, string.Empty, new(), 0, obj.TimeStamp, errorMessage);
+            payload = new ErrorPayload(errorMessage, obj.TimeStamp);
         }
 
         private static void HandleMultipleSessionsNotSupportedError(TraceEvent obj, string sessionId, out ICounterPayload payload)
@@ -288,7 +294,7 @@ private static void HandleMultipleSessionsNotSupportedError(TraceEvent obj, stri
                 string errorMessage = "Error: Another metrics collection session is already in progress for the target process, perhaps from another tool? " + Environment.NewLine +
                 "Concurrent sessions are not supported.";
 
-                payload = new ErrorPayload(string.Empty, string.Empty, string.Empty, string.Empty, new(), 0, obj.TimeStamp, errorMessage);
+                payload = new ErrorPayload(errorMessage, obj.TimeStamp);
             }
         }
 
@@ -307,14 +313,14 @@ private static void HandleObservableInstrumentCallbackError(TraceEvent obj, stri
             string errorMessage = "Exception thrown from an observable instrument callback in the target process:" + Environment.NewLine +
                 error;
 
-            payload = new ErrorPayload(string.Empty, string.Empty, string.Empty, string.Empty, new(), 0, obj.TimeStamp, errorMessage);
+            payload = new ErrorPayload(errorMessage, obj.TimeStamp);
         }
 
         //The metadata payload is formatted as a string of comma separated key:value pairs.
         //This limitation means that metadata values cannot include commas; otherwise, the
         //metadata will be parsed incorrectly. If a value contains a comma, then all metadata
         //is treated as invalid and excluded from the payload.
-        internal static Dictionary<string, string> GetMetadata(string metadataPayload)
+        public static IDictionary<string, string> GetMetadata(string metadataPayload, char kvSeparator = ':')
         {
             var metadataDict = new Dictionary<string, string>();
 
@@ -337,7 +343,7 @@ internal static Dictionary<string, string> GetMetadata(string metadataPayload)
                     metadata = metadata.Slice(commaIndex + 1);
                 }
 
-                int colonIndex = kvPair.IndexOf(':');
+                int colonIndex = kvPair.IndexOf(kvSeparator);
                 if (colonIndex < 0)
                 {
                     metadataDict.Clear();

src/Microsoft.Diagnostics.Monitoring.EventPipe/Microsoft.Diagnostics.Monitoring.EventPipe.csproj

@@ -42,5 +42,7 @@
     <InternalsVisibleTo Include="dotnet-monitor" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.EventPipe.UnitTests" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.WebApi" />
+    <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.Tool.UnitTests" />
+    <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.UnitTests" />
   </ItemGroup>
 </Project>

src/Microsoft.Diagnostics.Monitoring/Microsoft.Diagnostics.Monitoring.csproj

@@ -28,6 +28,7 @@
     <InternalsVisibleTo Include="dotnet-monitor" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.EventPipe" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.EventPipe.UnitTests" />
+    <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.Tool.UnitTests" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.UnitTests" />
     <InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.WebApi" />
   </ItemGroup>

src/tests/Microsoft.Diagnostics.Monitoring.EventPipe/EventCounterPipelineUnitTests.cs

@@ -67,13 +67,9 @@ public void Log(ICounterPayload metric)
                 }
             }
 
-            public void PipelineStarted()
-            {
-            }
+            public Task PipelineStarted(CancellationToken token) => Task.CompletedTask;
 
-            public void PipelineStopped()
-            {
-            }
+            public Task PipelineStopped(CancellationToken token) => Task.CompletedTask;
 
             private static string CreateKey(ICounterPayload payload)
             {

src/tests/Microsoft.Diagnostics.Monitoring.EventPipe/EventCounterTriggerTests.cs

@@ -499,7 +499,7 @@ public void ValidateMetadataParsing_Success()
             const string value1 = "V1";
             const string key2 = "K2";
             const string value2 = "V:2";
-            Dictionary<string, string> metadataDict = TraceEventExtensions.GetMetadata($"{key1}:{value1},{key2}:{value2}");
+            IDictionary<string, string> metadataDict = TraceEventExtensions.GetMetadata($"{key1}:{value1},{key2}:{value2}");
 
             Assert.Equal(2, metadataDict.Count);
             Assert.Equal(value1, metadataDict[key1]);
@@ -515,7 +515,7 @@ public void ValidateMetadataParsing_Success()
         [InlineData("K1")]
         public void ValidateMetadataParsing_Failure(string invalidMetadata)
         {
-            Dictionary<string, string> metadataDict = TraceEventExtensions.GetMetadata(invalidMetadata);
+            IDictionary<string, string> metadataDict = TraceEventExtensions.GetMetadata(invalidMetadata);
 
             Assert.Empty(metadataDict);
         }