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

Configuration issues #883

Open
na-- opened this issue Dec 19, 2018 · 20 comments
Open

Configuration issues #883

na-- opened this issue Dec 19, 2018 · 20 comments
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio refactor

Comments

@na--
Copy link
Member

na-- commented Dec 19, 2018

This is meant to be a tracking ("epic") issue that gathers all of the deficiencies and pain points in the current k6 configuration. Individual issues, whenever possible, should likely be tackled in their own separate issues and pull requests, unless at some point we decide it'd just be easier to overhaul everything remaining at once...

I'll start with a description of how the configuration works today AFAIK, and its shortcomings. Since things are quite complicated, I'll likely miss some aspects of it, so we should update the description or add comments in the issue when we notice something else amiss. We also have to decide how we want the configuration to behave in the future, and make the changes so we get from here to there... Maybe not immediately, but the current situation is untenable in the long run.

I'll start with the current state. Generally, users can configure k6 with a mix of 4 different option sources - CLI flags, environment variables, exported script options and global JSON config. They are hierarchical, so as described here , CLI flags can override environment variables, which can override script options, which can override JSON config options, which can override default values .

There are various issues and complications with the actual implementation though:

  • Adding a new option often necessitates changes in at least 4 different places in the k6 code (here, here, here, here, and where we actually use it...)
  • Setting default values for options is very illogical, it mostly happens the CLI flags, like this... unless it doesn't, like here or here...
  • Incompatibilities in the libraries we use (envconfig especially, due to this and this issues)
  • Null-able type incompatibilities, especially with serialization and deserialization - we use a mix of this null library and our own types, but occasional incompatibility issues pop up and the whole thing isn't very well tested
  • Different value formats in different config sources (for example, stages are configured quite differently with the JS options and the CLI flags)
  • Some options have no CLI flag (example + all collector/output options) or have only CLI and env vars options (i.e. no JS ones, even when it makes sense, like these)or can only be enabled with a flag (all outputs), etc.
  • Different k6 subcommands have different option sets (example)
  • Serialization and deserialization issues in general, especially when we need the result to be human-readable (eg. in the HAR converter we want to emit some consolidated options in a human-readable format, so without all of the null values)
  • We can actually use environment variables (tier 2) to configure in-script options (which are on the lower tier 3!) - that's cool and very often useful, but with the current architecture it's also a bit complicated
  • Validation of different options is very piecemeal and quite insufficient (case in point)
  • Various other individual issues like this
  • etc., etc... (I'm sure that I'm missing a lot of corner cases)

Also, a lot of the issues with the current config implementation are only catch-able during runtime. That's party a result of Go's relatively weak type system, but I'm sure it could be improved upon. And on top of the weak compile-time assurances, the behavior (default value setting, overriding, serialization, de-serialization, etc.) of different config entity types is difficult to test individually, since they're part of huge structs that also contain part of the behavior (esp. overriding and default value management). That makes the current implementation quite error prone, and unfortunately, the k6 users are usually the first ones to notice that something doesn't work correctly 😒

Back on the topic of specific configuration issues, by far the biggest problem are the options for collectors (i.e. outputs), and by far the biggest problem from them is actually the Load Impact cloud options... They break the aforementioned nice "4 config tiers" picture in so many ways it's actually difficult to know if I can list them all or consider them corner cases...

