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

Wire up the local and remote persisters #1203

Merged
merged 15 commits into from
Feb 13, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Feb 1, 2024

What?

Now that we have the LocalFilePersister and RemoteFilePersister, the next step is to easily switch between the two depending on the environment configuration.

The key environmental change is the addition of K6_BROWSER_SCREENSHOTS_OUTPUT which allows us to configure the necessary settings which will enable the browser module to upload the screenshots to the cloud. If this isn't present then it will assume that the screenshots are to be persisted to the local disk.

Why?

It will allow us to easily swap out the local persister with the remote one. The design of this closely follows how it was implemented for tracing.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #1160

@ankur22 ankur22 marked this pull request as draft February 1, 2024 14:51
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 3 times, most recently from 660e5e9 to c131559 Compare February 1, 2024 15:00
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 2 times, most recently from 7125509 to 205f173 Compare February 5, 2024 15:19
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 3 times, most recently from e68c888 to cc0496d Compare February 9, 2024 17:31
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch from cc0496d to 9ea3968 Compare February 12, 2024 14:26
This refactor will enable us to swap out the LocalFilePersister and
RemotePersister. The Persist method in LocalFilePersist must match what
is a requirement for the RemotePersister, which is the addition of the
context as a param, even though the LocalFilePersister doesn't work
with it.

These two persisters will be interchangeable at a later stage depending
on the environment setup.
This env var will be used to configure the remote persister. When this
env var is available and setup correctly, it will help configure the
module to swap out the LocalFilePersister with the RemotePersister.
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch from e3e2be3 to 7a2324d Compare February 12, 2024 14:47
@ankur22 ankur22 changed the title Refactor/wire local remote persister Wire up the local and remote persisters Feb 12, 2024
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 2 times, most recently from db21b71 to 35cba6e Compare February 12, 2024 15:19
This will parse the url, basePath and headers from
K6_BROWSER_SCREENSHOTS_OUTPUT. It will also help determine whether the
module should use a RemoteFilePersister or a LocalPersister.
This method will help determine whether to use the LocalFilePersister
or the RemoteFilePersister. It mainly depends on whether a valid
K6_BROWSER_SCREENSHOTS_OUTPUT env var has been setup.
This initialise a FilePersister (either a local or remote) depending on
the environment, which is dictated by K6_BROWSER_SCREENSHOTS_OUTPUT.
Use the interface instead of the LocalFilePersister throughout the
codebase so that we can easily swap it out for a different persister.
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 3 times, most recently from b330937 to 62d1405 Compare February 12, 2024 15:34
@ankur22 ankur22 marked this pull request as ready for review February 12, 2024 15:35
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly excellent. Some suggestions.

browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister_test.go Outdated Show resolved Hide resolved
browser/module.go Outdated Show resolved Hide resolved
tests/element_handle_test.go Outdated Show resolved Hide resolved
browser/file_persister_test.go Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Outdated Show resolved Hide resolved
browser/file_persister.go Show resolved Hide resolved
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch 2 times, most recently from 1456485 to 1a01469 Compare February 13, 2024 09:54
The packages will now own their own interfaces instead of borrowing one
that was implemented in storage.
The browser package is the work horse on initialising and setting up
the environment for the global test run, iteration or vu. The decision
on whether to create a Local or Remote persister should rest in the
browser package's hands, but leave the details of what and how the two
persisters do their job in the storage package.
This type helps encapsulate and helps document what the parsed types
and usages are from the K6_BROWSER_SCREENSHOTS_OUTPUT env var.
This explains what it is doing better than parseEnvVar.
@ankur22 ankur22 force-pushed the refactor/wire-local-remote-persister branch from 250b95c to f913425 Compare February 13, 2024 11:06
inancgumus
inancgumus previously approved these changes Feb 13, 2024
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work 👏 💯

browser/file_persister_test.go Outdated Show resolved Hide resolved
Use a mock persister when there's no need for either a local or remote
persister in a test. These tests do not need to validate that a
screenshot was saved.
@ankur22 ankur22 merged commit 8497dd7 into main Feb 13, 2024
17 checks passed
@ankur22 ankur22 deleted the refactor/wire-local-remote-persister branch February 13, 2024 16:45
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