Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 2 commits into
base: metrics-in-distributed-execution
Choose a base branch
from

Conversation

na--
Copy link
Member

@na-- na-- commented Dec 9, 2022

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 #2438, see #2438 (comment) for details.

As of right now, this PR contains rudimentary, hackish and very janky implementations of test suites (#1342) and HDR histograms (#763) 🎉 It used to contain a distributed execution (#140) PoC too, but that got split off into separate PRs, see #2816 (comment).

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@na-- na-- force-pushed the proofs-of-concept branch 2 times, most recently from aded156 to e8d93ec Compare December 9, 2022 20:46
@na-- na-- force-pushed the remove-the-engine branch 2 times, most recently from 4dd18e5 to 2e4f071 Compare January 20, 2023 20:45
@na-- na-- force-pushed the remove-the-engine branch 6 times, most recently from ee62441 to 943b1e7 Compare January 30, 2023 18:04
@na-- na-- mentioned this pull request Jan 31, 2023
Base automatically changed from remove-the-engine to master February 2, 2023 12:05
@na-- na-- mentioned this pull request Mar 8, 2023
@na--
Copy link
Member Author

na-- commented Jun 6, 2023

Rebased this on top of the latest master, there were some significant changes and a lot of merge conflicts since the days of merging #2815 (and its preceding chain of PRs)

@na-- na-- force-pushed the proofs-of-concept branch 3 times, most recently from 20802ee to 011397f Compare July 13, 2023 08:52
@na-- na-- force-pushed the proofs-of-concept branch 3 times, most recently from 9f1d496 to e15ca80 Compare July 13, 2023 20:45
@na--
Copy link
Member Author

na-- commented Jul 14, 2023

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 (#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, #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! 🎉), can safely be merged into master, because they are completely backwards compatible! 🎉 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 #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! 🎉 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.

@mstoykov mstoykov mentioned this pull request Jul 14, 2023
@na-- na-- force-pushed the proofs-of-concept branch 3 times, most recently from 66bda38 to 4997f6d Compare July 17, 2023 15:03
@na-- na-- force-pushed the proofs-of-concept branch 3 times, most recently from 529546e to 6071fb5 Compare July 19, 2023 14:13
@na-- na-- changed the base branch from master to metrics-in-distributed-execution July 19, 2023 15:00
@na--
Copy link
Member Author

na-- commented Jul 19, 2023

Because of #2816 (comment), I was able to move the distributed execution PoC into separate small and atomic PRs (#3191, #3204, #3205, #3210, #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 #3213. That way everything should hopefully be easier to review and manage 🤞 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 🤞

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-- na-- changed the title [WIP] Proof of concept changes [WIP] Proof of concept changes (test suites, HDR histograms) Jul 19, 2023
@na--
Copy link
Member Author

na-- commented Jul 20, 2023

I've added the design doc (#3217) and a tracking issue for the proposal (#3218), which are the final bits of work I intend to do here so that someone else picks this up.

This is a proof-of-concept for how we can use HDR/Sparse histograms for k6 Trend metrics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants