k6 icon indicating copy to clipboard operation
k6 copied to clipboard

[WIP] Proof of concept changes (test suites, HDR histograms)

Open na-- opened this issue 3 years ago • 5 comments

This is a repository for unpolished PoC changes before they can be split off in their own PRs. The commits here won't be merged without significant refactoring and changes and will probably be larger than average, to simplify rebases. It is the second iteration of such a PoC PR, the original PoC for distributed execution was in https://github.com/grafana/k6/pull/2438, see https://github.com/grafana/k6/pull/2438#issuecomment-1344034591 for details.

As of right now, this PR contains rudimentary, hackish and very janky implementations of test suites (https://github.com/grafana/k6/issues/1342) and HDR histograms (https://github.com/grafana/k6/issues/763) :tada: It used to contain a distributed execution (https://github.com/grafana/k6/issues/140) PoC too, but that got split off into separate PRs, see https://github.com/grafana/k6/pull/2816#issuecomment-1642271288.

na-- avatar Dec 09 '22 08:12 na--

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (metrics-in-distributed-execution@3363208). Click here to learn what that means.

:exclamation: Current head 79a9337 differs from pull request most recent head 1ad356b. Consider uploading reports for the commit 1ad356b to get more accurate results

Additional details and impacted files
@@                         Coverage Diff                         @@
##             metrics-in-distributed-execution    #2816   +/-   ##
===================================================================
  Coverage                                    ?   70.19%           
===================================================================
  Files                                       ?      276           
  Lines                                       ?    21061           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?    14783           
  Misses                                      ?     5344           
  Partials                                    ?      934           
Flag Coverage Δ
ubuntu 70.14% <0.00%> (?)
windows 70.04% <0.00%> (?)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Dec 09 '22 08:12 codecov-commenter

Rebased this on top of the latest master, there were some significant changes and a lot of merge conflicts since the days of merging https://github.com/grafana/k6/pull/2815 (and its preceding chain of PRs)

na-- avatar Jun 06 '23 13:06 na--

Over the last few days I split the previous 3 big commits here into 10 smaller, self-contained and atomic commits. That way some of the backwards-compatible commits can be merged into master now, while rebasing the rest on top of any master changes can also be easier. I've submitted one PR (https://github.com/grafana/k6/pull/3191) based on the first 4 purely refactoring commits, and will probably submit at least one more with the next 2 (no-op local execution.Controller) after it is merged.

Even more importantly, I moved the only commit that had somewhat breaking changes (the PoC for HDR histograms, https://github.com/grafana/k6/issues/763) to be the last one in the chain, after test suites even! They aren't really even breaking changes, but because it makes the threshold and end-of-test summary calculations slightly less accurate, more consideration should certainly be taken before we switch to a hastily picked algorithm. Some tests that expect exact results will also need to be adjusted...

However, having HDR histograms last means that, theoretically, the other commits here, up to and including distributed execution and metric support for it (these are now 2 separate commits! :tada:), can safely be merged into master, because they are completely backwards compatible! :tada: The only obstacle is some minor issues around xk6-output-prometheus-remote issues that need to be worked out (see this and this), though that should be easy to do with a few minor modifications of the extension and a new version built on top of https://github.com/grafana/k6/pull/3191...

So yeah, while the distributed execution implementation here is poorly performant, untested and "optimistic" (basically works only for the happy path and doesn't handle errors well), it is completely backwards compatible now! :tada: It should be safe to merge it as it is, it shouldn't affect anything else - notice how no existing tests were disabled or even changed! Since the k6 agent and k6 coordinator sub-commands are currently hidden, they can be considered super-experimental for a long while, until all of the remaining kinks are worked out.

The important bit is that they can be part of the codebase while this happens, so merge conflicts don't need to constantly be resolved when rebasing... And any improvements can be made iteratively, as regular PRs to master.

na-- avatar Jul 14 '23 08:07 na--

Because of https://github.com/grafana/k6/pull/2816#issuecomment-1635466149, I was able to move the distributed execution PoC into separate small and atomic PRs (https://github.com/grafana/k6/pull/3191, https://github.com/grafana/k6/pull/3204, https://github.com/grafana/k6/pull/3205, https://github.com/grafana/k6/pull/3210, https://github.com/grafana/k6/pull/3213). Even though they are still very far from polished or production-ready, they should be safe to merge into the k6 codebase because they are completely backwards-compatible.

So, as long as we are clear that we can make breaking changes in the k6 agent or k6 coordinator commands themselves (up to and including completely abandoning that approach to distributed execution and removing the commands at a later point in time!), it should be fine to merge them as they are and tackle the remaining work separately.

That's why I've adjusted the root branch of this PR to point to the branch of https://github.com/grafana/k6/pull/3213. That way everything should hopefully be easier to review and manage :crossed_fingers: The only thing that remains here is for me to publish another PR with a design doc that explains all of these things, as well as a new tracking issue for this specific approach to distributed execution. Both of these things should happen later today or tomorrow :crossed_fingers:

Again, all of this is still very much a proof of concept and considered highly experimental. Even if some of it ends up being merged, all of it can be changed at any given future point until it is officially deemed stable by the k6 team.

na-- avatar Jul 19 '23 15:07 na--

I've added the design doc (https://github.com/grafana/k6/pull/3217) and a tracking issue for the proposal (https://github.com/grafana/k6/issues/3218), which are the final bits of work I intend to do here so that someone else picks this up.

na-- avatar Jul 20 '23 10:07 na--