Improve config (closes #810) #834

ire-and-curses · 2019-01-25T22:51:32Z

This closes #810 by cleaning up the redundancy in the Horizon command line option handling.

A new array, configOpts, is used to fully define the command line parameters. To add a new command line parameter, it should be sufficient to add a line to configOpts, and a new field to the horizon.Config struct.

Optionally, the developer can hook a custom function to the new configOpts line for validation and/or transformation of the incoming parameter.

I wanted to avoid invasive changes, but during this refactoring I found a few inconsistencies which are worth pointing out:

Unused option ruby-horizon-url (I removed)
Incomplete configuration for skip-cursor-update (I added missing config)
Redundant viper defaults for port and history-retention-count (I removed)
friendbot-url is stored as a url.URL, but other URLs (e.g. db-url, stellar-core-db-url, stellar-core-url, redis-url, sentry-dsn etc.) are just strings. Should we change this in the future?
tls-cert and tls-key have custom checking to ensure both are provided, but there are other pairs that could be similarly checked, e.g. redis-limit-redis-key and redis-url, loggly-token and loggly-tag (easy to do).
Inconsistent names could be cleaned up, e.g. there is no reason to map history-stale-threshold to stale-threshold, or db-url to the rather generic env variable DATABASE_URL.
Some numbers are uint but others are not. Not sure what the logic is for the uints.

bartekn

Some small changes are required.

Answering your questions:

friendbot-url is stored as a url.URL, but other URLs (e.g. db-url, stellar-core-db-url, stellar-core-url, redis-url, sentry-dsn etc.) are just strings. Should we change this in the future?

friendbot-url is going to be removed eventually so I think we shouldn't spend more time on this.

tls-cert and tls-key have custom checking to ensure both are provided, but there are other pairs that could be similarly checked, e.g. redis-limit-redis-key and redis-url, loggly-token and loggly-tag (easy to do).

Can be checked in initConfig() after all variables have been set to Config.

Some numbers are uint but others are not. Not sure what the logic is for the uints.

Good point, let's change Port and MaxDBConnections to uint.

services/horizon/main.go

bartekn · 2019-01-28T10:49:05Z

services/horizon/main.go

+	name           string              // e.g. "db-url"
+	envVar         string              // e.g. "DATABASE_URL". Defaults to uppercase/underscore representation of name
+	flagType       flagType            // e.g. boolFlag
+	flagDefault    interface{}         // A default if no option is provided. Set to "" if no default


Let's use nil instead of "" if no default value.

I use "" because flagDefault is used to infer the type. We never set empty defaults for other types, so we can use the empty string as a type indicator. This allows us to determine the option type without the need to explicitly set it, saving a lot of extra configuration.

bartekn · 2019-01-28T10:58:51Z

services/horizon/main.go

+	// Check all required args were provided - needed for migrations check
+	for i := range configOpts {
+		co := &configOpts[i]
+		co.require()


Should be moved to func (* configOption) init() method.

I like this idea. But I think we can't move the contents of both loops to the same method. They are executed at different times. This require step only happens when the app is executed.

services/horizon/main.go

bartekn · 2019-01-28T18:31:45Z

PS. you can consider closing #817 in this PR.

ire-and-curses · 2019-01-28T23:14:38Z

Can't change MaxDBConnections to uint, code expects int.
Most other changes made. Please take another look.

bartekn

Other that new comments all LGTM!

bartekn · 2019-01-29T11:24:58Z

services/horizon/main.go

+		Name:        "db-url",
+		EnvVar:      "DATABASE_URL",
+		ConfigKey:   &config.DatabaseURL,
+		FlagDefault: "",


Continuing this discussion here.

I use "" because flagDefault is used to infer the type.

I think it shouldn't be used to get type. It's like embedding two values in a single field. Instead we should use types.* (types.Bool, types.Uint, types.String, etc). The advantage is that it's clear what type we are dealing with and we can omit FlagDefault if it's a zero value.

This suggestion makes sense to me too

Done. Unfortunately pflags library requires a default value for all flags (see e.g. pflag.Bool), so I wrote a helper setDefault to add the empty string if a FlagDefault is not explicitly declared.

bartekn · 2019-01-29T11:31:11Z

services/horizon/main.go

-		stdLog.Fatal("Invalid config: stellar-core-url is blank.  Please specify --stellar-core-url on the command line or set the STELLAR_CORE_URL environment variable.")
+	// Initialise and validate the global configuration
+	for _, co := range configOpts {
+		co.SetValue()


Continuing this discussion here.

But I think we can't move the contents of both loops to the same method. They are executed at different times. This require step only happens when the app is executed.

Any reason why Require and SetValue can't be called along with other init steps in one place? You can actually move:

for _, co := range configOpts { co.Init(rootCmd) }

to initConfig().

These can't be combined - they are needed at different stages of the option parsing lifecycle. I think this is why the code was originally written with both init() and initConfig() methods. You can test for yourself - if you move the code into a single loop, it will fail and display help text.

So I pulled the branch locally and indeed it wasn't possible with viper.BindPFlags as it required all flags to be set upfront. However, after switching to viper.BindPFlag (note: no s) it was possible. I actually pushed this commit: 75b8658 hope you don't mine. I also removed flagType as it was used in only a single method I actually changed and added error handling for viper methods that return errors.

Looks great, thanks for those improvements! 👍 Are we good to merge now?

services/horizon/main.go

support/config/config_option.go

bartekn

Last (promise!) round of minor comments.

services/horizon/main.go

support/config/config_option.go

ire-and-curses · 2019-01-31T00:55:17Z

Ok, fixed issue with location of pflags binding, addressed other comments. Please take another look, and I'd appreciate a quick test to make sure this runs for you as well.

bartekn

LGTM! Checked it locally and it works. I fixed a small comment issue in 1b08f7b.

* tag 'horizon-v0.16.0': horizon: 0.16.0 CHANGELOG (stellar#851) Allow smooth migration for failed transactions column (stellar#847) Clean up the redundancy in the Horizon command line option handling (stellar#834) horizon/actions: fix a bug in `GetPageQuery` and `GetAddress` (stellar#846) remove JSON horizon/actions: check hash before sending an event to the stream of orderbook (stellar#828) Deprecated check (stellar#838) Report right protocol version in horizon root path (stellar#783) fix migration ordering (stellar#821) Add failed transactions to ledger resource (stellar#756) Ignore "0" trades effects (stellar#791) Add package index to README (stellar#831) horizon/db2/history: optimize account queries for payments and operations (stellar#829) Update "gap detected" message (stellar#830) Update setTickInProgress comment (stellar#824) Bump postgres version for Travis (stellar#822) fix wrong effect of a circular path payment (stellar#807) Update README.md (stellar#820) Fix build badge (stellar#816) Require migration to run Horizon version successfully (fixes stellar#778) (stellar#808)

@bartekn

Move the call to `viper.BindPFlags` from our services that use the `support/config` package, into the config package `Init` function. Why: The `support/config` package does a good job of wrapping up cobra and viper functionality so that an application only needs to define one set of config and by calling a few functions can have that config provided by flags and environment variables in a consistent manner. While the package does most of the work to configure all the moving parts, it doesn't call the BindPFlags function and relies on the app to call this. It's really easy to miss that the app has to do this one thing, and I lost a good amount of time trying to understand why none of my configs were being populated. The package owns setting viper up, and so it should own this part of the process too. We're only using this on Horizon right now but this package looks like a good starting point for us to have a common pattern to configure our services with environment variables and flags. On some of our services this doesn't matter because the configs are all non-sensitive values, but as we write services that need keys making use of flags and environment variables consistently will be ideal. @bartekn originally proposed this change here #834 (comment) and made the change in 75b8658. Before the PR was merged @ire-and-curses reverted the change in 2a8354b because the first commit had broken the setting of values, but as it turned out only some of the code needed to be reverted.

ire-and-curses added 20 commits January 18, 2019 11:19

testing push access

a7b7f93

strutils helper lib

02d287c

WIP

890d947

WIP: factoring out persistentFlags

999a283

require missing args

7868fb5

missing log level default

beb27ca

prototyping validator func

86a88d5

proof of concept dynamic config

6218aaf

generalise config keys

212e975

all options handled, needs testing

7fc453c

clean up helpers

e197148

standardise ordering of config opts

baa6e24

docs

e901814

TODOs

7f0e36d

handle uints

5dcce43

fix rate limit

d9db8d0

final testing

e1323cc

final testing commit

644b381

final cleanup

bc12983

minor var

0bd48c5

ire-and-curses requested review from bartekn, tomquisel and howardtw January 25, 2019 22:51

ire-and-curses changed the title ~~Es 810 improve config~~ Improve config (closes #810) Jan 25, 2019

bartekn suggested changes Jan 28, 2019

View reviewed changes

uint port, remove unnecessary error return

99601a1

ire-and-curses added 3 commits January 28, 2019 10:44

simplify loops

1f06d3c

remove global TLS counter, add other pair validation

c2b445a

remove side effects from custom setters

d826adc

ire-and-curses added 6 commits January 28, 2019 13:42

reformat options table as record format

d07f1f9

removed aliased c config

a741e9e

refactor to init method

30c70ba

docstring

96cabe7

remove loggly check, due to default

95b4907

move ConfigOption to support/config

c89ac3a

remove NETWORK_PASSPHRASE default, closes #817

ecac457

bartekn added the horizon label Jan 29, 2019

bartekn suggested changes Jan 29, 2019

View reviewed changes

ire-and-curses and others added 2 commits January 29, 2019 13:47

more review comments

ae66557

Merge all init methods into one init function

75b8658

bartekn reviewed Jan 30, 2019

View reviewed changes

bartekn and others added 4 commits January 30, 2019 20:42

Fix horizon ConfigOption init

319cd0b

fix issue by splitting back out run time flag calls

2a8354b

fix validation of dual options

a5a10e3

more review improvements

888ac17

Update FlagDefault comment

1b08f7b

bartekn approved these changes Jan 31, 2019

View reviewed changes

bartekn merged commit 09d6ab4 into master Jan 31, 2019

bartekn deleted the es-810-improve-config branch January 31, 2019 13:38

ire-and-curses mentioned this pull request Jan 31, 2019

Remove setting NETWORK_PASSPHRASE to default testnet value #817

Closed

leighmcculloch mentioned this pull request Dec 12, 2019

support/config: move viper bindpflag into init #2041

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve config (closes #810) #834

Improve config (closes #810) #834

ire-and-curses commented Jan 25, 2019

bartekn left a comment

bartekn Jan 28, 2019

ire-and-curses Jan 28, 2019

bartekn Jan 28, 2019

ire-and-curses Jan 28, 2019

bartekn commented Jan 28, 2019

ire-and-curses commented Jan 28, 2019

bartekn left a comment

bartekn Jan 29, 2019

tomquisel Jan 29, 2019

ire-and-curses Jan 29, 2019

bartekn Jan 29, 2019

ire-and-curses Jan 29, 2019

bartekn Jan 30, 2019

ire-and-curses Jan 30, 2019

bartekn left a comment

ire-and-curses commented Jan 31, 2019

bartekn left a comment

Improve config (closes #810) #834

Improve config (closes #810) #834

Conversation

ire-and-curses commented Jan 25, 2019

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn commented Jan 28, 2019

ire-and-curses commented Jan 28, 2019

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

ire-and-curses commented Jan 31, 2019

bartekn left a comment

Choose a reason for hiding this comment