-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Plugin support #1396
Plugin support #1396
Conversation
@@ -104,6 +104,14 @@ func getNullString(flags *pflag.FlagSet, key string) null.String { | |||
return null.NewString(v, flags.Changed(key)) | |||
} | |||
|
|||
func getStringSlice(flags *pflag.FlagSet, key string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if this was really necessary, as an empty slice is fine. I just wrote it so we could have a panic
error handler 😃
@@ -107,6 +109,34 @@ a commandline interface for interacting with it.`, | |||
Left: func() string { return " init" }, | |||
} | |||
|
|||
// Load plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this up from where it was.
Long story short: plugins need to be loaded before the runner gets initialized, as lookup paths are going to be resolved by that time. getConfig
is the earliest point at which we have access to the plugin parameters so I decided to pull it up and do it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mention in the other comment, you shouldn't use getConfig()
here, but rather move and use getRuntimeOptions()
cmd/run.go
Outdated
|
||
plugins := []plugin.JavaScriptPlugin{} | ||
for _, pluginPath := range cliConf.Plugins { | ||
jsPlugin, err := lib.LoadJavaScriptPlugin(pluginPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy about this way to interface with plugins. This assumes (correctly, for now) that plugins are only for JavaScript.
Ultimately, I think I want to have plugins that export many different types of plugins (say, a metrics plugin that exposes the API to add metrics, but also the metrics itself to export from K6) but I'm not 100% sure of what that looks like or how it's organized. I think this needs to be part of a design discussion where we draw boxes and arrows and the plugin writing UX needs to be heavily considered too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that some kind of whiteboard session might be beneficial for this.
Further, I really think we need to solve, or at least discuss, this before this can get merged, or we'll end up in a situation where it will get increasingly hard to change for every plugin that's written. cc @na- Scratch that, apparently the scope for this was js plugins only at this point.
cmd/run.go
Outdated
|
||
// Run plugin setup and add it to the module tree | ||
if err = jsPlugin.Setup(); err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should just return without tearing down any other potential plugins that succeeded. Might be a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could've possibly defer
ed the teardown
as it is currently run at the end of this same function ... which definitely needs some refactoring ...
cmd/run.go
Outdated
|
||
// TODO: does this belong here, or is it the module package's responsibility? | ||
mods := jsPlugin.GetModules() | ||
for path, api := range mods { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be a method in the modules
package, as the runner shouldn't care about this sort of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it so that it's a method in the modules
package. Feels much nicer there.
@@ -473,6 +499,12 @@ a commandline interface for interacting with it.`, | |||
} | |||
} | |||
|
|||
// Teardown plugins | |||
for _, jsPlugin := range plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care if teardowns error out? Should we log after the output or are there repercussions for that? (ie. any tools that might start failing because output is slightly different)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely log it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please log any errors here
@@ -285,6 +285,9 @@ type Options struct { | |||
|
|||
// Redirect console logging to a file | |||
ConsoleOutput null.String `json:"-" envconfig:"K6_CONSOLE_OUTPUT"` | |||
|
|||
// Plugins to load | |||
Plugins []string `json:"plugins" envconfig:"K6_PLUGINS"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to figure out how this envconfig
flag works. I couldn't get it to read from the environment but I'm probably just missing something really basic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
envconfig has issues (#671) and we want to replace it, but the problem here is not related to them... The reason you can't read from the environment is that this reading happens after you've already initialized the plugins. The "consolidated" version of lib.Options
is constructed here: https://github.com/andremedeiros/k6/blob/9dbc2f9ff3d7dbceeb6f06c58b81dcfda3fb3243/cmd/run.go#L169-L172
And, as you can see, this "consolidation" consists of combining the {option defaults + JSON config options + the script exported options
+ environment variables + CLI flags}, in increasing order of precedence (more info). That chicken-and-egg situation is why you literally can't fix this here, given that plugins would have to be available when the init context of the script is executed to get its exported options
.
Instead of lib.Options
, you should have the plugin list in RuntimeOptions
, which contain things that might affect even the initial script execution (and thus, the exported script options
), like environment variables. They can also easily be moved earlier in the cmd/run.go
execution as well: https://github.com/loadimpact/k6/blob/0e94653c9954c17b2b1a480b456e23ec120feff2/cmd/run.go#L123-L126
"github.com/loadimpact/k6/plugin" | ||
) | ||
|
||
func LoadJavaScriptPlugin(path string) (plugin.JavaScriptPlugin, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
return jsPlugin, nil | ||
} | ||
|
||
func loadPlugin(path string) (*goplugin.Plugin, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to expand on this (maybe not necessarily for v1) to ensure that we return nice error messages for things like:
- plugin built with a different version of the
plugin
package - plugin built with a different version of go
- file not found
- wrong architecture
plugin/plugin.go
Outdated
|
||
package plugin | ||
|
||
// A Plugin ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really bad at writing docs. Can you tell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Plugin is used to extend the javascript VM with functionality implemented in go.
?
Codecov Report
@@ Coverage Diff @@
## master #1396 +/- ##
==========================================
+ Coverage 75.22% 75.26% +0.03%
==========================================
Files 150 151 +1
Lines 10911 10954 +43
==========================================
+ Hits 8208 8244 +36
- Misses 2238 2241 +3
- Partials 465 469 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! Here are some initial thoughts. I will do a more thorough review later when there are tests and such.
plugin/plugin.go
Outdated
|
||
package plugin | ||
|
||
// A Plugin ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Plugin is used to extend the javascript VM with functionality implemented in go.
?
cmd/run.go
Outdated
|
||
plugins := []plugin.JavaScriptPlugin{} | ||
for _, pluginPath := range cliConf.Plugins { | ||
jsPlugin, err := lib.LoadJavaScriptPlugin(pluginPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that some kind of whiteboard session might be beneficial for this.
Further, I really think we need to solve, or at least discuss, this before this can get merged, or we'll end up in a situation where it will get increasingly hard to change for every plugin that's written. cc @na- Scratch that, apparently the scope for this was js plugins only at this point.
I'm making the choice to remove support to specify plugins via When
Now, an easy fix would be to switch 3 and 4, but 4 depends on 3 to help gather options. What's more, at the time of compilation, require statements get resolved, so plugins already have to be loaded. I could try to refactor this dependency out, but that would require making product decisions that I'm not equipped to make. I'm happy to take over this work after a whiteboarding session with the team in a future PR, if you think it makes sense. |
With regards to nicer errors, unfortunately the plugin library does not expose error properties I can match against. Matching against error strings is heavily discouraged, as those can change anytime, so I won't be adding nicer error messages. Instead, errors are shown as they come, wrapped to help indicate what the source of the error is. |
.circleci/config.yml
Outdated
- run: | ||
name: Build plugin artifact for tests | ||
working_directory: /tmp | ||
command: | | ||
go version | ||
export GOMAXPROCS=2 | ||
export PATH=$GOPATH/bin:$PATH | ||
go get -d github.com/andremedeiros/leftpad | ||
go build -buildmode=plugin -o /tmp/leftpad.so github.com/andremedeiros/leftpad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that:
- the test should work without an additional step before that of building the plugin ... this should probably happen in the test ?
- I would really prefer if the test plugin is in our repo - probably in the
samples
folder? (great plugin name ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great plugin name
I can't take the credit. Someone in the slack suggested that for a test plugin, and I just went with it :-P
for path, impl := range modules { | ||
importPath := fmt.Sprintf("k6-plugin/%s", path) | ||
PluginIndex[importPath] = impl | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I do think it will probably be easier to strip the prefix in the lookup ... although maybe the error message will need some work so it does actually include the "k6-plugin/"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay :( . I did take a quick look - looks good to me, but I will need a second deeper look when I have more time. Thanks for doing it :D
The things that I do think will definitely need fixing are :
- the test will need to work without running some other commands before that ...
- the example(sample) plugin will need to live inside the k6 repo.
- we need to error out on windows with a nice error (maybe this can be left for us if you don't have a windows machine to test it on), also the test needs to be skipped on windows :)
I have other thoughts on some types, module names and where things are put, but this is dependant on how much we want to try to design now ...
Should I understand that this works for you? Like, have you used it for your use case, successfully?
Again sorry for the slow reviews ... they won't get much faster soon unfortunately and thanks :D
No apologies necessary. I appreciate you taking a look!
To be clear, are you OK with me exec'ing
Correct. I ignored this as something I was going to tackle later but I'll get on it.
Happy to hear them. We can either discuss here or have a call where we draw boxes and arrows and update the issue / code. I want this to work for you too, so whatever it takes to get it right the first time.
It works for me, yeah! We're loading a plugin and running tests with it, so that's a huge success! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this, I was imagining it would be much more complex. I agree with @mstoykov's comments about moving the sample plugin for tests in the repo, and removing the Makefile
changes. Not sure if it could be compiled with a go:generate
declaration, or if there's a cleaner alternative to exec-ing go build
in the test, but it's something to look into. You could probably avoid this by injecting mock plugins in tests and have more simpler lower-level tests for different scenarios, and just a few high-level ones that require compilation.
cmd/run.go
Outdated
} | ||
|
||
// Run plugin setup and add it to the module tree | ||
if err = jsPlugin.Setup(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider running this in a separate goroutine with some timeout, to avoid a misbehaving plugin from blocking further execution, and maybe load them concurrently before the Runner
is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider running this in a separate goroutine with some timeout
Added to the list :-)
Thank you, but now you're making me think I might have missed something :-P
I feel that we'd get no added value from mocking if we load a real plugin, but ultimately I'll defer the decision to you. I'll give this another pass tomorrow. Sorry it's taking so long, but I'm glad we're getting something polished out there. |
hi @andremedeiros , the internal discussion more or less finished with "we are unlikely to do any releases before we merge 1007 and given its size it is probably better if we don't add any more changes, also we want it to be done as fast as possible". Having said that, some words on the actual PR:
Again thank you for the work, and again sorry we are likely going to merge it anytime soon - we still want it and think it's a great change :D |
Alright, definitely done this time around.
I wanted to get the tests to run with and without race detection, so I had to add a Fixed the Windows implementation and tests too. Only thing in the way of a fully green build right now is the linter, which is complaining about silly things that we have full control over or want to stay by design. Again, sorry it took so long -- life kinda got in the way, but we're there now! |
Hi @andremedeiros, I just made a simple PoC on top of your PR: k6-plugin-kafka. |
Holy shit @mostafa, well done! Got any feedback re: the API and implementation? Would love to hear from the first plugin implementer out there! |
Since I am not a gopher, it was a little weird to start with, but with your leftpad example, I was good to go. I think we should do more work on docs and basics of implementing one. Also the API was very straight-forward. 👍 |
Hi @andremedeiros, I don't know if it is by design or any other way around, but the memory consumption behavior of the plugin seems to be like JS code, in that it expands with the number of VUs. I expected it not to increase memory usage, which is not the case. I think the goal for writing a plugin, rather than bundling a JS module/package, is to fully/partially address this memory consumption issue. Any comments? |
That makes sense to me. I assume that every VU has its own JS VM instance running, and that would get copied/cloned from the original. It wouldn't be more than, say, loading pre-built modules tho. |
Hi @andremedeiros, I am working on sending metrics to the k6 process from inside the plugin, yet I've hit the wall of state being nil. AFAIK, The |
Hi friends! Any chance this gets merged soon? |
Hi! At this point, this PR is still pending #1007 and making sure that the content of that PR is stable. Best, |
Hey, @andremedeiros 👋 We finally released k6 v0.27.0 last week, so the time for merging this PR has almost arrived! 🎉 We're currently cleaning up some minor issues and preparing to release v0.27.1 in the next few days, but our top priority after that will be adding support for JS plugins! In the meantime, can you rebase this PR on top of the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andremedeiros, I'm terribly sorry for the super delayed review... 😞 And thank you for the initiative and the awesome PR! It mostly looks very good, the only issue I can see, apart from a couple of nitpicks, is that the plugin list should be in lib.RuntimeOptions
and not lib.Options
. And the fact that it has to be rebased on the latest master
, which will probably result in a few conflicts post-#1007 ... 😞
We're aiming to get this merged in k6 soon, so we can include it in k6 v0.28.0, slated for release mid-September. Given how delayed we were in getting back to you, I don't want to presume that you have the time or energy to get back to it right now... Can you update us on your availability and willingness to rebase this and make the minor fixes in the coming days?
No worries and no pressure if you don't have the time or desire to do so now, just please let us know. Given that it's basically one of our top priorities right now, we can take it from here and finish up the remaining minor things ourselves (of course, giving you full credit in the release notes, commits, etc.).
btw one additional item we discovered we'll have to fix on our own is that, for plugins to work, we'll have to start building k6 with CGO_ENABLED=1
again, so we'll likely also have some CI work regardless
// Do the part that actually takes time in a runner group. | ||
g.Go(jsPlugin.Setup) | ||
|
||
modules.RegisterPluginModules(jsPlugin.GetModules()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm we should probably emit an error if there are conflicts in plugin module names?
@@ -473,6 +499,12 @@ a commandline interface for interacting with it.`, | |||
} | |||
} | |||
|
|||
// Teardown plugins | |||
for _, jsPlugin := range plugins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please log any errors here
@@ -285,6 +285,9 @@ type Options struct { | |||
|
|||
// Redirect console logging to a file | |||
ConsoleOutput null.String `json:"-" envconfig:"K6_CONSOLE_OUTPUT"` | |||
|
|||
// Plugins to load | |||
Plugins []string `json:"plugins" envconfig:"K6_PLUGINS"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
envconfig has issues (#671) and we want to replace it, but the problem here is not related to them... The reason you can't read from the environment is that this reading happens after you've already initialized the plugins. The "consolidated" version of lib.Options
is constructed here: https://github.com/andremedeiros/k6/blob/9dbc2f9ff3d7dbceeb6f06c58b81dcfda3fb3243/cmd/run.go#L169-L172
And, as you can see, this "consolidation" consists of combining the {option defaults + JSON config options + the script exported options
+ environment variables + CLI flags}, in increasing order of precedence (more info). That chicken-and-egg situation is why you literally can't fix this here, given that plugins would have to be available when the init context of the script is executed to get its exported options
.
Instead of lib.Options
, you should have the plugin list in RuntimeOptions
, which contain things that might affect even the initial script execution (and thus, the exported script options
), like environment variables. They can also easily be moved earlier in the cmd/run.go
execution as well: https://github.com/loadimpact/k6/blob/0e94653c9954c17b2b1a480b456e23ec120feff2/cmd/run.go#L123-L126
@@ -107,6 +109,34 @@ a commandline interface for interacting with it.`, | |||
Left: func() string { return " init" }, | |||
} | |||
|
|||
// Load plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mention in the other comment, you shouldn't use getConfig()
here, but rather move and use getRuntimeOptions()
For completeness' sake, I'll mention it here as well, in case anyone stumbles here first, before reading the issue. This approach for k6 plugins is probably not the way we'll go forward, at least in the short term. For more information about the issues with Go plugins and why we're exploring alternatives, please read through the discussion following #1353 (comment) |
Hey @andremedeiros, we've decided to adopt the xcaddy approach for plugins after all (see #1688), so I'll close this PR. Thanks for your hard work of initially proposing the feature and implementing it in this PR, it's great stuff! Unfortunately the Go plugin approach is not a great fit for k6 right now, and the xcaddy system--while not perfect--will offer a better experience for plugin developers, and that's our priority. :) |
Fixes #1353
To do:
Not fixing (see rationale below):