First, the general output issues are mostly described in this github issue: #587.
In short:

  • Outputs can be defined only via the CLI flags (i.e. k6 run -o cloud) and can be 0, 1, or more than 1 output, even from the same type
  • Some of them can be configured via CLI flags (eg. k6 run -o json=results.json -o influxdb=http://localhost:8086/test script.js), while others, like the cloud one, cannot...
  • All of them can also be configured (but not initialized!) via environment variables or the global JSON config (though as mentioned in the issue above, not when there are multiple outputs of the same type)
  • All outputs except the cloud one cannot be configured from the exported script options. The collectors configuration is outside of the lib.Options struct we use to store all of the JS-exported options, so it's only applicable for the JSON config, not the exported script options...

All of the above issues apply to the cloud options as well, but as mentioned, they're "special" for at least two other reasons...

The first reason is that we actually reuse the same configuration for both cloud execution ( k6 cloud script.js ) and for outputting stuff to the cloud ingest ( k6 run -o cloud script.js ). That makes some sense, since both operations have common options (token, projectID and name at the very least), but at the same time it makes everything super tightly-coupled, complicated and corner-case prone.

The second issue is that we actually can configure the cloud collector from the exported script options. Only, we don't do it in some sane uniform way, instead we have that special snowflake ext.loadimpact, where we can put stuff that affects both the cloud collector and the cloud ingest output... And since need to bundle the script options in the tar archive's metadata.json when we run k6 cloud, and that metadata.json file contains only the lib.Options instead of the full config map, k6 actually needs to populate stuff in that special ext.loadimpact snowflake as well as read stuff from it. So on top of everything, it's also kind of outside of the hierarchy of all of the other config sources. And on top of that, ext.loadimpact also contains stuff that k6 doesn't know about (like distribution) 😅

Add to the whole mess that the commands k6 run script.js, k6 run archive.tar, k6 archive script.js, k6 cloud script.js, and k6 cloud archive.tar build their configuration and behave in subtly different ways, and we have a great recipe for endless corner cases and annoying bugs...

So yeah, the k6 configuration is huge, multi-faceted pile of... technical debt 🙂 Any ideas and suggestions are quite welcome at this point, or even reports of other configuration issues that weren't mentioned above 😄

@na--
Copy link
Member Author

na-- commented Dec 19, 2018

And all of the issues in the wall of test above kind of ignore that the purpose of refactoring the configuration isn't just to fix any current bugs, but also to allow us to more easily and correctly develop new k6 features. Here are some that would heavily depend on a better configuration paradigm:

@na--
Copy link
Member Author

na-- commented Dec 27, 2018

A new issue that popped up and is likely a result of the complexity of the current configuration paradigm (and a lack of testing on my part 😑 ): #886

@na--
Copy link
Member Author

na-- commented Feb 1, 2019

As mentioned in this PR comment, some complex types don't have a good way to specify if they were set by the user, or if their default values were used

@na--
Copy link
Member Author

na-- commented Mar 1, 2019

Something else that's problematic with the current state is how the JSON configuration works, it has a few... peculiarities...

For some reason the CLI --config / -c flag is defined twice: once in the global root command here and once in configFileFlagSet() (used only in k6 archive and k6 run apparently) here. The first one's value is saved in cfgFile, which is never used in the k6 code. The second one's value is saved in configFile, which is used. But the in the CLI help (k6 -h), only the first one is shown 😕 And it seems that setting the config file to a custom location works mostly accidentally, and doesn't work for k6 cloud at all?

Compounded to that, this whole part of the k6 codebase is almost completely untested and untestable. One of the reasons is the use of the configdir library, which doesn't work with the afero.Fs abstractions we use. On top of all of that, parts of the code are misleading - the readDiskConfig() function receives an afero.Fs instance, but .... it doesn't use it at all 😑

na-- added a commit that referenced this issue Mar 1, 2019
This is woefully insufficient, but even that was painful to write... Further refactoring and tests will follow, especially relating to JSON/JS configs and mixing configuration between the CLI/env-vars/JS/JSON layers. The effort will hopefully be worth it, since we should be able to reuse these tests to verify that we're not breaking things when we finally tackle #883 properly.
na-- added a commit that referenced this issue Mar 1, 2019
This is woefully insufficient, but even that was painful to write... Further refactoring and tests will follow, especially relating to JSON/JS configs and mixing configuration between the CLI/env-vars/JS/JSON layers. The effort will hopefully be worth it, since we should be able to reuse these tests to verify that we're not breaking things when we finally tackle #883 properly.
@na--
Copy link
Member Author

na-- commented Mar 5, 2019

Some more details on current config issues with the CLI flags and environment variables: #935 (comment)

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Apr 10, 2019
@na--
Copy link
Member Author

na-- commented Apr 10, 2019

Something else that's problematic with the current configuration "paradigm" is the unserializable nature of the default values for different options.

Say we have a test with 10s for duration. k6 archive script.js will produce a .tar archive with the following metatada.json contents:

{
  "type": "js",
  "options": {
    "paused": null,
    "vus": null,
    "vusMax": null,
    "duration": "10s",
    "iterations": null,
    "stages": null,
    "execution": {
      "default": {
        "type": "constant-looping-vus",
        "startTime": null,
        "interruptible": null,
        "iterationTimeout": null,
        "env": null,
        "exec": null,
        "vus": null,
        "duration": "10s"
      }
    },
    "setupTimeout": null,
    "teardownTimeout": null,
    "rps": null,
    "maxRedirects": null,
    "userAgent": null,
    "batch": null,
    "batchPerHost": null,
    "httpDebug": null,
    "insecureSkipTLSVerify": null,
    "tlsCipherSuites": null,
    "tlsVersion": {
      "min": "",
      "max": ""
    },
    "tlsAuth": null,
    "throw": null,
    "thresholds": null,
    "blacklistIPs": null,
    "hosts": null,
    "noConnectionReuse": null,
    "noVUConnectionReuse": null,
    "minIterationDuration": null,
    "ext": null,
    "summaryTrendStats": null,
    "summaryTimeUnit": null,
    "systemTags": [
      "subproto",
      "name",
      "group",
      "check",
      "proto",
      "method",
      "url",
      "error",
      "error_code",
      "tls_version",
      "status"
    ],
    "tags": null,
    "metricSamplesBufferSize": null,
    "noCookiesReset": null,
    "discardResponseBodies": null
  },
  "filename": "/home/nobody/tmp.js",
  "pwd": "/home/nobody",
  "env": {}
}

Default values are generally marked as null. systemTags and tlsVersion are exceptions because of limitations of the Go type system, another minor issue in this epic... In any case, nowhere in this json does it say that k6 should execute this archive with 1 VU. That would happen simply because 1 is the default values for vus in k6 run. But that kind of means we should never, ever change those default values.

And while vus is an extreme example of an option the default value for which we'll likely never change, the same cannot be said about things like setupTimeout, batch, iterationTimeout, etc. Their default values should probably be saved here. On the other hand, for userAgent, we probably want to specify only user-defined values, not the default value... I don't have a good solution for these issues, just stating the problem as another piece of the puzzle.

@na--
Copy link
Member Author

na-- commented Jun 12, 2019

As seen in #1045 (comment), there's a mismatch in the way we specify some of the HTTP transport and client options... Currently, the only way to specify them is globally, for the whole test run. That's very restrictive and not really necessary, since even now, each VU has its own http.Transport, but there's no way to configure it independently.

So, my suggestion there to have a way for users to configure custom HTTP clients, but also have that same configuration be available globally, would probably cover all use cases in a reasonably user-friendly and performant way. Unfortunately, it's likely going to require deprecating some of the current global options... That's a good thing in the long term, but I'm mentioning it here since it's definitely going to be a complication in the short to mid-term (relative to when we decide to implement it) with regards to the current configuration mess...

@imiric
Copy link
Contributor

imiric commented Sep 26, 2019

We should definitely break this issue apart into several actionable ones, otherwise a complete overhaul towards a "perfect" configuration system would take months. :)

