Show metric counts for Trend metrics

[na--]

As this user has pointed out, it doesn't make sense that we don't show the count of Trend metrics, especially since we actually have it available.

[imiric]

I'm currently exploring this issue, and would like to confirm the requirements.

As discussed briefly over Slack with @na--, the trend count should be displayed immediately after the trend name and colon, as the first column, without a column name, like so:

...
    iterations.................: 1      1.615195/s
    my_trend...................: 12345  avg=309.5    min=266      med=309.5    max=353      p(90)=344.3    p(95)=348.65
    vus........................: 1      min=1 max=1
    vus_max....................: 1      min=1 max=1
...

And this should only be displayed for user-created metrics, not any of the built-in trend metrics like http_req_blocked, http_req_connecting, etc., correct?

If yes, that makes sense, but would skew the column alignment of this row compared to the other trend metrics. Should we care about this and pad the other rows with blank space to match?

But more important than this alignment issue is that I currently don't see an easy way of distinguishing user-created metrics from built-in ones when rendering the results. Is there an existing way of doing this (besides checking metric names), or should we add another field to stats.Metric like UserCreated bool or something similar for this purpose? I don't like this approach, so I'm hoping there's an easier method.

If no (i.e. the count should be shown for built-in metrics as well), then that avoids the above two issues, but it doesn't make much sense to show, since the count will likely be the same for all and http_reqs already shows that.

Also, one minor question: should we display the column name in addition to the value? E.g. count=12345. This would make it consistent with the other values, and perhaps more readable.

Let me know what you think.

[na--]

And this should only be displayed for user-created metrics, not any of the built-in trend metrics like http_req_blocked, http_req_connecting, etc., correct?

Hmm a very good question... My gut feeling is that we should display this for all Trend metrics, both custom and built-in ones. However, there will be tons and tons of duplication with the different http_req_* metrics, they will all have the same count 😕 What's more, as you've mentioned, we already have a separate Counter metric called http_reqs that displays this exact same count...

So, unfortunately, it seems like we'd either have to:

1. Distinguish between Trend metrics for which we should display counts, and thread metrics for which we shouldn't. And that's not necessarily the same as distinguishing between user/custom and built-in metrics, because while showing the counts for the http_req_* metrics would be mostly repetitive and useless, the same cannot be said for all built-in metrics. For example, group_duration, ws_ping, ws_connecting, and probably some future ones (Better metrics about sleep and group #921, New CPU and memory usage metrics #888, Feature request: DNS test library #851 among others).
   It's also worth pointing out that even if we do this, it would only affect the current human-readable end of test summary. We probably should always output the count value for all Trend metrics in the upcoming JSON output of the end-of-test summary stats (JSON -report does not work.  #355).

2. (my currently preferred solution) Figure out a non-annoying way to display the duplicate information, so we won't need to distinguish between the different Trend metrics. My initial suggestion of having the count immediately after the metric name and colon won't work, since displaying the most useless information in the most visible space would be stupid 😅 . But if we place it last and add the count column name as you suggested, it might work. Here's an example about how this would look like with the thresholds example from the README (with an extra custom time Trend metric):

  ✓ check_failure_rate.........: 0.00%   ✓ 0    ✗ 1745
    checks.....................: 100.00% ✓ 4363 ✗ 0   
    data_received..............: 4.7 MB  67 kB/s
    data_sent..................: 238 kB  3.4 kB/s
    group_duration.............: avg=151ms    min=137.81ms med=148.53ms max=937.56ms p(90)=154.42ms p(95)=156.84ms  count=824
    http_req_blocked...........: avg=8.48ms   min=1.13µs   med=2.81µs   max=585.59ms p(90)=5.09µs   p(95)=6.84µs    count=2617  
    http_req_connecting........: avg=2.79ms   min=0s       med=0s       max=189.75ms p(90)=0s       p(95)=0s        count=2617  
  ✓ http_req_duration..........: avg=149.09ms min=135.37ms med=147.71ms max=937.21ms p(90)=154.01ms p(95)=156.44ms  count=2617  
    ✓ { staticAsset:yes }......: avg=149.72ms min=135.37ms med=147.77ms max=937.21ms p(90)=153.43ms p(95)=156.14ms  count=1648    
    http_req_receiving.........: avg=449.14µs min=39.82µs  med=129.46µs max=148.99ms p(90)=323.54µs p(95)=642.32µs  count=2617  
    http_req_sending...........: avg=58.36µs  min=9.22µs   med=48.69µs  max=404.67µs p(90)=99.9µs   p(95)=128.44µs  count=2617  
    http_req_tls_handshaking...: avg=5.67ms   min=0s       med=0s       max=398.18ms p(90)=0s       p(95)=0s        count=2617  
    http_req_waiting...........: avg=148.58ms min=135.24ms med=147.34ms max=937.12ms p(90)=153.2ms  p(95)=155.74ms  count=2617  
    http_reqs..................: 2617    37.38567/s
    iteration_duration.........: avg=3.81s    min=2.29s    med=3.77s    max=5.71s    p(90)=5s       p(95)=5.14s     count=824  
    iterations.................: 824     11.771415/s
    my_custom_trend............: avg=149.76ms min=135.37ms med=147.4ms  max=937.21ms p(90)=152.98ms p(95)=155.74ms  count=123  
    vus........................: 50      min=5  max=50
    vus_max....................: 50      min=50 max=50

Not great, but not super annoying either... Notice that the subset of http_req_duration with the staticAsset tag has a different count, which is useful to know. And this specific script just has a single group, but generally the count of the group_duration metric would be different than the count of iterations.

So, in summary, I think we should always display the count, even if there's some repetition, just in a maximally unobtrusive way. What's more, I'm not sure we should always emit all http_req_* metrics. It will be a minor breaking change, but it's currently a bit strange that we emit http_req_tls_handshaking=0 for plaintext HTTP requests...

@mstoykov, @cuonglm - thoughts?

If yes, that makes sense, but would skew the column alignment of this row compared to the other trend metrics. Should we care about this and pad the other rows with blank space to match?

I'd consider fixing the alignment a separate issue, since we already have issues there: #1024

[na--]

Ah, I just realized another point in favor of the second approach 😄 If we don't like how this looks, we don't actually need to always display the count value of trend metrics.

Instead, we just need to have it available as an option that can be configured via the summaryTrendStats option. And properly document it, of course... That way, users can run their scripts with something like k6 run --summary-trend-stats="min,med,avg,max,p(70.1),p(99),p(99.9),p(99.999),count" script.js and have count available as a column in the report, positioned at wherever place they like in the columns.

[imiric]

Thanks @na--, I like the approach of adding it as a last column, since it doesn't shift the existing column positions, so it's mostly backwards compatible if people were parsing this output (like GitLab mentioned, I think). I'll give that a try and create a PR.

Giving the option to enable/disable/reorder columns would also be great eventually. Do we have an issue about that?

[na--]

We already have that option 😄 Read through the docs and the code to see how it's used: https://github.com/loadimpact/k6/blob/d385166a821a8cd98a4ab2c158b3982d87ee61a5/lib/options.go#L317-L321

So we should be able to expose the count variable for trend metrics to people who need it, without actually outputting it by default.

[imiric]

Oh, I thought that option was for showing additional custom stats (e.g. p(99.99)), not for controlling the visibility of built-in ones. Neat!

OK, it makes sense to show count optionally as well then. I'll look into it, thanks.