PR feedback

src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs

@@ -13,71 +13,82 @@
 
 namespace Microsoft.Diagnostics.Monitoring.EventPipe
 {
+    [Flags]
+    public enum MetricType
+    {
+        EventCounter = 0x1,
+        Meter = 0x2
+    }
+
+    public sealed class MetricEventPipeProvider
+    {
+        public string Provider { get; set; }
+
+        public float? IntervalSeconds { get; set; }
+
+        public MetricType Type { get; set; } = MetricType.EventCounter | MetricType.Meter;
+    }
+
     public sealed class MetricSourceConfiguration : MonitoringSourceConfiguration
     {
         private readonly IList<EventPipeProvider> _eventPipeProviders;
         public string SessionId { get; private set; }
 
-        public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> customProviderNames)
+        public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> eventCounterProviderNames)
+            : this(metricIntervalSeconds, CreateProviders(eventCounterProviderNames?.Any() == true ? eventCounterProviderNames : DefaultMetricProviders))
         {
-            RequestRundown = false;
-            if (customProviderNames == null)
-            {
-                throw new ArgumentNullException(nameof(customProviderNames));
-            }
-            MetricIntervalSeconds = metricIntervalSeconds.ToString(CultureInfo.InvariantCulture);
-
-            IEnumerable<string> providers = null;
-            if (customProviderNames.Any())
-            {
-                providers = customProviderNames;
-            }
-            else
-            {
-                providers = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer };
-            }
-
-            _eventPipeProviders = providers.Select((string provider) => new EventPipeProvider(provider,
-               EventLevel.Informational,
-               (long)ClrTraceEventParser.Keywords.None,
-               new Dictionary<string, string>()
-               {
-                    { "EventCounterIntervalSec", MetricIntervalSeconds }
-               })).ToList();
         }
 
-        public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<string> customProviderNames, int maxHistograms, int maxTimeSeries) : this(metricIntervalSeconds, customProviderNames)
+        public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable<MetricEventPipeProvider> providers, int maxHistograms = 20, int maxTimeSeries = 1000)
         {
-            const long TimeSeriesValues = 0x2;
-            StringBuilder metrics = new StringBuilder();
-            foreach (string provider in customProviderNames)
+            if (providers == null)
             {
-                if (metrics.Length != 0)
-                {
-                    metrics.Append(",");
-                }
-
-                metrics.Append(provider);
+                throw new ArgumentNullException(nameof(providers));
             }
 
-            SessionId = Guid.NewGuid().ToString();
+            RequestRundown = false;
 
-            EventPipeProvider metricsEventSourceProvider =
-                new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues,
+            _eventPipeProviders = providers.Where(provider => provider.Type.HasFlag(MetricType.EventCounter))
+                .Select((MetricEventPipeProvider provider) => new EventPipeProvider(provider.Provider,
+                    EventLevel.Informational,
+                    (long)ClrTraceEventParser.Keywords.None,
                     new Dictionary<string, string>()
                     {
-                        { "SessionId", SessionId },
-                        { "Metrics", metrics.ToString() },
-                        { "RefreshInterval", MetricIntervalSeconds.ToString() },
-                        { "MaxTimeSeries", maxTimeSeries.ToString() },
-                        { "MaxHistograms", maxHistograms.ToString() }
-                    }
-                );
-
-            _eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray();
+                        {
+                            "EventCounterIntervalSec", (provider.IntervalSeconds ?? metricIntervalSeconds).ToString(CultureInfo.InvariantCulture)
+                        }
+                    })).ToList();
+
+            IEnumerable<MetricEventPipeProvider> meterProviders = providers.Where(provider => provider.Type.HasFlag(MetricType.Meter));
+
+            if (meterProviders.Any())
+            {
+                const long TimeSeriesValues = 0x2;
+                string metrics = string.Join(',', meterProviders.Select(p => p.Provider));
+
+                SessionId = Guid.NewGuid().ToString();
+
+                EventPipeProvider metricsEventSourceProvider =
+                    new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues,
+                        new Dictionary<string, string>()
+                        {
+                            { "SessionId", SessionId },
+                            { "Metrics", metrics },
+                            { "RefreshInterval", metricIntervalSeconds.ToString(CultureInfo.InvariantCulture) },
+                            { "MaxTimeSeries", maxTimeSeries.ToString() },
+                            { "MaxHistograms", maxHistograms.ToString() }
+                        }
+                    );
+
+                _eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray();
+            }
         }
 
-        private string MetricIntervalSeconds { get; }
+        private static IEnumerable<MetricEventPipeProvider> CreateProviders(IEnumerable<string> providers) =>
+            providers.Select(provider => new MetricEventPipeProvider {
+                Provider = provider,
+                Type = MetricType.EventCounter
+            });
 
         public override IList<EventPipeProvider> GetProviders() => _eventPipeProviders;
     }

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

@@ -129,7 +129,6 @@ public ErrorPayload(string errorMessage, DateTime timestamp) :
         public string ErrorMessage { get; private set; }
     }
 
-    // If keep this, should probably put it somewhere else
     internal enum EventType : int
     {
         Rate,

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

@@ -6,6 +6,7 @@
 using Microsoft.Diagnostics.Tracing;
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 
@@ -39,7 +40,13 @@ public CounterPipeline(DiagnosticsClient client,
 
         protected override MonitoringSourceConfiguration CreateConfiguration()
         {
-            var config = new MetricSourceConfiguration(Settings.CounterIntervalSeconds, _filter.GetProviders(), Settings.MaxHistograms, Settings.MaxTimeSeries);
+            var config = new MetricSourceConfiguration(Settings.CounterIntervalSeconds, Settings.CounterGroups.Select((EventPipeCounterGroup counterGroup) => new MetricEventPipeProvider
+                {
+                    Provider = counterGroup.ProviderName,
+                    IntervalSeconds = counterGroup.IntervalSeconds,
+                    Type = (MetricType)counterGroup.Type
+                }),
+                Settings.MaxHistograms, Settings.MaxTimeSeries);
 
             _sessionId = config.SessionId;
 

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

@@ -21,9 +21,21 @@ internal class CounterPipelineSettings : EventSourcePipelineSettings
         public int MaxTimeSeries { get; set; }
     }
 
+    [Flags]
+    internal enum CounterGroupType
+    {
+        EventCounter = 0x1,
+        Meter = 0x2,
+    }
+
     internal class EventPipeCounterGroup
     {
         public string ProviderName { get; set; }
+
         public string[] CounterNames { get; set; }
+
+        public CounterGroupType Type { get; set; } = CounterGroupType.EventCounter | CounterGroupType.Meter;
+
+        public float? IntervalSeconds { get; set; }
     }
 }

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

@@ -166,10 +166,7 @@ private static void HandleGauge(TraceEvent obj, CounterFilter filter, string ses
             {
                 // 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);
-
             }
         }
 
@@ -200,6 +197,12 @@ private static void HandleCounterRate(TraceEvent traceEvent, CounterFilter filte
             {
                 payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, filter.IntervalSeconds, 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.
+                payload = new CounterEndedPayload(meterName, instrumentName, null, traceEvent.TimeStamp);
+            }
         }
 
         private static void HandleHistogram(TraceEvent obj, CounterFilter filter, string sessionId, out List<ICounterPayload> payload)

src/Tools/dotnet-counters/CounterMonitor.cs

@@ -171,7 +171,6 @@ private void HandleCounterRate(TraceEvent obj)
                 CounterPayload payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, _interval, obj.TimeStamp);
                 _renderer.CounterPayloadReceived(payload, _pauseCmdSet);
             }
-
         }
 
         private void HandleGauge(TraceEvent obj)