Allow config options in file as yaml mappings #1737

alejandrodnm · 2022-11-03T14:48:13Z

Replaces the ff package with a custom implementation that uses Viper to parse the config file and sets the values for the flag.Flags.

By design the ff package doesn't allow flag subsets or nested maps inside the config yaml and all config options must be explicitely defined as flag.Flag. This limits the ability to specify more dynamic config options like lists or maps of user defined rules (Ex: metric rollups).

Viper provides the flexibility we require for parsing the config file, and unmarshaling directly into more complex datastructures. To define how to unmarshal a subtree of the configuration into a datastructure via unmarshalRule:

withUnmarshalRules(
  []unmarshalRule{{"startup.dataset", &dataset.Config}}
)

Given the the config file contains the subtree:

startup:
  dataset:
    metrics:
      ...
    traces:
      ...

Viper can access a nested field by passing a . delimited path of keys. In this scenario, the subtree startup.dataset will be unmarshal into &dataset.Config by Viper using the github.com/mitchellh/mapstructure package.

This change also allows us to format our configuration files to use yaml mappings instead of long key names with ., thus maintaining backwards compatibility:

web.listen-address: localhost:9201
web.auth.password: my-password
web.auth.username: promscale

Can be rewritten as:

web:
  listen-address: localhost:9201
  auth:
    password: my-password
    username: promscale

Merge requirements

Please take into account the following non-code changes that you may need to make with your PR:

CHANGELOG entry for user-facing changes
Updated the relevant documentation

pkg/runner/client.go

alejandrodnm · 2022-11-03T15:16:02Z

pkg/dataset/config.go

