Parse the scenarios json from the K6_INSTANCE_SCENARIOS env var
#906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
changed the title
ankur22
changed the title
ankur22
force-pushed
the
add/scenario-json-parsing
branch
2 times, most recently
from
cf90f3a
to
6893d12
Compare
ankur22
changed the title
K6_INSTANCE_SCENARIOS env var
inancgumus
approved these changes
May 26, 2023
There was a problem hiding this comment.
LGTM 👍 Thanks!
We're panicking in New() *RootModule. And, it might not align with the expectations of the k6-core about how a module should behave when constructing it.
One suggestion to improve the situation is to handle initializations in NewModuleInstance where we have a VU. This way, we can handle any unexpected issues properly—with a k6 panic. To implement this, we can make use of sync.Once for these global variables.
It might be unnecessary now. We can address this concern later on.
Squashing this PR might help. For example, "Add Some" 😄
Yeah, that's something I wasn't sure of, so I just used
🤦 thanks! |
This new method parses K6_BROWSER_WS_URL differently. Instead of a list of ws urls, it is a scenarios object, which needs to be unmarshalled and all ws urls extracted. The end result is the same -- a list of ws urls. This is a step closer to being able to work with a set of chrome browsers each with different characteristics that are set in the scenario. This commit contains the addition of this new method. Eventually this method will be used instead of the existing newRemoteRegistry method. Closes: #888
The scenarios will be set on the K6_INSTANCE_SCENARIOS env var instead of the K6_BROWSER_WS_URL. We feel that K6_BROWSER_WS_URL might still be useful for debugging in the short term, but it's worth noting that it doesn't provide the flexibility and granularity of the new env var.
This refactor will enable us to read from K6_BROWSER_WS_URL from the new method in a future commit. This will eventually allow us to retrieve both the values from K6_BROWSER_WS_URL and K6_INSTANCE_SCENARIOS from the new method.
This commit now works with the newly created checkForBrowserWSURLs in newRemoteRegistry.
This change the signature of the method to return the raw values. This will eventually allow the newRemoteRegistry method to pick from this set of raw values and the ones from checkForBrowserWSURLs.
We need to accommodate two reads from the environment (so two env vars) . This changes the test to set and read from the env.
Add the checkForScenarios before checkForBrowserWSURLs in newRemoteRegistry. The scenarios take precedence over the browser ws urls.
We're now implicitly testing checkForScenarios through the newRemoteRegistry tests.
If scenarios cannot be parsed an error is returned. It needs to be returned to the caller to handle. At the moment we should panic and stop the test for running if we're unable to parse the scenarios.
This test just ensures that K6_INSTANCE_SCENARIOS values are used instead of K6_BROWSER_WS_URL, if both are defined in the env.
Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>
ankur22
force-pushed
the
add/scenario-json-parsing
branch
from
00da9ca
to
78d971a
Compare
Since creating a remoteRegistry now errors, we need to be able to send a signal back to k6 to let it know to shut down cleanly. At the moment there is no way to do this through New. We can achieve this with a workaround in NewModuleInstance though. Since NewModuleInstance is called once per vu and remoteRegistry only needs to be created once, we have to use sync.Once to init a new remoteRegistry once which is then used by all vus.
ankur22
force-pushed
the
add/scenario-json-parsing
branch
from
b7c2f59
to
1221193
Compare
inancgumus
reviewed
May 29, 2023
inancgumus
added a commit
that referenced
this pull request
Jun 1, 2023
inancgumus
added a commit
that referenced
this pull request
Jun 2, 2023
inancgumus
added a commit
that referenced
this pull request
Jun 2, 2023
inancgumus
added
remote
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
newRemoteRegistrynow needs to parse two env vars. The first is the one that we have already defined,K6_BROWSER_WS_URL, the new one isK6_INSTANCE_SCENARIOS.K6_INSTANCE_SCENARIOSdefines a JSON object of the form:[ { "id": "one", "browsers": [ { "handle": "ws://1..." } ] }, ]Each scenario will generally point to a unique ws url, which is from a unique instance of chrome. Currently we are only working with chrome, and all chrome instances are setup in the same way. In the future we will want to be able to launch different chrome instances each with their own unique characteristics as well as other web browsers.
K6_INSTANCE_SCENARIOStakes precedence overK6_BROWSER_WS_URL.K6_BROWSER_WS_URLcould still be useful for debugging purposes.Closes: #888
Checklist
Tasks