Summary callback changes #1768

na-- · 2020-12-14T09:50:02Z

This is a WIP attempt at fixing a bunch of issues (fixes #1316, fixes #1611, closes #817, closes #647, closes #351) and making possible the implementation of a bunch of other ones (#1315, #1675, #1319)...

The way we do that is by supporting a new handleSummary() exported function that allows users to customize the test summary (or summaries) however the want. The final goal is something like this:

import http from 'k6/http';
import { JUnit, TextSummary } from 'https://jslib.k6.io/k6-summary/1.0.0/index.js' // or something like that
// ...
export function handleSummary(data) {
    // save everything to files and send it to a server when the test finishes or gets aborted
    http.post('https://example.com/testResults', JSON.stringify(data));

    return {
        'stdout': TextSummary(data),
        './results/junit.xml': JUnit(data),
        './results/summary.json': JSON.stringify(data),
    }
}

With the current state of the PR, handleSummary() is supported and its results are saved to stderr/stdout/files. I've also heavily refactored the --summary-export (implemented it through the new system in a backwards compatible way) and fixed a bunch of --sumary-trend-stats bugs and group/check order. But we don't have a JS equivalent of the old summary (TextSummary() in the example above) yet or JUnit(). For now, I've exposed the old Go code that generated the test summary as a second parameter to handleSummary(), but I'll remove that in the final version. So, to test this, you might use something like this:

export function handleSummary(data, oldCallback) {
    console.log('handleSummary() was called');
    return {
        'stdout': oldCallback(),
        'raw-data.json': JSON.stringify(data, null, 4),
    };
}

The following things remain to be done:

replace the old text summary in Go with one a JS function
write a JUnit example (JUnit Results Output #1366, Output check/threshold results in a machine-readable unit test format to publish test results in CI #1120) and publish it and the old JSON/text summary methods to jslib
make a k6-docs PR
go through the old k6 issues and see if there are other ones that should be closed or updated now that we have this

Breaking changes and other bugfixes

--no-summary now disables even --summary-export
the value for Rate metrics in the old --summary-export JSON file was was always 0, regardless of the pass/(pass+fail) ratio

na-- · 2020-12-14T10:19:50Z

Here is a more complete example that also demonstrates the fixed summary columns even in the old backwards-compatible --summary-export handling, and the fact that the wrappers can work in --compatibility-mode=base: https://gist.github.com/na--/647cbafe46abd1a0af37402577e5d251

sniku · 2020-12-16T13:57:18Z

I'm working on creating an end-of-test summary for functional/integration tests.
This will take some time, so I'll just post comments here as I work through it. I don't expect any comments back since we have holidays! 😄

The example posted above is hitting a bug/mistake in the summaryWrapperLamnbdaCode. The result is:

ERRO[0001] handleSummary() failed with error "TypeError: Value is not an object: undefined", falling back to the default summary  source=console

oldCallback should probably be passed to exportedSummaryCallback as a second argument.

https://github.com/loadimpact/k6/pull/1768/files#diff-a0d09d808d4fa4f068a2bb81b9fa180f596f46439d040f840687d559404b5cd9R99

I can fix/work around it, so it's not a problem.

imiric

Nice work! 👏

The code LGTM, besides the minor comments. I'll take another look next year ;)

Just some notes:

Strong 👍 for splitting this into separate commits/PRs. It would've been easier to review like that as well. 😅
I like the GetResolversForTrendColumns() refactor, despite of those Sink pain points. 👍

js/runner.go

stats/stats.go

todo.txt

cmd/runtime_options.go

js/runner.go

mstoykov · 2021-01-04T12:44:15Z

I have 2 concept/UX/big issues:

I think I've already expressed this - but I don't think returning an object is good, IMO it's probably better to provide some API, through one of the arguments, not an import, on how to write to stdout/files so something like:

import http from 'k6/http';
import { JUnit, TextSummary } from 'https://jslib.k6.io/k6-summary/1.0.0/index.js' // or something like that
// ...
export function handleSummary(data, api)  { // Worst name ever I know :)
    // save everything to files and send it to a server when the test finishes or gets aborted
    http.post('https://example.com/testResults', JSON.stringify(data));
    api.writeToStdout(TextSummary(data));
    // or TextSummary(api.stdout, data);
    var f = api.openFile("./results/junit.xml");
    f.write(JUnit(data));
    // or JUnit(f, data); 
}

This will (without generators that are currently not supported, last I checked) let us write big responses without being memory hungry.
All of those objects/functions should have very similar syntax - probably just write which appends data, we can add seek to the file ones at a later point if there is sufficient demand.

I haven't looked into it, but I expect there is a somewhat common "interface" for this in the JS world that can be used.

I feel like there will be a lot of cases where the test will be executed and the function will throw an exception because the test wasn't called correctly and the most common case IMO will be because the handleSummary will expect that it has p(23) but the user didn't' call it with the correct --summary-trend-stats, so it will blow up after an hour of testing.

I think it is worth trying to figure out if we can actually have a way that this is prevented. My first thought (which works great for the summary-trend-stats) is to completely change the API to something like (all names of stuff are bad and only for illustrative purposes):

import summary from "k6/summary";

summary.register(function(data) {<same as previously>}, ["p(23)"]);

Where the first argument is the function as before and the second is something that says which summaryTrendStats it wants.
This probably should be extended with other things such as ... if the user wants the data for http_req_duration with tag something=else . Especially this might be better just to be ... designed/agreed upon how it will work, but to be implemented in another PR.

edit; this second syntax also makes it easy to have multiple handleSummary functions making the different types instead of having a single one which makes all.

na-- · 2021-01-04T16:32:50Z

@mstoykov, I think we've previously discussed most of these ideas in the private issue, but to rehash things:

I don't think returning an object is good, IMO it's probably better to provide some API, through one of the arguments, not an import, on how to write to stdout/files

If you recall, having some sort of a file writing API was port of my original proposal for this functionality. I later retracted this, because that API would have been a deep and bikesheddy rabbit hole which didn't (and still doesn't) seem worth getting into. I didn't want to have to figure out a nice JS API for file opening/closing, permissions, appending, binary data, etc. This feature is big enough without having to deal with that from scratch as well...

The currently returned {filepath: contents} map is basically a simple MVP version that would likely satisfy 95+% of users. Memory usage is unlikely to be a concern, even if someone decides to generate fancy PDF or HTML reports. And if this ever proves insufficient, we can always add a file writing API at a later point, as a separate PR. The current implementation is 20 lines of simple and testable code, so it won't be a big overhead to maintain.

Besides, any file writing API will probably be better off as something generic, and better of being polished as an xk6 extension for a while, not directly in core.

cc @sniku @imiric what are your thoughts here? I am not opposed to a proper API, but that would mean delaying the feature again and not getting it in v0.30 for sure.

I feel like there will be a lot of cases where the test will be executed and the function will throw an exception because the test wasn't called correctly...

True, that's a risk, but you can say the same thing about teardown(). If you saw, I plan to add some exception handling for errors in the JS handleSummary() wrapper, so we will at least show the default summary if an error is thrown by user code.

I don't think the UX of passing the required percentiles (["p(23)"]) and other data is very good. Even the toy example looks strange, but a real-life example would be even more awkward. And having percentile APIs instead of plain data wouldn't necessarily be less error prone.

That said, I like the approach of relying on an imported module and calling a method on it (summary.register()) more than relying on exported magic functions with specific names... I am not sure it makes sense to register() multiple handlers for the end-of-test summary, though we might want to instead make this a more generic event-handling system. Something like:

import events from "k6/events";

events.registerHandler('endOfTestSummary', someHandler); // or maybe addEventListener :D 
events.registerHandler('midTestSummary', someOtherHandler);
events.registerHandler('vuInit', perVUInit); 
events.registerHandler('abortedByThreshold', whatever); 
// ...

But again, this seems like it can grow in complexity and needs careful design because of the k6 multi-JS-runtime arhitecture. It also seems like something that can be added in the future, in a backwards compatible way. So, for now, starting with an exported handleSummary function doesn't seem like a bad idea and doesn't seem like it will introduce any overhead. If anything, most of the overhead of this feature comes from the huge amount of old text summary and --summary-export cruft. Once we wrap those in handleSummary(), it should be reasonably easy to work with in the future.

imiric · 2021-01-12T10:05:38Z

I like the flexibility of @mstoykov's proposal, but also feel that it would cause a delay of v0.30.0 to implement it. Since we can add a proper API in a backwards compatible way in the future (new users simply won't have to return an object), my vote is to merge this as is and discuss and design the API proposal in a subsequent issue/PR.