I stumbled upon some of the limitations of the current config system while working on #1049. Essentially, it was very difficult to pass a new option down to the JS compiler, without having to specify it multiple times, dealing with the overriding behavior edge cases, and adding more technical debt in the process.

So I took a stab at implementing a friendlier API to make this easier and hopefully fix some of the issues mentioned here. See the very rough PoC here.

In summary, the API would look something like:

// k6/config.go
package config

// Mostly the current cmd.Config
type Config struct {...}

type Getter func() (Config, error)

func FromEnv() Getter {...}

func FromCLI(flags *pflag.FlagSet) Getter {...}

func FromFile(fs afero.Fs, path string) Getter {...}

// Or we could just use conf.Apply() for scripts
func FromScript(opts lib.Options) Getter {...}

// Analog to the current `getConsolidatedConfig`
func Get(configGetters ...Getter) (conf Config, err error) {...}

Usage:

import "k6/config"

...
     conf, err := config.Get(config.FromFile(afero.NewOsFs(), os.Getenv("K6_CONFIG")),
         configFromCli, config.FromEnv())
...

Because of variadic args for config.Get(), its usage makes it immediately clear the order options are overridden in, and makes the consolidation flexible (e.g. only read env or CLI args). (The current configFromCli lambda is to avoid having to move cmd/{config.go,options.go} which is heavily cmd-centric, but we probably should move it to k6/config as well.)

