Fix quantile issue for summaries (#3546) (#3551)

* Fix quantile issue for summaries * Fix unit test * Fixup labels Co-authored-by: Wiktor Kopec <[email protected]>
dotnet · Feb 3, 2023 · 25ab523 · 25ab523
1 parent 1c94c83
commit 25ab523
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 13 deletions.
diff --git a/src/Microsoft.Diagnostics.Monitoring.WebApi/Metrics/MetricsStore.cs b/src/Microsoft.Diagnostics.Monitoring.WebApi/Metrics/MetricsStore.cs
@@ -5,6 +5,7 @@
 using Microsoft.Extensions.Logging;
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -103,6 +104,12 @@ public void AddMetric(ICounterPayload metric)
                 {
                     metrics.Dequeue();
                 }
+
+                // CONSIDER We only keep 1 histogram representation per snapshot. Is it meaningful for Prometheus to see previous histograms? These are not timestamped.
+                if ((metrics.Count > 1) && (metric is PercentilePayload))
+                {
+                    metrics.Dequeue();
+                }
             }
         }
 
@@ -133,7 +140,8 @@ public async Task SnapshotMetrics(Stream outputStream, CancellationToken token)
                 {
                     if (metric is PercentilePayload percentilePayload)
                     {
-                        foreach (Quantile quantile in percentilePayload.Quantiles)
+                        // Summary quantiles must appear from smallest to largest
+                        foreach (Quantile quantile in percentilePayload.Quantiles.OrderBy(q => q.Percentage))
                         {
                             string metricValue = PrometheusDataModel.GetPrometheusNormalizedValue(metric.Unit, quantile.Value);
                             string metricLabels = GetMetricLabels(metric, quantile.Percentage);
@@ -153,14 +161,17 @@ public async Task SnapshotMetrics(Stream outputStream, CancellationToken token)
         private static string GetMetricLabels(ICounterPayload metric, double? quantile)
         {
             string metadata = metric.Metadata;
+
+            char separator = IsMeter(metric) ? '=' : ':';
+            var metadataValues = CounterUtilities.GetMetadata(metadata, separator);
             if (quantile.HasValue)
             {
-                metadata = CounterUtilities.AppendPercentile(metadata, quantile.Value);
+                metadataValues.Add("quantile", quantile.Value.ToString(CultureInfo.InvariantCulture));
             }
 
-            char separator = IsMeter(metric) ? '=' : ':';
-            var keyValuePairs = from pair in CounterUtilities.GetMetadata(metadata, separator)
-                                select pair.Key + "=" + "\"" + pair.Value + "\"";
+            var keyValuePairs = from pair in metadataValues
+                                select PrometheusDataModel.GetPrometheusNormalizedLabel(pair.Key, pair.Value);
+
             string metricLabels = string.Join(", ", keyValuePairs);
 
             return metricLabels;

diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/MetricsFormattingTests.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/MetricsFormattingTests.cs
@@ -42,8 +42,7 @@ public async Task HistogramFormat_Test()
         {
             List<ICounterPayload> payload = new();
 
-            string tags1 = "Percentile=50";
-            payload.Add(new PercentilePayload(MeterName, InstrumentName, "DisplayName", string.Empty, tags1,
+            payload.Add(new PercentilePayload(MeterName, InstrumentName, "DisplayName", string.Empty, string.Empty,
                 new Quantile[] { new(0.5, Value1), new(0.95, Value2), new(0.99, Value3) },
                 Timestamp));
 
@@ -54,16 +53,16 @@ public async Task HistogramFormat_Test()
             // should we call this method, or should this also be implicitly testing its behavior by having this hard-coded?
             string metricName = $"{MeterName.ToLowerInvariant()}_{payload[0].Name}";
 
-            const string percentile_50 = "{Percentile=\"50\"}";
-            const string percentile_95 = "{Percentile=\"95\"}";
-            const string percentile_99 = "{Percentile=\"99\"}";
+            const string quantile_50 = "{quantile=\"0.5\"}";
+            const string quantile_95 = "{quantile=\"0.95\"}";
+            const string quantile_99 = "{quantile=\"0.99\"}";
 
             Assert.Equal(5, lines.Count);
             Assert.Equal(FormattableString.Invariant($"# HELP {metricName}{payload[0].Unit} {payload[0].DisplayName}"), lines[0]);
             Assert.Equal(FormattableString.Invariant($"# TYPE {metricName} summary"), lines[1]);
-            Assert.Equal(FormattableString.Invariant($"{metricName}{percentile_50} {Value1}"), lines[2]);
-            Assert.Equal(FormattableString.Invariant($"{metricName}{percentile_95} {Value2}"), lines[3]);
-            Assert.Equal(FormattableString.Invariant($"{metricName}{percentile_99} {Value3}"), lines[4]);
+            Assert.Equal(FormattableString.Invariant($"{metricName}{quantile_50} {Value1}"), lines[2]);
+            Assert.Equal(FormattableString.Invariant($"{metricName}{quantile_95} {Value2}"), lines[3]);
+            Assert.Equal(FormattableString.Invariant($"{metricName}{quantile_99} {Value3}"), lines[4]);
         }
 
         [Fact]