xk6 plugin integration

[imiric]

This is a proposal for an external plugin system for k6, inspired by xcaddy. See also the xk6 repo and the SQL plugin adapted from @mostafa's Go plugin version.

Initially plugins are only meant for external JS modules, but support for other plugin types (output, etc.) is likely to be added in the future.

The module support in (x)caddy is much broader in scope than what's currently in this PoC. The main distinction is that Caddy modules can recursively load other modules, and only the "guest"/"child" modules are referred to as "plugins". See the documentation.

Usage

To build a k6 binary with the SQL plugin, from the xk6 repo @ 11baed0 run:

CGO_ENABLED=1 go run ./cmd/xk6/main.go build feat/1353-xk6-plugins \
  --with github.com/imiric/k6-plugin-sql

CGO_ENABLED=1 is required for the SQL dependencies.

With the following script.js:

import { check } from 'k6';
import sql from 'k6/x/sql';

export default function () {
  let db = sql.open("sqlite3", "./test.sqlite");
  db.exec(`CREATE TABLE IF NOT EXISTS keyvalues (
        id integer PRIMARY KEY AUTOINCREMENT,
        key varchar NOT NULL,
        value varchar);`);
  db.exec("INSERT INTO keyvalues (key, value) VALUES('plugin-name', 'k6-plugin-sql');");
  let results = sql.query(db, "SELECT * FROM keyvalues;");
  for (const row of results) {
    console.log(`key: ${row.key}, value: ${row.value}`);
  }
  db.close();
}

Running ./k6 run script.js should create a test.sqlite DB and show INFO[0000] key: plugin-name, value: k6-plugin-sql source=console.

Closes #1353

[codecov-io]

Codecov Report

Merging #1688 into master will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master    #1688   +/-   ##
=======================================
  Coverage   71.42%   71.43%           
=======================================
  Files         176      177    +1     
  Lines       13681    13698   +17     
=======================================
+ Hits         9772     9785   +13     
- Misses       3298     3301    +3     
- Partials      611      612    +1     