The major ideas of this approach are:

  • create config.Config{} once, use it for all configuration, and only pass it to each non-cmd package that needs it. The nullable values in config.Config should be destructured and passed as clean values to sub-packages (i.e. js/compiler shouldn't see config.Config).

  • abandon lib.RuntimeOptions and possibly lib.Options? lib.RuntimeOptions is a wrapper around os.Environ() and doesn't need to be separate from config.Config. I was thinking about making "source" fields on config.Config (e.g. Config.sourceFilePath, Config.sourceEnv, Config.sourceFlags) since RuntimeOptions.Env is passed to the JS runtime. lib.Options is trickier since it's mostly meant for test runtime options, so not sure if we should leave this as is or merge it with config.Config.

I realize this is a far cry from fixing everything mentioned in this issue, but it could solve some immediate pain points, and get the ball rolling in the right direction for others. Let me know what you think.

@josephwoodward
Copy link
Contributor

josephwoodward commented Oct 4, 2021

I'd like to add another "nice to have" here, which is that whilst retiring a soon to be deprecated command line argument (see #2150) I need to be able to emit a warning to logs informing users that they're using a soon to be deprecated flag and that they should migrate to the new one. Currently there is no means of logging during parsing configuration.

@yorugac
Copy link
Contributor

yorugac commented Nov 29, 2021

Adding one more todo-point for the configuration-related changes. Currently, k6 archive, k6 run and k6 cloud all support runtime flags and options flags. However, k6 run also supports the so-called config flags which are not accepted by k6 archive and k6 cloud:

	flags.StringArrayP("out", "o", []string{}, "`uri` for an external metrics database")
	flags.BoolP("linger", "l", false, "keep the API server alive past test end")
	flags.Bool("no-usage-report", false, "don't send anonymous stats to the developers")

So TODO is to move those options and perhaps fully discard config flags as a separate set: It's likely that k6 cloud and k6 archive will support --out option in the future too while --linger and --no-usage-report can be simply moved to runtime flags as they're not needed in init context.
Then k6 archive, k6 cloud and k6 run would all accept the same set of flags with k6 cloud accepting its own additional flags.

This is related to the issues partially described in grafana/k6-operator#9 (comment)

imiric pushed a commit that referenced this issue Apr 26, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

It doesn't try to enable generic options for any JS module as #3000 did,
but it seems Ned abandoned this idea. We can open this discussion again
once #883 is resolved.
imiric pushed a commit that referenced this issue Apr 26, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

It doesn't try to enable generic options for any JS module as #3000 did,
but it seems Ned abandoned this idea. We can open this discussion again
once #883 is resolved.
imiric pushed a commit that referenced this issue Apr 28, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

It doesn't try to enable generic options for any JS module as #3000 did,
but it seems Ned abandoned this idea. We can open this discussion again
once #883 is resolved.
imiric pushed a commit that referenced this issue Apr 28, 2023
This is a quick and possibly naive change to support browser options per
scenario (#3022).

It doesn't try to enable generic options for any JS module as #3000 did,
but it seems Ned abandoned this idea. We can open this discussion again
once #883 is resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio refactor
Projects
None yet
Development

No branches or pull requests

5 participants