From 46202f12b1798b7efbf9ac6d5935b6e70fa3f2f5 Mon Sep 17 00:00:00 2001 From: wiktork Date: Fri, 27 Jan 2023 13:46:08 -0800 Subject: [PATCH] Initial PR feedback --- .../MetricSourceConfiguration.cs | 8 +++---- .../MonitoringSourceConfiguration.cs | 1 + .../Counters/CounterPayload.cs | 4 ++-- .../Counters/CounterUtilities.cs | 3 ++- ...{CounterPipeline.cs => MetricsPipeline.cs} | 6 +++--- ...Settings.cs => MetricsPipelineSettings.cs} | 2 +- .../Counters/TraceEventExtensions.cs | 21 ++++++++----------- src/Tools/dotnet-counters/CounterMonitor.cs | 1 + 8 files changed, 23 insertions(+), 23 deletions(-) rename src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/{CounterPipeline.cs => MetricsPipeline.cs} (95%) rename src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/{CounterPipelineSettings.cs => MetricsPipelineSettings.cs} (94%) diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs index f5bb9eae30..a045a343ec 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs @@ -64,20 +64,20 @@ public MetricSourceConfiguration(float metricIntervalSeconds, IEnumerable p.Provider)); SessionId = Guid.NewGuid().ToString(); EventPipeProvider metricsEventSourceProvider = - new EventPipeProvider("System.Diagnostics.Metrics", EventLevel.Informational, TimeSeriesValues, + new EventPipeProvider(MonitoringSourceConfiguration.SystemDiagnosticsMetricsProviderName, EventLevel.Informational, TimeSeriesValuesEventKeyword, new Dictionary() { { "SessionId", SessionId }, { "Metrics", metrics }, { "RefreshInterval", metricIntervalSeconds.ToString(CultureInfo.InvariantCulture) }, - { "MaxTimeSeries", maxTimeSeries.ToString() }, - { "MaxHistograms", maxHistograms.ToString() } + { "MaxTimeSeries", maxTimeSeries.ToString(CultureInfo.InvariantCulture) }, + { "MaxHistograms", maxHistograms.ToString(CultureInfo.InvariantCulture) } } ); diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs index 00486370f5..ec1aec13d0 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs @@ -27,6 +27,7 @@ public abstract class MonitoringSourceConfiguration public const string TplEventSource = "System.Threading.Tasks.TplEventSource"; public const string SampleProfilerProviderName = "Microsoft-DotNETCore-SampleProfiler"; public const string EventPipeProviderName = "Microsoft-DotNETCore-EventPipe"; + public const string SystemDiagnosticsMetricsProviderName = "System.Diagnostics.Metrics"; public static IEnumerable DefaultMetricProviders => new[] { SystemRuntimeEventSourceName, MicrosoftAspNetCoreHostingEventSourceName, GrpcAspNetCoreServer }; diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs index 810f0cff00..187cbfc387 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs @@ -106,13 +106,13 @@ public RatePayload(string providerName, string name, string displayName, string internal class PercentilePayload : CounterPayload { - public PercentilePayload(string providerName, string name, string displayName, string displayUnits, string metadata, IEnumerable<(double quantile, double value)> quantiles, DateTime timestamp) : + public PercentilePayload(string providerName, string name, string displayName, string displayUnits, string metadata, IEnumerable quantiles, DateTime timestamp) : base(providerName, name, displayName, displayUnits, metadata, 0.0, timestamp, "Metric", EventType.Histogram) { // In case these properties are not provided, set them to appropriate values. string counterName = string.IsNullOrEmpty(displayName) ? name : displayName; DisplayName = !string.IsNullOrEmpty(displayUnits) ? $"{counterName} ({displayUnits})" : counterName; - Quantiles = quantiles.Select(v => new Quantile(v.quantile, v.value)).ToArray(); + Quantiles = quantiles.ToArray(); } public Quantile[] Quantiles { get; } diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterUtilities.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterUtilities.cs index b09c4bc19e..26ead38882 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterUtilities.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterUtilities.cs @@ -51,4 +51,5 @@ public static IDictionary GetMetadata(string metadataPayload, ch private static string AppendPercentile(string tags, string percentile) => string.IsNullOrEmpty(tags) ? percentile : string.Concat(tags, ",", percentile); } -} \ No newline at end of file +} + \ No newline at end of file diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipeline.cs similarity index 95% rename from src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs rename to src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipeline.cs index ebb0c79301..1dfad989c7 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipeline.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipeline.cs @@ -12,14 +12,14 @@ namespace Microsoft.Diagnostics.Monitoring.EventPipe { - internal class CounterPipeline : EventSourcePipeline + internal class MetricsPipeline : EventSourcePipeline { private readonly IEnumerable _loggers; private readonly CounterFilter _filter; private string _sessionId; - public CounterPipeline(DiagnosticsClient client, - CounterPipelineSettings settings, + public MetricsPipeline(DiagnosticsClient client, + MetricsPipelineSettings settings, IEnumerable loggers) : base(client, settings) { _loggers = loggers ?? throw new ArgumentNullException(nameof(loggers)); diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipelineSettings.cs similarity index 94% rename from src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs rename to src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipelineSettings.cs index 7fdc85e24f..f0c9d6e64e 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/MetricsPipelineSettings.cs @@ -8,7 +8,7 @@ namespace Microsoft.Diagnostics.Monitoring.EventPipe { - internal class CounterPipelineSettings : EventSourcePipelineSettings + internal class MetricsPipelineSettings : EventSourcePipelineSettings { public EventPipeCounterGroup[] CounterGroups { get; set; } diff --git a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs index 645bb456f4..34255c6d34 100644 --- a/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs +++ b/src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs @@ -5,6 +5,7 @@ using Microsoft.Diagnostics.Tracing; using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; namespace Microsoft.Diagnostics.Monitoring.EventPipe @@ -72,7 +73,7 @@ public static bool TryGetCounterPayload(this TraceEvent traceEvent, CounterFilte return true; } - if (sessionId != null && "System.Diagnostics.Metrics".Equals(traceEvent.ProviderName)) + if (sessionId != null && MonitoringSourceConfiguration.SystemDiagnosticsMetricsProviderName.Equals(traceEvent.ProviderName)) { if (traceEvent.EventName == "BeginInstrumentReporting") { @@ -142,7 +143,7 @@ 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)) + if (double.TryParse(lastValueText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double lastValue)) { payload = new GaugePayload(meterName, instrumentName, null, unit, tags, lastValue, obj.TimeStamp); } @@ -177,7 +178,7 @@ private static void HandleCounterRate(TraceEvent traceEvent, CounterFilter filte return; } - if (double.TryParse(rateText, out double rate)) + if (double.TryParse(rateText, NumberStyles.Number | NumberStyles.Float, CultureInfo.InvariantCulture, out double rate)) { payload = new RatePayload(meterName, instrumentName, null, unit, tags, rate, filter.IntervalSeconds, traceEvent.TimeStamp); } @@ -211,12 +212,8 @@ private static void HandleHistogram(TraceEvent obj, CounterFilter filter, string return; } - if (string.IsNullOrEmpty(quantilesText)) - { - return; - } - - IList<(double, double)> quantiles = ParseQuantiles(quantilesText); + //Note quantiles can be empty. + IList quantiles = ParseQuantiles(quantilesText); payload = new PercentilePayload(meterName, instrumentName, null, unit, tags, quantiles, obj.TimeStamp); } @@ -308,10 +305,10 @@ private static void HandleObservableInstrumentCallbackError(TraceEvent obj, stri payload = new ErrorPayload(errorMessage, obj.TimeStamp); } - private static IList<(double, double)> ParseQuantiles(string quantileList) + private static IList ParseQuantiles(string quantileList) { string[] quantileParts = quantileList.Split(';', StringSplitOptions.RemoveEmptyEntries); - var quantiles = new List<(double, double)>(); + var quantiles = new List(); foreach (string quantile in quantileParts) { string[] keyValParts = quantile.Split('=', StringSplitOptions.RemoveEmptyEntries); @@ -327,7 +324,7 @@ private static void HandleObservableInstrumentCallbackError(TraceEvent obj, stri { continue; } - quantiles.Add((key, val)); + quantiles.Add(new Quantile(key, val)); } return quantiles; } diff --git a/src/Tools/dotnet-counters/CounterMonitor.cs b/src/Tools/dotnet-counters/CounterMonitor.cs index 5aaa1ef4eb..beb1e009c4 100644 --- a/src/Tools/dotnet-counters/CounterMonitor.cs +++ b/src/Tools/dotnet-counters/CounterMonitor.cs @@ -171,6 +171,7 @@ 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)