Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions internal/cmd/tests/cmd_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 20 additions & 3 deletions internal/js/summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ function renderThresholdResults(
: formatter.decorate(failMark, 'red');

const sourceText = formatter.decorate(
`'${threshold.source}'`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't directly related, but it's so small that I prefer to add it as part of this PR. It's not a huge deal, but I think it's nice if we trim the threshold's source before displaying it in the summary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for trimming here? If the source includes spaces why we don't interpret it as intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the source includes spaces why we don't interpret it as intentional?

I'm a bit confused now; is there any reason for doing so?

To me, it feels like when you mistakenly add a leading or trailing whitespace in an input designed to write your name or similar.

Looking at the threshold syntax expression (ref), I can see for instance users adding (or not) whitespaces before/after the <operator>, for readability, but not why doing so at the beginning/end of the threshold definition.

`'${threshold.source.trim()}'`,
'white',
);

Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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.
Expand All @@ -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 = {};
Copy link
Contributor

@codebien codebien Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// While "trendCols" contain the values for each "trendStats"

It doesn't sound right without applying in-depth thinking. Because Trend is itself a stat. Wouldn't it be easier to just use the concept of Threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure 🤷🏻 The code/data structure is quite generic, and while it's true that this is now used for threshold, it could technically be used for anything else, so I tried to keep it consistent with the existing "conventions" (if any) - one data structure holds stats identified by "columns", and this one by "keys" 😅

Overall, that code has many aspects that could be improved (there's already some // TODOs), and @oleiade already did a great job fixing some as part of #4089, but I'd prefer if we can keep focus on fixing the issue that's been reported, and re-evaluate namings and possible improvements over this code in another PR/ as part of another issue/work.


let maxNameWidth = 0;
let maxNonTrendValueLen = 0;
let nonTrendExtraMaxLens = []; // FIXME: "lens"?
Expand All @@ -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),
);
Expand All @@ -1094,6 +1109,7 @@ function computeSummaryInfo(metrics, renderContext, options) {
);
});
trendCols[name] = cols;
trendKeys[name] = keys;
} else {
const values = nonTrendMetricValueForSum(
metric,
Expand Down Expand Up @@ -1127,6 +1143,7 @@ function computeSummaryInfo(metrics, renderContext, options) {
nonTrendExtras,
trendCols,
trendColMaxLens,
trendKeys,
numTrendColumns,
trendStats,
maxNonTrendValueLen,
Expand Down
36 changes: 36 additions & 0 deletions internal/output/summary/data.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package summary

import (
"regexp"
"strconv"
"strings"
"sync/atomic"
"time"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Loading