[2/2] Make k6 cloud run --local-execution upload test archive #3931

oleiade · 2024-09-04T14:13:12Z

What?

This Pull request modifies the behavior of the k6 cloud run command when using the --local-execution mode. It makes uploading the test archive to the cloud a default. This is meant to support our internal efforts to improve the UX of our cloud offering.

This Pull Request also adds support for an additional --no-archive-upload flag to the k6 cloud run command, which enables users to prohibit uploading their test archive to our cloud.

Review

Archive upload

Following a recently published change in our platform, creating test runs now supports providing the test archive along the test run parameters. To provide an archive when creating a test run, the request is expected to be a multipart form instead of the traditional JSON body. Creating test runs without archives does not change and works as before. To support this change you will find that we had to modify some of the behavior of the cloud API client CreateTestRun method.

Configuration aspects

We developed this PR hand-in-hand with @joanlopez and particularly struggled to set up its configuration aspects. We took the approach that felt the most convenient and practical to us, in lack of a better approach. If you have ideas or insights on how to handle that particular part specifically, please let us know 👍🏻

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make lint) and all checks pass.
I have run tests locally (make tests) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#3904

oleiade · 2024-09-05T14:52:39Z

Heads-up I tried to run the tests on my windows machine, and I can't reproduce the issue that's reported by the tests, if you folks have any ideas, I'll take it 🙇🏻

oleiade · 2024-09-11T08:44:03Z

The tests are now green. There is a test in the cloudapi package that's very flaky on windows because it expects log lines in a given order, and is time based.

