From 149f3adb4b0d4b36e835a91907b3566fe8664d4a Mon Sep 17 00:00:00 2001 From: wiktork Date: Thu, 19 Jan 2023 13:58:32 -0800 Subject: [PATCH 1/2] PR feedback --- .../MetricSourceConfiguration.cs | 97 +++++++++++-------- .../Counters/CounterPayload.cs | 1 - .../Counters/CounterPipeline.cs | 9 +- .../Counters/CounterPipelineSettings.cs | 12 +++ .../Counters/TraceEventExtensions.cs | 9 +- src/Tools/dotnet-counters/CounterMonitor.cs | 1 - 6 files changed, 82 insertions(+), 47 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs index 1c3891f123..13567ee754 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs @@ -13,70 +13,85 @@ 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 _eventPipeProviders; public string SessionId { get; private set; } + public static readonly string[] DefaultProviders = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer }; + public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable customProviderNames) + : this(metricIntervalSeconds, customProviderNames?.Any() == true ? CreateProviders(metricIntervalSeconds, customProviderNames) : + CreateProviders(metricIntervalSeconds, DefaultProviders)) { - RequestRundown = false; - if (customProviderNames == null) - { + } + + public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable customProviderNames, int maxHistograms = 20, int maxTimeSeries = 1000) { + if (customProviderNames == null) { throw new ArgumentNullException(nameof(customProviderNames)); } + + RequestRundown = false; MetricIntervalSeconds = metricIntervalSeconds.ToString(CultureInfo.InvariantCulture); - IEnumerable providers = null; - if (customProviderNames.Any()) - { - providers = customProviderNames; - } - else - { - providers = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer }; - } + _eventPipeProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.EventCounter)) + .Select((MetricEventPipeProvider provider) => new EventPipeProvider(provider.Provider, + EventLevel.Informational, + (long)ClrTraceEventParser.Keywords.None, + new Dictionary() + { + { "EventCounterIntervalSec", provider.IntervalSeconds.ToString(CultureInfo.InvariantCulture)} + })).ToList(); - _eventPipeProviders = providers.Select((string provider) => new EventPipeProvider(provider, - EventLevel.Informational, - (long)ClrTraceEventParser.Keywords.None, - new Dictionary() - { - { "EventCounterIntervalSec", MetricIntervalSeconds } - })).ToList(); - } + IEnumerable meterProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.Meter)); - public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable customProviderNames, int maxHistograms, int maxTimeSeries) : this(metricIntervalSeconds, customProviderNames) - { - const long TimeSeriesValues = 0x2; - StringBuilder metrics = new StringBuilder(); - foreach (string provider in customProviderNames) + if (meterProviders.Any()) { - if (metrics.Length != 0) - { - metrics.Append(","); - } - - metrics.Append(provider); - } + const long TimeSeriesValues = 0x2; + string metrics = string.Join(',', meterProviders.Select(p => p.Provider)); - SessionId = Guid.NewGuid().ToString(); + SessionId = Guid.NewGuid().ToString(); - EventPipeProvider metricsEventSourceProvider = - new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues, - new Dictionary() - { + EventPipeProvider metricsEventSourceProvider = + new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues, + new Dictionary() + { { "SessionId", SessionId }, - { "Metrics", metrics.ToString() }, + { "Metrics", metrics }, { "RefreshInterval", MetricIntervalSeconds.ToString() }, { "MaxTimeSeries", maxTimeSeries.ToString() }, { "MaxHistograms", maxHistograms.ToString() } - } - ); + } + ); - _eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray(); + _eventPipeProviders = _eventPipeProviders.Append(metricsEventSourceProvider).ToArray(); + } } + private static IEnumerable CreateProviders(float metricIntervalSeconds, IEnumerable customProviderNames) => + customProviderNames.Select(provider => new MetricEventPipeProvider { + Provider = provider, + IntervalSeconds = metricIntervalSeconds, + Type = MetricType.EventCounter + }); + + private string MetricIntervalSeconds { get; } public override IList GetProviders() => _eventPipeProviders; diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs index 459440ee21..cbdee69e79 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs +++ b/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, diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs index 76c81143d6..f764f99834 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs +++ b/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; diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs index 51ce90dbfd..3926db3f8f 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs +++ b/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; } } } diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs index d4fec2e295..01bf1aa8a3 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs +++ b/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 payload) diff --git a/src/Tools/dotnet-counters/CounterMonitor.cs b/src/Tools/dotnet-counters/CounterMonitor.cs index beb1e009c4..5aaa1ef4eb 100644 --- a/src/Tools/dotnet-counters/CounterMonitor.cs +++ b/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) From 22ff6446cdd75ba19fc1a9d14da2b32f94edfbb5 Mon Sep 17 00:00:00 2001 From: wiktork Date: Mon, 23 Jan 2023 21:36:26 -0800 Subject: [PATCH 2/2] Pr feedback feedback --- .../MetricSourceConfiguration.cs | 56 +++++++++---------- .../Counters/CounterPipelineSettings.cs | 2 +- 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs index 13567ee754..701d5e8c34 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs @@ -19,12 +19,12 @@ public enum MetricType EventCounter = 0x1, Meter = 0x2 } - + public sealed class MetricEventPipeProvider { public string Provider { get; set; } - public float IntervalSeconds { get; set; } + public float? IntervalSeconds { get; set; } public MetricType Type { get; set; } = MetricType.EventCounter | MetricType.Meter; } @@ -34,32 +34,32 @@ public sealed class MetricSourceConfiguration : MonitoringSourceConfiguration private readonly IList _eventPipeProviders; public string SessionId { get; private set; } - public static readonly string[] DefaultProviders = new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer }; - - public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable customProviderNames) - : this(metricIntervalSeconds, customProviderNames?.Any() == true ? CreateProviders(metricIntervalSeconds, customProviderNames) : - CreateProviders(metricIntervalSeconds, DefaultProviders)) + public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable eventCounterProviderNames) + : this(metricIntervalSeconds, CreateProviders(eventCounterProviderNames?.Any() == true ? eventCounterProviderNames : DefaultMetricProviders)) { } - public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable customProviderNames, int maxHistograms = 20, int maxTimeSeries = 1000) { - if (customProviderNames == null) { - throw new ArgumentNullException(nameof(customProviderNames)); + public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable providers, int maxHistograms = 20, int maxTimeSeries = 1000) + { + if (providers == null) + { + throw new ArgumentNullException(nameof(providers)); } RequestRundown = false; - MetricIntervalSeconds = metricIntervalSeconds.ToString(CultureInfo.InvariantCulture); - _eventPipeProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.EventCounter)) + _eventPipeProviders = providers.Where(provider => provider.Type.HasFlag(MetricType.EventCounter)) .Select((MetricEventPipeProvider provider) => new EventPipeProvider(provider.Provider, - EventLevel.Informational, - (long)ClrTraceEventParser.Keywords.None, - new Dictionary() - { - { "EventCounterIntervalSec", provider.IntervalSeconds.ToString(CultureInfo.InvariantCulture)} - })).ToList(); + EventLevel.Informational, + (long)ClrTraceEventParser.Keywords.None, + new Dictionary() + { + { + "EventCounterIntervalSec", (provider.IntervalSeconds ?? metricIntervalSeconds).ToString(CultureInfo.InvariantCulture) + } + })).ToList(); - IEnumerable meterProviders = customProviderNames.Where(provider => provider.Type.HasFlag(MetricType.Meter)); + IEnumerable meterProviders = providers.Where(provider => provider.Type.HasFlag(MetricType.Meter)); if (meterProviders.Any()) { @@ -72,11 +72,11 @@ public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable() { - { "SessionId", SessionId }, - { "Metrics", metrics }, - { "RefreshInterval", MetricIntervalSeconds.ToString() }, - { "MaxTimeSeries", maxTimeSeries.ToString() }, - { "MaxHistograms", maxHistograms.ToString() } + { "SessionId", SessionId }, + { "Metrics", metrics }, + { "RefreshInterval", metricIntervalSeconds.ToString(CultureInfo.InvariantCulture) }, + { "MaxTimeSeries", maxTimeSeries.ToString() }, + { "MaxHistograms", maxHistograms.ToString() } } ); @@ -84,16 +84,12 @@ public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable CreateProviders(float metricIntervalSeconds, IEnumerable customProviderNames) => - customProviderNames.Select(provider => new MetricEventPipeProvider { + private static IEnumerable CreateProviders(IEnumerable providers) => + providers.Select(provider => new MetricEventPipeProvider { Provider = provider, - IntervalSeconds = metricIntervalSeconds, Type = MetricType.EventCounter }); - - private string MetricIntervalSeconds { get; } - public override IList GetProviders() => _eventPipeProviders; } } diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs index 3926db3f8f..9833f71038 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs @@ -36,6 +36,6 @@ internal class EventPipeCounterGroup public CounterGroupType Type { get; set; } = CounterGroupType.EventCounter | CounterGroupType.Meter; - public float IntervalSeconds { get; set; } + public float? IntervalSeconds { get; set; } } }