Summary: Display thresholds values even when not in summaryTrendStats (#4698)

* Summary: Display thresholds values even when not in summaryTrendStats

* Apply suggestions from code review

Author: joanlopez

internal/cmd/tests/cmd_run_test.go

@@ -342,6 +342,38 @@ func TestMetricsAndThresholds(t *testing.T) {
 	require.Equal(t, expected, teardownThresholds)
 }
 
+func TestThresholdsWithCustomPercentile(t *testing.T) {
+	t.Parallel()
+	script := `
+		export const options = {
+			scenarios: {
+				sc1: {
+					executor: 'per-vu-iterations',
+					vus: 1,
+					iterations: 1,
+				},
+			},
+			thresholds: {
+				'iteration_duration': ['p(0)<50', 'p(90)<100', 'p(99.5)<150', 'p(99.99)<200'],
+			},
+		};
+
+		export default function () {}
+	`
+	ts := getSingleFileTestState(t, script, nil, 0)
+	cmd.ExecuteWithGlobalState(ts.GlobalState)
+
+	stdout := ts.Stdout.String()
+	t.Log(stdout)
+
+	// We want to make sure that all the thresholds expressions are accompanied
+	// by their values, despite those being present in `options.summaryTrendStats` or not.
+	assert.Regexp(t, `✓ 'p\(0\)<50' p\(0\)=\d+(\.\d+)?(µs|ms|ns|s)`, stdout)
+	assert.Regexp(t, `✓ 'p\(90\)<100' p\(90\)=\d+(\.\d+)?(µs|ms|ns|s)`, stdout)
+	assert.Regexp(t, `✓ 'p\(99.5\)<150' p\(99.5\)=\d+(\.\d+)?(µs|ms|ns|s)`, stdout)
+	assert.Regexp(t, `✓ 'p\(99.99\)<200' p\(99.99\)=\d+(\.\d+)?(µs|ms|ns|s)`, stdout)
+}
+
 func TestSSLKEYLOGFILEAbsolute(t *testing.T) {
 	t.Parallel()
 	ts := NewGlobalTestState(t)

internal/js/summary.js

@@ -839,7 +839,7 @@ function renderThresholdResults(
 			: formatter.decorate(failMark, 'red');
 
 		const sourceText = formatter.decorate(
-			`'${threshold.source}'`,
+			`'${threshold.source.trim()}'`,
 			'white',
 		);
 
@@ -941,13 +941,16 @@ function renderMetricValueForThresholds(
 	info,
 	formatter,
 ) {
-	const {trendStats, trendCols, nonTrendValues, nonTrendExtras} = info;
+	const {trendStats, trendCols, trendKeys, nonTrendValues, nonTrendExtras} = info;
 	const thresholdAgg = threshold.source.split(/[=><]/)[0].trim();
 
 	let value;
 	switch (metric.type) {
 		case 'trend':
-			value = trendCols[metric.name][trendStats.indexOf(thresholdAgg)]
+			const trendStatIndex = trendStats.indexOf(thresholdAgg);
+			value = (trendStatIndex !== -1)
+				? trendCols[metric.name]?.[trendStatIndex]
+				: trendKeys[metric.name]?.[thresholdAgg];
 			break;
 		case 'counter':
 			value = (thresholdAgg === 'count')
@@ -1042,6 +1045,7 @@ function renderTrendValue(value, stat, metric, options) {
  * @property {Object} nonTrendValues - The non-trend metric values.
  * @property {Object} nonTrendExtras - The non-trend metric extras.
  * @property {Object} trendCols - The trend columns.
+ * @property {Object} trendKeys - The trend keys (values that aren't included within `trendStats`).
  * @property {number[]} trendColMaxLens - The trend column maximum lengths.
  * @property {number} numTrendColumns - The number of trend columns.
  * @property {string[]} trendStats - The trend statistics.
@@ -1061,6 +1065,10 @@ function computeSummaryInfo(metrics, renderContext, options) {
 	const nonTrendExtras = {};
 	const trendCols = {};
 
+	// While "trendCols" contain the values for each "trendStats" aggregation (e.g. p(90) as a sorted array,
+	// "trendKeys" is used to store specific aggregation values that aren't part of "trendStats"; mainly for thresholds.
+	const trendKeys = {};
+
 	let maxNameWidth = 0;
 	let maxNonTrendValueLen = 0;
 	let nonTrendExtraMaxLens = []; // FIXME: "lens"?
@@ -1082,6 +1090,13 @@ function computeSummaryInfo(metrics, renderContext, options) {
 		maxNameWidth = Math.max(maxNameWidth, strWidth(displayName));
 
 		if (metric.type === 'trend') {
+			const keys = Object.keys(metric.values).reduce((acc, key) => {
+				if (!trendStats.includes(key)) {
+					acc[key] = renderTrendValue(metric.values[key], key, metric, options);
+				}
+				return acc;
+			}, {});
+
 			const cols = trendStats.map((stat) =>
 				renderTrendValue(metric.values[stat], stat, metric, options),
 			);
@@ -1094,6 +1109,7 @@ function computeSummaryInfo(metrics, renderContext, options) {
 				);
 			});
 			trendCols[name] = cols;
+			trendKeys[name] = keys;
 		} else {
 			const values = nonTrendMetricValueForSum(
 				metric,
@@ -1127,6 +1143,7 @@ function computeSummaryInfo(metrics, renderContext, options) {
 		nonTrendExtras,
 		trendCols,
 		trendColMaxLens,
+		trendKeys,
 		numTrendColumns,
 		trendStats,
 		maxNonTrendValueLen,

internal/output/summary/data.go

@@ -1,6 +1,8 @@
 package summary
 
 import (
+	"regexp"
+	"strconv"
 	"strings"
 	"sync/atomic"
 	"time"
@@ -245,6 +247,15 @@ func summaryThresholds(
 				Source: threshold.Source,
 				Ok:     !threshold.LastFailed,
 			})
+
+			// Additionally, if metric is a trend and the threshold source is a percentile,
+			// we may need to add the percentile value to the metric values, in case it
+			// isn't one of [summaryTrendStats].
+			if trendSink, isTrend := mThresholds.Sink.(*metrics.TrendSink); isTrend {
+				if agg, percentile, isPercentile := extractPercentileThresholdSource(threshold.Source); isPercentile {
+					mt.Metric.Values[agg] = trendSink.P(percentile / 100)
+				}
+			}
 		}
 
 		rts[mName] = mt
@@ -393,3 +404,31 @@ func metricValueGetter(summaryTrendStats []string) func(metrics.Sink, time.Durat
 		return result
 	}
 }
+
+var percentileThresholdSourceRe = regexp.MustCompile(`^p\((\d+(?:\.\d+)?)\)\s*([<>=])`)
+
+// TODO(@joanlopez): Evaluate whether we should expose the parsed expression from the threshold.
+// For now, and until we formally decide how do we want the `metrics` package to look like, we do "manually"
+// parse the threshold expression here in order to extract, if existing, the percentile aggregation.
+func extractPercentileThresholdSource(source string) (agg string, percentile float64, isPercentile bool) {
+	// We capture the following three matches, in order to detect whether source is a percentile:
+	// 1. The percentile definition: p(...)
+	// 2. The percentile value: p(??)
+	// 3. The beginning of the operator: '<', '>', or '='
+	const expectedMatches = 3
+	matches := percentileThresholdSourceRe.FindStringSubmatch(strings.TrimSpace(source))
+
+	if len(matches) == expectedMatches {
+		var err error
+		percentile, err = strconv.ParseFloat(matches[1], 64)
+		if err != nil {
+			return "", 0, false
+		}
+
+		agg = "p(" + matches[1] + ")"
+		isPercentile = true
+		return
+	}
+
+	return "", 0, false
+}

internal/output/summary/data_test.go

@@ -0,0 +1,46 @@
+package summary
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_extractPercentileThresholdSource(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		source          string
+		expAgg          string
+		expPercentile   float64
+		expIsPercentile bool
+	}{
+		"empty source": {
+			source: "", expAgg: "", expPercentile: 0, expIsPercentile: false,
+		},
+		"invalid source": {
+			source: "rate<<<<0.01", expAgg: "", expPercentile: 0, expIsPercentile: false,
+		},
+		"rate threshold": {
+			source: "rate<0.01", expAgg: "", expPercentile: 0, expIsPercentile: false,
+		},
+		"integer percentile": {
+			source: "p(95)<200", expAgg: "p(95)", expPercentile: 95, expIsPercentile: true,
+		},
+		"floating-point percentile": {
+			source: "p(99.9)<=200", expAgg: "p(99.9)", expPercentile: 99.9, expIsPercentile: true,
+		},
+		"percentile with whitespaces": {
+			source: "  p(99.9)  <  200  ", expAgg: "p(99.9)", expPercentile: 99.9, expIsPercentile: true,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			agg, percentile, isPercentile := extractPercentileThresholdSource(tc.source)
+			assert.Equal(t, tc.expAgg, agg)
+			assert.Equal(t, tc.expPercentile, percentile)
+			assert.Equal(t, tc.expIsPercentile, isPercentile)
+		})
+	}
+}