I created a dedicated PR (#3943) to try and cater to that flakiness. Commit 4d2636d will, as a result, be dropped when merged.

In the meantime, please still consider this ready for review 🦾

cmd/cloud_run.go

This commits aligns the handling of the --no-usage-report option in the context of the k6 cloud run command with the following agreed upon behavior: * k6 cloud run --no-usage-report should ignore the flag, and make sure we don't send the report. * k6 cloud run --local-execution --no-usage-report should take the flag in consideration, and act accordingly.

Co-authored-by: Ivan <[email protected]>

mstoykov · 2024-09-12T07:52:27Z

cloudapi/api_test.go


-		exp := `{"name":"test","vus":0,"thresholds":null,"duration":0}`
-		assert.JSONEq(t, exp, string(b))
+	t.Run("creating a test run should succeed", func(t *testing.T) {


nit: I am not a big fan of having a test with subtests jsut for grouping as you can do that with only the name of the test. Additionally it doens't add a level of identation.

In this case you also move a bunch of tests into subtests which makes the reviewing harder:(

mstoykov · 2024-09-12T08:01:19Z

output/types.go

+// WithArchive is an output that can receive the archive object in order
+// to upload it to the cloud.
+type WithArchive interface {
+	Output
+	SetArchive(archive *lib.Archive)
+}
+


This is the exact thing #3282 is trying to prevent - us keep adding "abstracitons" to the Output type that only makes sense for the cloud output and don't really make it better just make it do all the work.

IMO in this case k6 cloud run --local-execution probahly shouldn't just the output interface at all and use the cloud output directly.

Preferably it will make the test before cloud output Start gets called so it won't actually call CreateTestRun, but the k6 cloud run --local-execution does.

I see your point, and I agree that it's a good idea. We actually considered a similar approach with @joanlopez. However, we encountered some challenges when trying to access the cloud output from the run entry point to handle the archiving and upload process as you described (before Start is called). Based on my understanding of your point, I think most of our implementation matches what you describe, besides indeed how we pass down "configuration" to the cloud output.

One of the reasons for this difficulty is that the cloud run command currently relies on the k6 run command internally but forces its output to cloud by overriding the loadConfiguredTest function. This setup makes it hard to achieve what you're suggesting without the workaround we've implemented here—adding an interface that the run command can detect and use to configure the cloud output.

Do you have any suggestions on how we could accomplish this (passing down a test archive to the cloud output) without the WithArchive interface and run command specific logic we've introduced?

Or, are you proposing that we should define the logic for local execution directly in the cloud run command, rather than relying on the run command? This would give us more flexibility and control, potentially avoiding changes to the output interface and the run command, as discussed in #3282, but would result in a much bigger and riskier change.

Please let me know if I’ve interpreted your suggestion correctly 🙇🏻

Unfortunately I don't have a 10 line fix that will make this possible.

As explained in the linked issue, this will only get worse and for logs AFAIK practically impossible to be done without refactoring how we run k6.

At this point it seems that the code in the "run" command will need to be refactored to be more composable and then reused in k6 cloud run --local-execution.

Looking at the cmd package it seems like it might be a good idea if the cmd packages deals more wiht the configuraiton and parsing of cli flags and less wiht actually running the k6 app. Currently both of those are very intertwined but that likely is not a good idea.

As mentioned privately I am okay with us merging this for the purposes of getting this released in 0.54.0, but will definitely prefer if work on the actual refactoring and working towards the mentioned issue goals of stop making the output interface just one big pile of one off editions and the cloud output in an omnipotent part of the code.

mstoykov · 2024-09-13T11:49:44Z

output/types.go

@@ -68,6 +68,13 @@ type WithThresholds interface {
 	SetThresholds(map[string]metrics.Thresholds)
 }

+// WithArchive is an output that can receive the archive object in order
+// to upload it to the cloud.


Suggested change

// to upload it to the cloud.

// to upload it to the cloud.

// NOTE: This API is EXPERIMENTAL and may be changed, renamed or

// completely removed in a later k6 release.

oleiade added the enhancement label Sep 4, 2024

oleiade added this to the v0.54.0 milestone Sep 4, 2024

oleiade self-assigned this Sep 4, 2024

oleiade changed the base branch from master to new_cloud_run_local_execution September 4, 2024 14:16

oleiade force-pushed the cloud_run_local_execution_upload_archive branch 2 times, most recently from a6082ee to f99b7ad Compare September 4, 2024 15:43

oleiade changed the title ~~[2/2] Implement the --local-execution mode for k6 cloud run~~ [2/2] Make k6 cloud run --local-execution upload test archive Sep 5, 2024

oleiade marked this pull request as ready for review September 5, 2024 14:52

oleiade requested a review from a team as a code owner September 5, 2024 14:52

oleiade requested review from mstoykov and joanlopez and removed request for a team September 5, 2024 14:52

Base automatically changed from new_cloud_run_local_execution to master September 10, 2024 14:37

oleiade force-pushed the cloud_run_local_execution_upload_archive branch from 044d6f2 to 4d2636d Compare September 10, 2024 15:10

oleiade mentioned this pull request Sep 11, 2024

Raise TestStreamLogsToLogger log sending delay #3943

Merged

5 tasks

mstoykov reviewed Sep 11, 2024

View reviewed changes

cmd/cloud_run.go Outdated Show resolved Hide resolved

oleiade and others added 12 commits September 11, 2024 15:05

Implement the --local-execution mode for k6 cloud run

e8f6b2a

Fix tests

73d8155

Update cmd/cloud_run.go

cfd8fa7

Co-authored-by: Ivan <[email protected]>

Apply pull request review suggestions

dffb25f

Apply pull request review suggestions

d3ec23c

Add NoArchiveUpload to run command config

af610f6

Add support for archive upload to the cloud output

6592337

Add support for creating test run with and archive to cloud api client

6bac68c

Add --no-archive-upload option to k6 cloud run

01a1a16

Add tests asserting the behavior of the --no-archive-upload flag

0a2437c

Apply pull request review suggestions

b179a5d

oleiade force-pushed the cloud_run_local_execution_upload_archive branch from 4f26cbe to b179a5d Compare September 11, 2024 13:05

mstoykov reviewed Sep 12, 2024

View reviewed changes

mstoykov reviewed Sep 13, 2024

View reviewed changes

mstoykov approved these changes Sep 13, 2024

View reviewed changes

olegbespalov approved these changes Sep 13, 2024

View reviewed changes

oleiade merged commit 31e3db7 into master Sep 13, 2024
22 checks passed

oleiade deleted the cloud_run_local_execution_upload_archive branch September 13, 2024 17:08

BrewTestBot mentioned this pull request Sep 30, 2024

k6 0.54.0 Homebrew/homebrew-core#192373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/2] Make k6 cloud run --local-execution upload test archive #3931

[2/2] Make k6 cloud run --local-execution upload test archive #3931

oleiade commented Sep 4, 2024 •

edited

Loading

oleiade commented Sep 5, 2024

oleiade commented Sep 11, 2024 •

edited

Loading

mstoykov Sep 12, 2024

mstoykov Sep 12, 2024

oleiade Sep 12, 2024

mstoykov Sep 13, 2024

mstoykov Sep 13, 2024

[2/2] Make k6 cloud run --local-execution upload test archive #3931

[2/2] Make k6 cloud run --local-execution upload test archive #3931

Conversation

oleiade commented Sep 4, 2024 • edited Loading

What?

Review

Archive upload

Configuration aspects

Checklist

Related PR(s)/Issue(s)

oleiade commented Sep 5, 2024

oleiade commented Sep 11, 2024 • edited Loading

mstoykov Sep 12, 2024

Choose a reason for hiding this comment

mstoykov Sep 12, 2024

Choose a reason for hiding this comment

oleiade Sep 12, 2024

Choose a reason for hiding this comment

mstoykov Sep 13, 2024

Choose a reason for hiding this comment

mstoykov Sep 13, 2024

Choose a reason for hiding this comment

oleiade commented Sep 4, 2024 •

edited

Loading

oleiade commented Sep 11, 2024 •

edited

Loading