Flag	Coverage Δ
ubuntu	71.38% <90.00%> (+<0.01%)	⬆️
windows	70.00% <90.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/internal/modules/modules.go	75.00% <75.00%> (ø)
js/initcontext.go	92.13% <100.00%> (ø)
js/modules/k6/crypto/crypto.go	97.19% <100.00%> (+0.02%)	⬆️
js/modules/k6/crypto/x509/x509.go	98.05% <100.00%> (+0.01%)	⬆️
js/modules/k6/encoding/encoding.go	93.10% <100.00%> (+0.24%)	⬆️
js/modules/k6/grpc/grpc.go	100.00% <100.00%> (ø)
js/modules/k6/html/html.go	81.95% <100.00%> (+0.08%)	⬆️
js/modules/k6/http/http.go	100.00% <100.00%> (ø)
js/modules/k6/k6.go	100.00% <100.00%> (ø)
js/modules/k6/metrics/metrics.go	92.59% <100.00%> (+0.28%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c51095a...6b7fe83. Read the comment docs.

[mstoykov]

From my initial look at this I think ... that 95% of this is not needed.
I was more thinking of just adding a function that changes the current module map and use that in the plugins.
I would even argue that it shouldn't check (at least for now) that someone tries to add "totally/not/valid/path" as ... well that is their problem ;)

This whole thing looks like it tries to be way too complex for what we want from the current js plugins for no added benefit but more code that the "plugins" need to write and it is slightly harder for them to just be copied in k6 once we decided they should be part of it.

Additionally I am pretty sure the sql module has a k6Module function in js with this code, which obviously can be fixed in the bridge or through some other means ... but IMO is again .. not needed

[imiric]

Thanks @mstoykov. This is already a cut-down version of Caddy plugins, but I left what's currently here as open design questions since we didn't really spec out the feature past "do what xcaddy does" 😄

You're right that Caddy modules/plugins are much broader in scope than what we need, but I don't think there's much complexity here, it's mostly just namespace handling and getters.

OK, so to summarize:

• Get rid of namespaces, we don't need them? I still feel they're a good idea if we expect this ecosystem to grow, and adding them in later after some plugins are written will be more work than having them in place since v1.

• Get rid of the Module interface and requirement for plugins to implement it. Plugins will simply be added to the js.modules.Index map in RegisterModule(). I totally missed that k6Module would be exposed in JS, thanks.

On the topic of naming, should we abandon the "module" naming used by Caddy and use "plugin" instead? Caddy's naming is a bit confusing, but given the additional scope "modules" makes more sense in their case, whereas we could be fine with referring to them as "plugins" altogether.

[na--]

Diclaimer: this is a very preliminary comment, since I have only skimmed this PR and taken a glance at xk6. But, from what I can see, I will probably agree with @mstoykov that this is unnecessarily complex.

Take for example the Provisioner interface. It probably makes total sense in the context of caddy, but in k6, do we really need it, when users can call arbitrary plugin functions from JS? If a plugin needs some setup, can't it be handled manually, like this:

import plugin from 'k6/x/awesome-plugin';

plugin.provision() // maybe call this in setup() even?

Instead of Validate(), what is stopping the plugin from doing validation when its module is loaded in the script? And while I can see the rationale behind CleanerUpper, again, for the first iteration why can't that be handled as a method call in teardown() or something like that.

I think it's still early to tell exactly how the final k6 plugin API should look like, so the more minimal it is right now, the fewer breaking changes we'll have to make in the future. All of these interfaces are optional extra features that we can add in the future, in a backwards-compatible way, right? So we should probably skip them, for now, and add them properly when we see a clear need for them.

Adopting namespaces would also be useful to distinguish between plugin types once we add support for output and other types of plugins besides JS ones.

Not sure if that is the best way to distinguish between plugin types. Wouldn't interfaces be better? Seems like a much cleaner and type-safe alternative than relying on stringy checks

[mstoykov]

am I correct in thinking that the move of all the current modules in internal was not needed? If I remember correctly the access to internal is for any packages that are the "parent" of the internal and all of its other "children"?

I am not particularly strongly opinionated on whether or not they should be in internal - I doubt if anyone will need them, and I doubt that we will need to use them from somewhere else, and they are quite tightly coupled with goja, so reusage for some other project is also unlikely. On the other hand, this makes this change bigger and out of what IMO is needed.
cc @na--

[imiric]

@mstoykov You're correct, moving all modules to internal wasn't necessary, we just need the module map there. I fixed this in the latest push.

I'll squash everything into a single commit before merging, as there was a lot of experimentation here.

[mstoykov]

LGTM, but drop the html changes that were leftover and .. maybe squash this now in one commit?

[mstoykov]

nit: I am not certain having a Lock is truly necessary as the Register happens in init functions and I would argue should not happen at any other time. And the inits are by definition not concurrent so 🤷

Don't get me wrong ... it is probably better to have lock in the case that we ever decide to have Register outside the init functons. And I doubt the RLock significantly slows anything

[imiric]

Good point, I wasn't aware init()s weren't concurrent.

I don't feel strongly about it to remove it, but if it's YAGNI then we might as well do so.

@na-- What do you think?

[na--]

I don't think it matters much - it shouldn't be needed, but it won't slow down anything either, so 🤷‍♂️

[mstoykov]

I kind of wonder if we shouldn't just either:

1. always prefix it
2. panic if it isn't prefixed instead of prefixing it.

My reasoning is that if someone writes Register("k7/x/something") and then goes to use their module they will be really surprised that it totally doesn't work. While if it panics or the documentation specifically says taht we always prepend k6/x/ that will be much better IMO

[imiric]

Hhmm well the function comment does explain that imports will be in the form k6/x/<name>, and this gives the option to register with or without it. I can see how panicking would be helpful though, but not sure if we should force users to specify the prefix. Kind of torn on this one...

[mostafa]

I think it would be better to just ask for the category and the name (net/rtp) and always prepend the prefix (k6/x/) ourselves. We can simply mention this approach in the docs and our reasoning would be that it is the intended behavior. Then there's no need for panic.

[simskij]

Automatically adding prefixes will likely just lead to more confusion.

I'm thinking that manual import path config + Panics would be preferable as:

1. The error tells the user how to solve it.
2. No hidden behavior you need to know or document.
3. Import path in the JS file will match exactly what they have written in their plugin.

[imiric]

OK, @simskij convinced me, panic it is. 😄

Will push the change in a bit.

@mostafa Asking for the category would be interesting, and similar to xcaddy's namespaces. Right now the consensus is that it doesn't seem necessary for this PoC, but if it becomes a requirement in the future then it would be difficult to change the existing API, so I'm on the fence about it. @na-- @mstoykov What are your thoughts about it?

[mstoykov]

as before I don't think categories are needed for k6. I find the "we have different module registers and functions for different categories" approach much better and easier to document and easy for the people who want add 1 type of plugin to not have to read how it's done for the others.

We can always add a more central plugin system and wire the old calls to it and recommend that people use the new system ... although again ... I highly doubt this day will come

[na--]

Hmm why do we need internal/modules at all? From what I can see, there's nothing stopping a JS plugin from directly calling internal/modules.Register("k6/non-x-path"), right?

Wouldn't it be both simpler and more secure to have something like this in the old js/modules/index.go file?

var modules = map[string]interface{}{
	"k6":                k6.New(),
	"k6/crypto":         crypto.New(),
	"k6/crypto/x509":    x509.New(),
	"k6/encoding":       encoding.New(),
	"k6/http":           http.New(),
	"k6/metrics":        metrics.New(),
	"k6/html":           html.New(),
	"k6/ws":             ws.New(),
	"k6/protocols/grpc": grpc.New(),
}
const extPrefix string = "k6/x/"

func Get(name string) interface{} {
	return modules[name]
}

func Register(name string, mod interface{}) {
	if !strings.HasPrefix(name, extPrefix) {
		panic(fmt.Errorf("external module names must be prefixed with '%s', tried to register: %s", extPrefix, name))
	}

	modules[name] = mod
}

The built-in k6 modules would be an immutable static map and and you can only add modules starting with k6/x, while you can get any module?

[na--]

I didn't know about this... 😊 Live and learn, I guess - LGTM!