Env variable DURATION is being used implicitly

[oleksandr-kolisnyk]

I've bumped into this accidentally:
If global environment variable DURATION is set, it is being used by k6 to parse duration value.
In my case I wanted to execute k6 script with following command:
k6 run --vus 2 --duration 60s <script_name.js>
But also I had a global environment variable DURATION=60, and as result my execution kept failing with

ERRO[0001] envconfig.Process: assigning K6_DURATION to Duration: converting '60' to type types.NullDuration. details: time: missing unit in duration 60

I didn't find any mentions about DURATION in docs:
https://docs.k6.io/docs/running-k6
https://docs.k6.io/docs/environment-variables
https://docs.k6.io/docs/execution-context-variables

[na--]

Thanks for reporting this! It is supposed to be K6_DURATION, but it seems that envconfig tries to parse it without the K6 prefix as well... I'd treat this as a bug and if we can't disable this behavior, it may turn out to be yet another reason to replace envconfig...

[oleksandr-kolisnyk]

@na-- just discovered that there is the same issue with ITERATIONS variable

[luizbafilho]

Hi @na-- just checked envonfig and there is no configuration option to avoid that behavior.

[na--]

I guess that means we should just replace or fork the library, since there has also been no progress on the issue I submitted in it's github (about unnecessarily initializing empty struct pointers)

[marco-m]

Hello @na--, any progress in replacing/forking the library? (sorry for somehow hijacking this thread)

[na--]

@marco-m you're not hijacking the thread, unfortunately this hasn't been a big priority recently and we haven't made any progress in replacing or forking the original library 😞

At this point, I'd say that we're very unlikely to fork it, since it has at least 2 major architectural decisions (this issue and linked one about unnecessary initialization of struct pointers) that we disagree with and consider bugs. We'll likely either use another library or write something very simple ourselves to replace it - I think we just need something that reads struct tags (to get the environment variable name) and knows how to work with simple types and encoding.TextUnmarshaler.

[marco-m]

Thanks for the update @na-- !

[mstoykov]

As explained in the upstream issue this is the expected behaviour
Looking at the Readme though I came up with a ... solution for most cases:

1. don't use the envconfig tag
2. where needed because multiple words use split_words

package main

import (
        "fmt"

        "github.com/kelseyhightower/envconfig"
)

type ImplicitTest struct {
        UserName string `split_words:"true"`
}

type ExplicitTest struct {
        UserName string `envconfig:"USER_NAME"`
}

func main() {
        implicitTest := ImplicitTest{}
        envconfig.MustProcess("my_app", &implicitTest)
        fmt.Printf("The value of ImplicitTest.UserName: \"%s\"\n", implicitTest.UserName)

        explicitTest := ExplicitTest{}
        envconfig.MustProcess("my_app", &explicitTest)
        fmt.Printf("The value of ExplicitTest.UserName: \"%s\"\n", explicitTest.UserName)
}

$ USER_NAME=pesho go run main.go
The value of ImplicitTest.UserName: ""
The value of ExplicitTest.UserName: "pesho"
$ MY_APP_USER_NAME=pesho go run main.go
The value of ImplicitTest.UserName: "pesho"
The value of ExplicitTest.UserName: "pesho"

If we agree this is okay I might try to do a fast sweep through the code possibly with sed and just add split_words:"true" instead of any envconfig values ...

$ rg -g "*.go"  'envconfig:"' --count-matches
lib/runtime_options.go:2
lib/options.go:34
cmd/config.go:5
stats/csv/config.go:4
stats/datadog/collector.go:1
stats/cloud/config.go:18
stats/influxdb/config.go:12
stats/kafka/config.go:8
stats/statsd/common/config.go:4

[mstoykov]

Also ... obviously we can just set envconfig to include the K6_ prefix

I don't think we have any use case for not doing it that way ... and it will probably be much easier ...

[na--]

Also ... obviously we can just set envconfig to include the K6_ prefix

Hmm yeah, if we just set the struct tags to envconfig:"K6_WHATEVER_VARIABLE" and discard the prefix for envconfig.Process(), then this should work fine! And I'd prefer that short-term fix than the split_words one, because with split_words we have the potential of introducing new bugs (say, if we have Go variable names that are subtly different than the envconfig ones).

Long term I'd still want to replace the envconfig library because of a lot of other issues (lack of testability, initialization of struct pointers, etc.), but this can patch this particular issue for now in an easy way. Who knows if after #883 our configuration paradigm would even be the same huge structs littered with special tags and magic... 😅

[mstoykov]

After #1215 in order to completely consider this fixed the config struct for statsd and datadog outputs should be split in two.
And the corresponding struct to be tagged accordingly.