+	ChunkInterval   DayDuration `mapstructure:"default_chunk_interval" yaml:"default_chunk_interval"`
+	Compression     *bool       `mapstructure:"compress_data" yaml:"compress_data"` // Using pointer to check if the the value was set.
+	HALeaseRefresh  DayDuration `mapstructure:"ha_lease_refresh" yaml:"ha_lease_refresh"`
+	HALeaseTimeout  DayDuration `mapstructure:"ha_lease_timeout" yaml:"ha_lease_timeout"`
+	RetentionPeriod DayDuration `mapstructure:"default_retention_period" yaml:"default_retention_period"`
 }

 // Traces contains dataset configuration options for traces data.
 type Traces struct {
-	RetentionPeriod DayDuration `yaml:"default_retention_period"`
+	RetentionPeriod DayDuration `mapstructure:"default_retention_period" yaml:"default_retention_period"`


These can be exposed as command line flags, is there a reason why they're not?

@Harkishen-Singh @niksajakovljevic ?

These can be exposed as command line flags, is there a reason why they're not?

I think we see dataset config as a place for those flags that are purely related to DB (hence dataset), which is why these are not kept as flags.

cevian

Overall LGTM. I want to ask @antekresic to review as well.

antekresic

Looks good overall, some minor tweak suggestions and a test case to add.

pkg/dataset/duration.go

pkg/runner/flags.go

pkg/runner/client.go

docs/dataset.md

pkg/runner/config_parser.go

CHANGELOG.md

pkg/runner/flags_test.go

alejandrodnm · 2022-11-07T08:24:52Z

cc @paulfantom this is the PR to allow a yaml mapping instead of a string for the dataset config

Replaces the `ff` package with a custom implementation that uses Viper to parse the config file and sets the values for the `flag.Flag`s. By design the ff package doesn't allow flag subsets or nested maps inside the config yaml and all config options must be explicitely defined as `flag.Flag`. This limits the ability to specify more dynamic config options like lists or maps of user defined rules (Ex: metric rollups). Viper provides the flexibility we require for parsing the config file, and unmarshaling directly into more complex datastructures. To define how to unmarshal a subtree of the configuration into a datastructure via unmarshalRule. An unmarshalRule for the dataset config was added as: ``` withUnmarshalRules( []unmarshalRule{{"startup.dataset", &dataset.Config}} ) ``` Given a config file that contains the subtree: ``` startup: dataset: metrics: ... traces: ... ``` The subtree `startup.dataset` will be unmarshal into `&dataset.Config` by Viper using the github.com/mitchellh/mapstructure package. Viper can access a nested field by passing a `.` delimited path of keys. This change also allows us to format our configuration files to use yaml mappings instead of long key names with `.`, thus maintaining backwards compatibility: ``` web.listen-address: localhost:9201 web.auth.password: my-password web.auth.username: promscale ``` Can be rewritten as: ``` web: listen-address: localhost:9201 auth: password: my-password username: promscale ```

pkg/runner/flags.go

docs/configuration.md

alejandrodnm commented Nov 3, 2022

View reviewed changes

pkg/runner/client.go Outdated Show resolved Hide resolved

alejandrodnm force-pushed the adn/dataset-as-map branch from deb1c6b to c5afb5f Compare November 3, 2022 15:14

alejandrodnm commented Nov 3, 2022

View reviewed changes

alejandrodnm force-pushed the adn/dataset-as-map branch 2 times, most recently from 803c9cc to 6829137 Compare November 3, 2022 15:22

alejandrodnm marked this pull request as ready for review November 3, 2022 15:40

alejandrodnm requested a review from a team as a code owner November 3, 2022 15:40

cevian approved these changes Nov 3, 2022

View reviewed changes

cevian requested a review from antekresic November 3, 2022 17:44

alejandrodnm force-pushed the adn/dataset-as-map branch from 6829137 to b7c40eb Compare November 4, 2022 09:00

antekresic reviewed Nov 4, 2022

View reviewed changes

alejandrodnm force-pushed the adn/dataset-as-map branch 2 times, most recently from f2ebbaa to 57662ae Compare November 4, 2022 15:59

alejandrodnm requested a review from antekresic November 7, 2022 08:22

alejandrodnm force-pushed the adn/dataset-as-map branch 4 times, most recently from e040fc6 to b5dca9a Compare November 14, 2022 11:19

alejandrodnm force-pushed the adn/dataset-as-map branch from b5dca9a to ed4d8da Compare November 14, 2022 11:25

antekresic reviewed Nov 15, 2022

View reviewed changes

pkg/runner/flags.go Show resolved Hide resolved

docs/configuration.md Show resolved Hide resolved

Add deprecation warning for startup.dataset.config

0156836

alejandrodnm force-pushed the adn/dataset-as-map branch from ed4d8da to 0156836 Compare November 15, 2022 13:11

antekresic approved these changes Nov 15, 2022

View reviewed changes

alejandrodnm merged commit 9df822a into master Nov 15, 2022

alejandrodnm deleted the adn/dataset-as-map branch November 15, 2022 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow config options in file as yaml mappings #1737

Allow config options in file as yaml mappings #1737

alejandrodnm commented Nov 3, 2022 •

edited

Loading

alejandrodnm Nov 3, 2022 •

edited

Loading

cevian Nov 3, 2022

Harkishen-Singh Nov 9, 2022

cevian left a comment

antekresic left a comment

alejandrodnm commented Nov 7, 2022

Allow config options in file as yaml mappings #1737

Allow config options in file as yaml mappings #1737

Conversation

alejandrodnm commented Nov 3, 2022 • edited Loading

Merge requirements

alejandrodnm Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

cevian Nov 3, 2022

Choose a reason for hiding this comment

Harkishen-Singh Nov 9, 2022

Choose a reason for hiding this comment

cevian left a comment

Choose a reason for hiding this comment

antekresic left a comment

Choose a reason for hiding this comment

alejandrodnm commented Nov 7, 2022

alejandrodnm commented Nov 3, 2022 •

edited

Loading

alejandrodnm Nov 3, 2022 •

edited

Loading