I also like the event-based registration idea for summary handlers, but that also feels out of scope for this MVP.

codecov-io · 2021-01-12T10:39:57Z

Codecov Report

Merging #1768 (0838813) into master (def5ac7) will increase coverage by 0.00%.
The diff coverage is 69.30%.

@@           Coverage Diff           @@
##           master    #1768   +/-   ##
=======================================
  Coverage   71.49%   71.50%           
=======================================
  Files         180      181    +1     
  Lines       13851    13939   +88     
=======================================
+ Hits         9903     9967   +64     
- Misses       3325     3340   +15     
- Partials      623      632    +9

Flag	Coverage Δ
ubuntu	`71.48% <69.30%> (+0.03%)`	⬆️
windows	`70.12% <69.30%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/options.go	`60.46% <0.00%> (ø)`
cmd/run.go	`7.29% <0.00%> (-0.10%)`	⬇️
lib/testutils/minirunner/minirunner.go	`80.43% <0.00%> (-0.97%)`	⬇️
stats/sink.go	`100.00% <ø> (ø)`
js/common/util.go	`46.66% <27.27%> (-53.34%)`	⬇️
js/runner.go	`80.86% <68.08%> (+0.41%)`	⬆️
js/summary.go	`90.00% <90.00%> (ø)`
cmd/config.go	`88.28% <100.00%> (+0.10%)`	⬆️
stats/stats.go	`80.63% <100.00%> (+3.51%)`	⬆️
ui/summary.go	`86.39% <100.00%> (-2.96%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update def5ac7...0838813. Read the comment docs.

mstoykov · 2021-01-12T14:14:46Z

I tried (successfully I would say) port https://github.com/Mattihew/k6-to-junit to this new change here

mstoykov · 2021-01-12T14:24:34Z

cmd/run.go

-					if err := s.SummarizeMetricsJSON(f, data); err != nil {
-						logger.WithError(err).Error("failed to make summary export file")
-					}
+					logger.WithError(err).Error("failed to handle the end-of-test summary")


I guess that in a sequential release we will fallback to a default representation or the dumping the lib.Summary as JSON to the disk?

not sure what you mean by "a sequential release", but we'll get an error here only if something goes majorly wrong... Both initRunner.HandleSummary() and handleSummaryResult() try to continue working even if there are errors. handleSummaryResult() tries to save all files, even if some of them fail (because of wrong names, lack of permissions, etc.). And initRunner.HandleSummary() has internal try / catch in the JS wrapper code: https://github.com/loadimpact/k6/blob/0838813db6edead0f6faa3cdaadf3b5eb070a1ab/js/summary.go#L97-L104

Though I should add tests for both of these things...

yeah, I missed this code at the time ...

This is probably my biggest issue reviewing the PR, the code is all over the place, although I don't know how to fix that if it's at all possible given what the code tries to do, so I don't have some concrete feedback. I hope that when we rewrite even more of the whole metric and threshold (which specifically are also over the place) this might get more streamlined

mstoykov · 2021-01-12T14:36:10Z

ui/summary.go

-				switch v := resolver(sink).(type) {
-				case float64:
+				v := resolver(sink)
+				if tc != "count" { // sigh


nitpick ... but why are you checking for the reverse here instead of for the equal one? ;)

🤷‍♂️ probably just because that was the case order in the old switch, but yeah, doesn't makes sense, so I'll change it

in the JS variant of this though, no need to further mess with the Go code

mstoykov

LGTM.

I had some more comments, but even the runPart thing seems likely to be more involved with little actual benefit, so I guess it can be even be left for a later PR or not done at all.

mstoykov · 2021-01-12T15:00:51Z

js/runner.go

+		fn := exported.Get(consts.HandleSummaryFn)
+		if _, ok := goja.AssertFunction(fn); ok {
+			handleSummaryFn = fn
+		} else if fn != nil && !goja.IsUndefined(fn) && !goja.IsNull(fn) {


I checked and Get returns nil if it doesn't exist. Did you intend for exports.handleSummary = undefined; (or null) to be a valid thing that won't raise an error or were you over zealous in the checks?

I am not particularly certain it matters either way, to be honest. If someone has defined handleSummary it is probably better to use delete exports.handleSummary, on the other hand, I don't think we will be hitting it. So the question is mostly questing what was the idea behind it, not changing it one way or another

were you over zealous in the checks?

Likely that, and it makes sense to leave only the nil check 👍

mstoykov · 2021-01-12T15:08:42Z

js/runner.go

+	}
+	rawResult, _, _, err := vu.runFn(ctx, false, handleSummaryWrapper, wrapperArgs...)
+
+	// TODO: refactor the whole JS runner to avoid copy-pasting these complicated bits...


I am pretty sure you can just use runPart for most of the above.
You will need to keep the check for whether exports.handleSummary is a function, but everything else seems to be done by runPart either way ... I think ...

Although I guess you will need to move https://github.com/loadimpact/k6/blob/0838813db6edead0f6faa3cdaadf3b5eb070a1ab/js/summary.go#L95-L118 to golang code after the runPart

Yeah, I initially tried using runPart, but it was an even bigger mess. We can figure out some way to refactor the whole js.Runner internally in the future to reduce the boilerplate, but for now I don't want to mess with it more if I don't need to... 😅

imiric

LGTM, nice work!

js/runner.go

na-- changed the title ~~[WIP~~ [WIP] Summary callback changes Dec 14, 2020

na-- requested review from mstoykov, imiric and sniku December 14, 2020 09:54

imiric suggested changes Dec 23, 2020

View reviewed changes

js/runner.go Show resolved Hide resolved

stats/stats.go Outdated Show resolved Hide resolved

stats/stats.go Outdated Show resolved Hide resolved

stats/stats.go Outdated Show resolved Hide resolved

todo.txt Outdated Show resolved Hide resolved

na-- mentioned this pull request Jan 4, 2021

Document that the summary export threshold is unintuitive grafana/k6-docs#30

Closed

mstoykov reviewed Jan 4, 2021

View reviewed changes

cmd/runtime_options.go Outdated Show resolved Hide resolved

mstoykov reviewed Jan 4, 2021

View reviewed changes

js/runner.go Outdated Show resolved Hide resolved

This was referenced Jan 5, 2021

--summary-trend-stats not being used in the JSON final summary report; --summary-export #1779

Closed

Misc fixes for summary callback changes #1788

Merged

na-- force-pushed the summary-changes branch from 970122c to 6e560ca Compare January 11, 2021 09:09

na-- changed the base branch from master to misc-fixes-for-summary January 11, 2021 09:09

na-- added this to the v0.30.0 milestone Jan 12, 2021

na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jan 12, 2021

na-- force-pushed the summary-changes branch from 6e560ca to fc7e258 Compare January 12, 2021 10:39

na-- force-pushed the summary-changes branch from fc7e258 to 45ed93c Compare January 12, 2021 11:56

Base automatically changed from misc-fixes-for-summary to master January 12, 2021 13:21

na-- added 3 commits January 12, 2021 15:25

Move all end-of-test summary handling to a handleSummary() callback

11da195

Make summary tests more realistic

616fced

Add test for the raw handleSummary() data parameter

0838813

na-- force-pushed the summary-changes branch from 45ed93c to 0838813 Compare January 12, 2021 13:27

na-- requested review from imiric and mstoykov January 12, 2021 13:27

na-- changed the title ~~[WIP] Summary callback changes~~ Summary callback changes Jan 12, 2021

mstoykov reviewed Jan 12, 2021

View reviewed changes

mstoykov previously approved these changes Jan 12, 2021

View reviewed changes

This was referenced Jan 12, 2021

Add explicit tracking and ignoring of metrics and sub-metrics #1321

Open

Tutorials and examples for generating information-rich reports grafana/k6-docs#186

Open

imiric previously approved these changes Jan 12, 2021

View reviewed changes

js/runner.go Show resolved Hide resolved

Improve tests for the JS summary handling

839b0b2

na-- dismissed stale reviews from imiric and mstoykov via 839b0b2 January 13, 2021 10:05

mstoykov approved these changes Jan 13, 2021

View reviewed changes

imiric approved these changes Jan 13, 2021

View reviewed changes

imiric merged commit c33490d into master Jan 13, 2021

imiric deleted the summary-changes branch January 13, 2021 10:20

mstoykov mentioned this pull request Jan 27, 2021

Show the number of passed checks when all the checks pass grafana/jslib.k6.io#22

Open

na-- mentioned this pull request Jan 27, 2021

Add option to change how the UI is rendered #1376

Open

mstoykov mentioned this pull request Feb 15, 2021

Proposal for adding a new standard http_req_failures metric #1828

Closed

na-- mentioned this pull request Jun 8, 2021

handleSummary() support in k6 cloud grafana/k6-cloud-feature-requests#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Summary callback changes #1768

Summary callback changes #1768

na-- commented Dec 14, 2020 •

edited

Loading

na-- commented Dec 14, 2020

sniku commented Dec 16, 2020

imiric left a comment

mstoykov commented Jan 4, 2021 •

edited

Loading

na-- commented Jan 4, 2021

imiric commented Jan 12, 2021

codecov-io commented Jan 12, 2021 •

edited

Loading

mstoykov commented Jan 12, 2021

mstoykov Jan 12, 2021

na-- Jan 12, 2021 •

edited

Loading

mstoykov Jan 12, 2021

mstoykov Jan 12, 2021

na-- Jan 12, 2021

na-- Jan 12, 2021

mstoykov left a comment

mstoykov Jan 12, 2021

na-- Jan 13, 2021

mstoykov Jan 12, 2021

mstoykov Jan 12, 2021

na-- Jan 13, 2021

imiric left a comment

Summary callback changes #1768

Summary callback changes #1768

Conversation

na-- commented Dec 14, 2020 • edited Loading

Breaking changes and other bugfixes

na-- commented Dec 14, 2020

sniku commented Dec 16, 2020

imiric left a comment

Choose a reason for hiding this comment

mstoykov commented Jan 4, 2021 • edited Loading

na-- commented Jan 4, 2021

imiric commented Jan 12, 2021

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

mstoykov commented Jan 12, 2021

Choose a reason for hiding this comment

na-- Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

na-- commented Dec 14, 2020 •

edited

Loading

mstoykov commented Jan 4, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading

na-- Jan 12, 2021 •

edited

Loading