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.

Sign up for GitHub

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

Output refactoring #1869

Merged
merged 11 commits into from
Feb 26, 2021
Merged

Output refactoring #1869

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b7abb73
Use a new interface for Outputs and adapt the old Collectors to it
na-- Feb 23, 2021
8bdb2b2
Improve output.SampleBuffer and add some tests
na-- Feb 23, 2021
e5afaa3
Properly wait for old Collectors to finish
na-- Feb 24, 2021
cbe173b
Fix TestSampleBufferBasics randomness
na-- Feb 24, 2021
19e623f
Improve and test output.PeriodicFlusher
na-- Feb 24, 2021
fc321fb
Add tests for the new JSON output
na-- Feb 24, 2021
88bc997
Remove the old JSON Collector
na-- Feb 24, 2021
7e1bc5d
Fix a linting error
na-- Feb 24, 2021
ac4a3f9
Fix typos and improve UX
na-- Feb 25, 2021
626694d
Restore cloud test name overwriting from the CLI argument
na-- Feb 25, 2021
5e41f5e
Move the output extension registry to the main output module
na-- Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestWithEngine(t *testing.T) {
logger.SetOutput(testutils.NewTestOutput(t))
execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

rw := httptest.NewRecorder()
Expand Down
2 changes: 1 addition & 1 deletion api/v1/group_routes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestGetGroups(t *testing.T) {

execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{Group: g0}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

t.Run("list", func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions api/v1/metric_routes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestGetMetrics(t *testing.T) {
logger.SetOutput(testutils.NewTestOutput(t))
execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

engine.Metrics = map[string]*stats.Metric{
Expand Down Expand Up @@ -88,7 +88,7 @@ func TestGetMetric(t *testing.T) {
logger.SetOutput(testutils.NewTestOutput(t))
execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

engine.Metrics = map[string]*stats.Metric{
Expand Down
2 changes: 1 addition & 1 deletion api/v1/setup_teardown_routes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func TestSetupData(t *testing.T) {
})
execScheduler, err := local.NewExecutionScheduler(runner, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, runner.GetOptions(), lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, runner.GetOptions(), lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

globalCtx, globalCancel := context.WithCancel(context.Background())
Expand Down
4 changes: 2 additions & 2 deletions api/v1/status_routes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestGetStatus(t *testing.T) {
logger.SetOutput(testutils.NewTestOutput(t))
execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, lib.Options{}, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)

rw := httptest.NewRecorder()
Expand Down Expand Up @@ -101,7 +101,7 @@ func TestPatchStatus(t *testing.T) {
t.Run(name, func(t *testing.T) {
execScheduler, err := local.NewExecutionScheduler(&minirunner.MiniRunner{Options: options}, logger)
require.NoError(t, err)
engine, err := core.NewEngine(execScheduler, options, lib.RuntimeOptions{}, logger)
engine, err := core.NewEngine(execScheduler, options, lib.RuntimeOptions{}, nil, logger)
require.NoError(t, err)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
Expand Down
25 changes: 24 additions & 1 deletion cloudapi/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"gopkg.in/guregu/null.v3"

"github.com/kelseyhightower/envconfig"
"github.com/loadimpact/k6/lib/types"
)

Expand Down Expand Up @@ -241,7 +242,8 @@ func (c Config) Apply(cfg Config) Config {
return c
}

// MergeFromExternal merges three fields from json in a loadimpact key of the provided external map
// MergeFromExternal merges three fields from the JSON in a loadimpact key of
// the provided external map. Used for options.ext.loadimpact settings.
func MergeFromExternal(external map[string]json.RawMessage, conf *Config) error {
if val, ok := external["loadimpact"]; ok {
// TODO: Important! Separate configs and fix the whole 2 configs mess!
Expand All @@ -262,3 +264,24 @@ func MergeFromExternal(external map[string]json.RawMessage, conf *Config) error
}
return nil
}

// GetConsolidatedConfig combines the default config values with the JSON config
// values and environment variables and returns the final result.
func GetConsolidatedConfig(jsonRawConf json.RawMessage, env map[string]string) (Config, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like some tech debt we'll have to address when working on #883. :-/

Ideally outputs shouldn't have to deal with raw configuration sources and this consolidation should happen before they're initialized. It is a bit cleaner and more consistent than before, but there's still a lot of duplication and slight differences between each implementation which is probably what made it difficult to abstract away.

Copy link
Member Author

Choose a reason for hiding this comment

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

this consolidation should happen before they're initialized

It does 😕 This is precisely what this bit of code does: https://github.com/loadimpact/k6/blob/7e1bc5dcab910f0d197a63e843e5591de4c60423/cmd/outputs.go#L68-L72

And this isn't new technical debt, it was already part of k6 and #883, it was just squirreled away in cmd/ in a much, much worse way, see these snippets from master:
https://github.com/loadimpact/k6/blob/51790fc8df37113ac20b586b28051e21672dab16/cmd/config.go#L68
https://github.com/loadimpact/k6/blob/51790fc8df37113ac20b586b28051e21672dab16/cmd/config.go#L96
https://github.com/loadimpact/k6/blob/51790fc8df37113ac20b586b28051e21672dab16/cmd/config.go#L174
https://github.com/loadimpact/k6/blob/51790fc8df37113ac20b586b28051e21672dab16/cmd/config.go#L196
https://github.com/loadimpact/k6/blob/51790fc8df37113ac20b586b28051e21672dab16/cmd/collectors.go#L90-L97

Though I just saw that I missed config.Name = null.StringFrom(arg) from that last code snippet, so I'll fix that... 😅

But yeah, we have to fix this configuration mess - the current state is definitely not the end goal, it is just slightly more sane than before, #883 is still very much required.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does 😕 This is precisely what this bit of code does

I know. My point was that each output package shouldn't handle raw configuration at all, and should just receive the consolidated values, which should be done much earlier and only once / in a single location.

But yeah, maybe something for #883 then. At least it's consistently bad now 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

each output package shouldn't handle raw configuration at all,

We can debate this when we start dealing with #883, but if I correctly understand what you want, I am not sure I agree. I don't think having a central configuration clearing house is wise - why should we have one huge module that deals with every type of configuration? Why not have each component expose a unified interface for dealing with raw configuration sources and returning errors, but leave the particular configuration logic internal for every module?

Besides, this will never work with extensions, there is no way for k6 to know how they are configured, so the only way is to to pass the "raw" values to the extension and return any resulting validation values, since it's the only thing that knows its own config.

Btw I think that the new outputs would be ideal testing grounds for potential #883 solutions. They are now self-contained, and each one receives a raw JSON chunk, a CLI argument, and a map of environment variables in their constructor. So we can start converting their configuration parsing and validation to something saner (that avoids Apply() and envconfig and all of the other issues) one by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the cloud regression in 626694d

result := NewConfig()
if jsonRawConf != nil {
jsonConf := Config{}
if err := json.Unmarshal(jsonRawConf, &jsonConf); err != nil {
return result, err
}
result = result.Apply(jsonConf)
}

envConfig := Config{}
if err := envconfig.Process("", &envConfig); err != nil {
// TODO: get rid of envconfig and actually use the env parameter...
return result, err
}

return result.Apply(envConfig), nil
}
12 changes: 6 additions & 6 deletions cmd/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"syscall"
"time"

"github.com/kelseyhightower/envconfig"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"github.com/spf13/afero"
Expand Down Expand Up @@ -105,7 +104,8 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
return err
}

runtimeOptions, err := getRuntimeOptions(cmd.Flags(), buildEnvMap(os.Environ()))
osEnvironment := buildEnvMap(os.Environ())
runtimeOptions, err := getRuntimeOptions(cmd.Flags(), osEnvironment)
if err != nil {
return err
}
Expand Down Expand Up @@ -141,8 +141,8 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
}

// Cloud config
cloudConfig := cloudapi.NewConfig().Apply(derivedConf.Collectors.Cloud)
if err = envconfig.Process("", &cloudConfig); err != nil {
cloudConfig, err := cloudapi.GetConsolidatedConfig(derivedConf.Collectors["cloud"], osEnvironment)
if err != nil {
return err
}
if !cloudConfig.Token.Valid {
Expand All @@ -153,8 +153,8 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
arc := r.MakeArchive()
// TODO: Fix this
// We reuse cloud.Config for parsing options.ext.loadimpact, but this probably shouldn't be
// done as the idea of options.ext is that they are extensible without touching k6. But in
// order for this to happen we shouldn't actually marshall cloud.Config on top of it because
// done, as the idea of options.ext is that they are extensible without touching k6. But in
// order for this to happen, we shouldn't actually marshall cloud.Config on top of it, because
// it will be missing some fields that aren't actually mentioned in the struct.
// So in order for use to copy the fields that we need for loadimpact's api we unmarshal in
// map[string]interface{} and copy what we need if it isn't set already
Expand Down
175 changes: 0 additions & 175 deletions cmd/collectors.go
View file
Open in desktop

This file was deleted.

Loading