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.

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

Loki: Common Config #4347

Merged
merged 11 commits into from
Oct 1, 2021
2 changes: 1 addition & 1 deletion clients/cmd/promtail/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (c *Config) Clone() flagext.Registerer {
func main() {
// Load config, merging config file and CLI flags
var config Config
if err := cfg.Parse(&config); err != nil {
if err := cfg.DefaultUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil {
fmt.Println("Unable to parse config:", err)
os.Exit(1)
}
Expand Down
42 changes: 6 additions & 36 deletions cmd/loki/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"reflect"

"github.com/go-kit/kit/log/level"
"github.com/grafana/dskit/flagext"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/version"
"github.com/weaveworks/common/logging"
Expand All @@ -26,43 +25,14 @@ func init() {
prometheus.MustRegister(version.NewCollector("loki"))
}

type Config struct {
loki.Config `yaml:",inline"`
printVersion bool
verifyConfig bool
printConfig bool
logConfig bool
configFile string
configExpandEnv bool
}

func (c *Config) RegisterFlags(f *flag.FlagSet) {
f.BoolVar(&c.printVersion, "version", false, "Print this builds version information")
f.BoolVar(&c.verifyConfig, "verify-config", false, "Verify config file and exits")
f.BoolVar(&c.printConfig, "print-config-stderr", false, "Dump the entire Loki config object to stderr")
f.BoolVar(&c.logConfig, "log-config-reverse-order", false, "Dump the entire Loki config object at Info log "+
"level with the order reversed, reversing the order makes viewing the entries easier in Grafana.")
f.StringVar(&c.configFile, "config.file", "", "yaml file to load")
f.BoolVar(&c.configExpandEnv, "config.expand-env", false, "Expands ${var} in config according to the values of the environment variables.")
c.Config.RegisterFlags(f)
}

// Clone takes advantage of pass-by-value semantics to return a distinct *Config.
// This is primarily used to parse a different flag set without mutating the original *Config.
func (c *Config) Clone() flagext.Registerer {
return func(c Config) *Config {
return &c
}(*c)
}

func main() {
var config Config
var config loki.ConfigWrapper

if err := cfg.Parse(&config); err != nil {
if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil {
fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err)
os.Exit(1)
}
if config.printVersion {
if config.PrintVersion {
fmt.Println(version.Print("loki"))
os.Exit(0)
}
Expand All @@ -87,19 +57,19 @@ func main() {
os.Exit(1)
}

if config.verifyConfig {
if config.VerifyConfig {
level.Info(util_log.Logger).Log("msg", "config is valid")
os.Exit(0)
}

if config.printConfig {
if config.PrintConfig {
err := logutil.PrintConfig(os.Stderr, &config)
if err != nil {
level.Error(util_log.Logger).Log("msg", "failed to print config to stderr", "err", err.Error())
}
}

if config.logConfig {
if config.LogConfig {
err := logutil.LogConfig(&config)
if err != nil {
level.Error(util_log.Logger).Log("msg", "failed to log config object", "err", err.Error())
Expand Down
2 changes: 1 addition & 1 deletion cmd/migrate/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func main() {
flag.Parse()

// Create a set of defaults
if err := cfg.Unmarshal(&defaultsConfig, cfg.Defaults()); err != nil {
if err := cfg.Unmarshal(&defaultsConfig, cfg.Defaults(flag.CommandLine)); err != nil {
log.Println("Failed parsing defaults config:", err)
os.Exit(1)
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/loki/common/common.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package common

// Config holds common config that can be shared between multiple other config sections
type Config struct {
PathPrefix string `yaml:"path_prefix"`
owen-d marked this conversation as resolved.
Show resolved Hide resolved
}
74 changes: 74 additions & 0 deletions pkg/loki/config_wrapper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package loki

import (
"flag"
"fmt"

"github.com/grafana/dskit/flagext"
"github.com/pkg/errors"

"github.com/grafana/loki/pkg/util/cfg"
)

// ConfigWrapper is a struct containing the Loki config along with other values that can be set on the command line
// for interacting with the config file or the application directly.
// ConfigWrapper implements cfg.DynamicCloneable, allowing configuration to be dynamically set based
// on the logic in ApplyDynamicConfig, which receives values set in config file
type ConfigWrapper struct {
Config `yaml:",inline"`
PrintVersion bool
VerifyConfig bool
PrintConfig bool
LogConfig bool
ConfigFile string
ConfigExpandEnv bool
}

func (c *ConfigWrapper) RegisterFlags(f *flag.FlagSet) {
f.BoolVar(&c.PrintVersion, "version", false, "Print this builds version information")
f.BoolVar(&c.VerifyConfig, "verify-config", false, "Verify config file and exits")
f.BoolVar(&c.PrintConfig, "print-config-stderr", false, "Dump the entire Loki config object to stderr")
f.BoolVar(&c.LogConfig, "log-config-reverse-order", false, "Dump the entire Loki config object at Info log "+
"level with the order reversed, reversing the order makes viewing the entries easier in Grafana.")
f.StringVar(&c.ConfigFile, "config.file", "", "yaml file to load")
f.BoolVar(&c.ConfigExpandEnv, "config.expand-env", false, "Expands ${var} in config according to the values of the environment variables.")
c.Config.RegisterFlags(f)
}

// Clone takes advantage of pass-by-value semantics to return a distinct *Config.
// This is primarily used to parse a different flag set without mutating the original *Config.
func (c *ConfigWrapper) Clone() flagext.Registerer {
return func(c ConfigWrapper) *ConfigWrapper {
return &c
}(*c)
}

// ApplyDynamicConfig satisfies WithCommonCloneable interface, and applies all rules for setting Loki
// config values from the common section of the Loki config file.
// This method's purpose is to simplify Loki's config in an opinionated way so that Loki can be run
// with the minimal amount of config options for most use cases. It also aims to reduce redundancy where
// some values are set multiple times through the Loki config.
func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
defaults := ConfigWrapper{}
flagext.DefaultValues(&defaults)

return func(dst cfg.Cloneable) error {
r, ok := dst.(*ConfigWrapper)
if !ok {
return errors.New("dst is not a Loki ConfigWrapper")
}

// Apply all our custom logic here to set values in the Loki config from values in the common config
if r.Common.PathPrefix != "" {
if r.Ruler.RulePath == defaults.Ruler.RulePath {
r.Ruler.RulePath = fmt.Sprintf("%s/rules", r.Common.PathPrefix)
}

if r.Ingester.WAL.Dir == defaults.Ingester.WAL.Dir {
r.Ingester.WAL.Dir = fmt.Sprintf("%s/wal", r.Common.PathPrefix)
}
}

return nil
}
}
87 changes: 87 additions & 0 deletions pkg/loki/config_wrapper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package loki

import (
"flag"
"io/ioutil"
"os"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/grafana/loki/pkg/util/cfg"
)

func Test_CommonConfig(t *testing.T) {
testContext := func(configFileString string, args []string) (ConfigWrapper, ConfigWrapper) {
config := ConfigWrapper{}
fs := flag.NewFlagSet(t.Name(), flag.PanicOnError)

file, err := ioutil.TempFile("", "config.yaml")
Copy link
Member

Choose a reason for hiding this comment

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

It'd be good to remove this file via defer when the test completes.

defer func() {
os.Remove(file.Name())
}()

require.NoError(t, err)
_, err = file.WriteString(configFileString)
require.NoError(t, err)

configFileArgs := []string{"-config.file", file.Name()}
if args == nil {
args = configFileArgs
} else {
args = append(args, configFileArgs...)
}
err = cfg.DynamicUnmarshal(&config, args, fs)
require.NoError(t, err)

defaults := ConfigWrapper{}
freshFlags := flag.NewFlagSet(t.Name(), flag.PanicOnError)
err = cfg.DefaultUnmarshal(&defaults, args, freshFlags)
require.NoError(t, err)

return config, defaults
}

t.Run("common path prefix config", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: These look like good candidates for table tests.

Copy link
Collaborator

@trevorwhitney trevorwhitney Sep 30, 2021

Choose a reason for hiding this comment

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

TBH, I find table tests harder to read and understand. I think they're great for checking relationships between inputs and outputs that resemble a truth table, but for something like this I think it would make it more difficult to understand why a test fails.

For example, if the test testing that command line input takes precedence fails, the output will give me a line number for that assertion. When I go to that assertion, it won't be immediately clear why it failed because we'll just be passing tt.args to the function. In order to determine which input actually caused the failure I would have to copy the name of the failing test from my terminal and then grep for that in the array of inputs to correlate the failure to the correct input data.

Instead, structuring the test like this, all shared logic is still extracted to the testContext function, but now when a test fails I'm taken to a line with the failing input data right there above it (ie. args := []string{"-foo", "bar"}). IMO this makes it a lot easier to quickly understand why a test fails. Sure it may be a few more lines, but I will trade verbosity for readability all day, especially in tests.

Thoughts?

t.Run("does not override defaults for file paths when not provided", func(t *testing.T) {
configFileString := `---`
config, defaults := testContext(configFileString, nil)

assert.EqualValues(t, defaults.Ruler.RulePath, config.Ruler.RulePath)
assert.EqualValues(t, defaults.Ingester.WAL.Dir, config.Ingester.WAL.Dir)
})

t.Run("when provided, rewrites all default file paths to use common prefix", func(t *testing.T) {
configFileString := `---
common:
path_prefix: /opt/loki`
config, _ := testContext(configFileString, nil)

assert.EqualValues(t, "/opt/loki/rules", config.Ruler.RulePath)
assert.EqualValues(t, "/opt/loki/wal", config.Ingester.WAL.Dir)
})

t.Run("does not rewrite custom (non-default) paths passed via config file", func(t *testing.T) {
configFileString := `---
common:
path_prefix: /opt/loki
ruler:
rule_path: /etc/ruler/rules`
config, _ := testContext(configFileString, nil)

assert.EqualValues(t, "/etc/ruler/rules", config.Ruler.RulePath)
assert.EqualValues(t, "/opt/loki/wal", config.Ingester.WAL.Dir)
})

t.Run("does not rewrite custom (non-default) paths passed via the command line", func(t *testing.T) {
configFileString := `---
common:
path_prefix: /opt/loki`
config, _ := testContext(configFileString, []string{"-ruler.rule-path", "/etc/ruler/rules"})

assert.EqualValues(t, "/etc/ruler/rules", config.Ruler.RulePath)
assert.EqualValues(t, "/opt/loki/wal", config.Ingester.WAL.Dir)
})
})
}
2 changes: 2 additions & 0 deletions pkg/loki/loki.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/grafana/loki/pkg/distributor"
"github.com/grafana/loki/pkg/ingester"
"github.com/grafana/loki/pkg/ingester/client"
"github.com/grafana/loki/pkg/loki/common"
"github.com/grafana/loki/pkg/lokifrontend"
"github.com/grafana/loki/pkg/querier"
"github.com/grafana/loki/pkg/querier/queryrange"
Expand All @@ -52,6 +53,7 @@ type Config struct {
AuthEnabled bool `yaml:"auth_enabled,omitempty"`
HTTPPrefix string `yaml:"http_prefix"`

Common common.Config `yaml:"common,omitempty"`
Server server.Config `yaml:"server,omitempty"`
Distributor distributor.Config `yaml:"distributor,omitempty"`
Querier querier.Config `yaml:"querier,omitempty"`
Expand Down
27 changes: 5 additions & 22 deletions pkg/util/cfg/cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package cfg

import (
"flag"
"os"
"reflect"

"github.com/grafana/dskit/flagext"
Expand Down Expand Up @@ -44,27 +43,11 @@ func Unmarshal(dst Cloneable, sources ...Source) error {
return nil
}

// Parse is a higher level wrapper for Unmarshal that automatically parses flags and a .yaml file
func Parse(dst Cloneable) error {
return dParse(dst,
dDefaults(flag.CommandLine),
YAMLFlag(os.Args[1:], "config.file"),
Flags(),
)
}

// dParse is the same as Parse, but with dependency injection for testing
func dParse(dst Cloneable, defaults, yaml, flags Source) error {
// check dst is a pointer
v := reflect.ValueOf(dst)
if v.Kind() != reflect.Ptr {
return ErrNotPointer
}

// unmarshal config
// DefaultUnmarshal is a higher level wrapper for Unmarshal that automatically parses flags and a .yaml file
func DefaultUnmarshal(dst Cloneable, args []string, fs *flag.FlagSet) error {
return Unmarshal(dst,
defaults,
yaml,
flags,
Defaults(fs),
YAMLFlag(args, "config.file"),
Flags(args, fs),
)
}
8 changes: 4 additions & 4 deletions pkg/util/cfg/cfg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ tls:
flagSource := dFlags(fs, []string{"-verbose", "-server.port=21"})

data := Data{}
err := dParse(&data,
dDefaults(fs),
err := Unmarshal(&data,
Defaults(fs),
yamlSource,
flagSource,
)
Expand Down Expand Up @@ -55,8 +55,8 @@ tls:
flagSource := dFlags(fs, []string{"-verbose", "-server.port=21"})

data := Data{}
err := dParse(&data,
dDefaults(fs),
err := Unmarshal(&data,
Defaults(fs),
yamlSource,
flagSource,
)
Expand Down
36 changes: 36 additions & 0 deletions pkg/util/cfg/dynamic.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package cfg

import (
"flag"
)

// DynamicCloneable must be implemented by config structs that can be dynamically unmarshalled
type DynamicCloneable interface {
Cloneable
ApplyDynamicConfig() Source
}

// DynamicUnmarshal handles populating a config based on the following precedence:
// 1. Defaults provided by the `RegisterFlags` interface
// 2. Sections populated by dynamic logic. Configs passed to this function must implement ApplyDynamicConfig()
// 3. Any config options specified directly in the config file
// 4. Any config options specified on the command line.
func DynamicUnmarshal(dst DynamicCloneable, args []string, fs *flag.FlagSet) error {
return Unmarshal(dst,
// First populate the config with defaults including flags from the command line
Defaults(fs),
// Next populate the config from the config file, we do this to populate the `common`
// section of the config file by taking advantage of the code in YAMLFlag which will load
// and process the config file.
YAMLFlag(args, "config.file"),
// Apply any dynamic logic to set other defaults in the config. This function is called after parsing the
// config files so that values from a common, or shared, section can be used in
// the dynamic evaluation
dst.ApplyDynamicConfig(),
// Load configs from the config file a second time, this will supersede anything set by the common
// config with values specified in the config file.
YAMLFlag(args, "config.file"),
// Load the flags again, this will supersede anything set from config file with flags from the command line.
Flags(args, fs),
)
}
Loading