From c4bb651cb85d7df34df23b7140c4eeb97f5c5828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20L=C3=B3pez=20de=20la=20Franca=20Beltran?= Date: Thu, 17 Apr 2025 12:29:30 +0200 Subject: [PATCH 1/2] Summary: Display thresholds values even when not in summaryTrendStats --- internal/cmd/tests/cmd_run_test.go | 32 ++++++++++++++++++++++++++ internal/js/summary.js | 23 ++++++++++++++++--- internal/output/summary/data.go | 36 ++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/internal/cmd/tests/cmd_run_test.go b/internal/cmd/tests/cmd_run_test.go index d16ffd6edb..5fa71f2fc7 100644 --- a/internal/cmd/tests/cmd_run_test.go +++ b/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) diff --git a/internal/js/summary.js b/internal/js/summary.js index c3cbfda883..2923ca5743 100644 --- a/internal/js/summary.js +++ b/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, diff --git a/internal/output/summary/data.go b/internal/output/summary/data.go index d8f9b6e516..89bc509e0a 100644 --- a/internal/output/summary/data.go +++ b/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's + // not 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,28 @@ func metricValueGetter(summaryTrendStats []string) func(metrics.Sink, time.Durat return result } } + +var percentileThresholdSourceRe = regexp.MustCompile(`^p\((\d+(?:\.\d+)?)\)\s*([<>=])`) + +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 +} From 062cc972112942b54b18cab994795a0887022c09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joan=20L=C3=B3pez=20de=20la=20Franca=20Beltran?= Date: Wed, 23 Apr 2025 08:41:14 +0200 Subject: [PATCH 2/2] Apply suggestions from code review --- internal/output/summary/data.go | 7 +++-- internal/output/summary/data_test.go | 46 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 internal/output/summary/data_test.go diff --git a/internal/output/summary/data.go b/internal/output/summary/data.go index 89bc509e0a..ea3e6455c4 100644 --- a/internal/output/summary/data.go +++ b/internal/output/summary/data.go @@ -249,8 +249,8 @@ func summaryThresholds( }) // 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's - // not one of [summaryTrendStats]. + // 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) @@ -407,6 +407,9 @@ func metricValueGetter(summaryTrendStats []string) func(metrics.Sink, time.Durat 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(...) diff --git a/internal/output/summary/data_test.go b/internal/output/summary/data_test.go new file mode 100644 index 0000000000..223d958885 --- /dev/null +++ b/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) + }) + } +}