Loki: Common Config #4347

slim-bean · 2021-09-17T12:27:47Z

This PR sets out to add a new common section to Loki's config file which is to be used for simplifying Loki's config for most use cases by applying a set of opinionated defaults for as many config options as possible covering a vast majority of Loki use cases.

Checklist

Documentation added
Tests updated

…outside of this package. create a pkg/cfg which handles building the Loki config, this is created separately from pkg/util (which is Apache 2 licensed) to maintain proper separations of AGPL and Apache 2 code Update the main.go for several of the apps with the refactoring

slim-bean · 2021-09-17T13:58:14Z

pkg/cfg/cfg.go

+
+		// Apply all our custom logic here to set values in the Loki config from values in the common config
+		// FIXME this is just an example showing how we can use values from the common section to set values on the Loki config object
+		r.StorageConfig.BoltDBShipperConfig.SharedStoreType = r.Common.Store


Here we can do anything needed to map common configs to the Loki configs

slim-bean · 2021-09-17T13:58:29Z

pkg/cfg/common/common.go

+package common
+
+type Config struct {
+	Store string // This is just an example, but here we should define all the 'common' config values used to set other Loki values.


Here we can define the common config struct

trevorwhitney · 2021-09-17T21:27:21Z

I'd like to see a ring config make it into common. Redefining the same ring kv config for each component is tedious, since most deployments typically use 1 kv type for all their rings.

09jvilla · 2021-09-20T18:46:06Z

@slim-bean does this just create a common config, or does it also set some reasonable defaults in that common config? I'm wondering if we need to leave
https://github.com/grafana/backend-enterprise/issues/2173 (Set chunk target size to 1MB by default)
open or if it is covered in this draft PR.

(My rough glance as this PR suggests it only puts the common config in place, but doesn't redefine any of the defaults).

slim-bean · 2021-09-20T19:39:52Z

@09jvilla this was mostly just a POC to make sure this mechanism would work. We can do whatever we want with whatever defaults we want in here. So we could definitely add some code for setting the chunk_target_size

replay · 2021-09-20T23:29:12Z

I think this mechanism makes sense.
Unfortunately it's really a bit hacky how cfg.YAMLFlag(os.Args[1:], "config.file") has to be called twice, but I can't think of a better way to do this, so let's go with it.

If that's fine with you @slim-bean then i'll push into this PR's branch to add more common configs?

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

owen-d · 2021-09-29T12:23:36Z

Looks reasonable to me 👍

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney · 2021-09-29T16:10:17Z

@slim-bean @replay @owen-d I refactored the implementation a bit, added tests, and added an example of how this will work using a common path prefix config. Let me know what you think, thanks!

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

pkg/loki/config_wrapper.go

replay

LGTM

owen-d

A few nits, then LGTM

pkg/loki/common/common.go

pkg/loki/config_wrapper.go

pkg/loki/config_wrapper_test.go

owen-d · 2021-09-30T13:46:45Z

pkg/loki/config_wrapper_test.go

+		config := ConfigWrapper{}
+		fs := flag.NewFlagSet(t.Name(), flag.PanicOnError)
+
+		file, err := ioutil.TempFile("", "config.yaml")


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

owen-d · 2021-09-30T13:47:57Z

pkg/loki/config_wrapper_test.go

+		return config, defaults
+	}
+
+	t.Run("common path prefix config", func(t *testing.T) {


nit: These look like good candidates for table tests.

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?

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

pull-request-size bot added the size/L label Sep 17, 2021

slim-bean force-pushed the loki-common-config branch from 476fa1e to cb9c670 Compare September 17, 2021 13:28

slim-bean commented Sep 17, 2021

View reviewed changes

refactor how dynamic/common config is loaded

b16004b

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney added 4 commits September 29, 2021 09:59

Add common config logic for path prefix

4aaf99c

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

Missed a file, update common config with prefix property

81a8b1b

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

fix test, remove loki from args

c0afa15

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

Clean up test names

c4c9b2c

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney marked this pull request as ready for review September 29, 2021 16:10

trevorwhitney requested a review from a team as a code owner September 29, 2021 16:10

fix typo in filename, make linter happy

780efaf

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

replay reviewed Sep 29, 2021

View reviewed changes

pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved

replay approved these changes Sep 29, 2021

View reviewed changes

owen-d approved these changes Sep 30, 2021

View reviewed changes

trevorwhitney and others added 4 commits September 30, 2021 15:24

Update pkg/loki/config_wrapper.go

317efba

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Update pkg/loki/config_wrapper.go

3404c95

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Update pkg/loki/config_wrapper_test.go

d993c18

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

remove temp files in test

1140046

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

owen-d merged commit 11e215d into main Oct 1, 2021

owen-d deleted the loki-common-config branch October 1, 2021 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Common Config #4347

Loki: Common Config #4347

slim-bean commented Sep 17, 2021 •

edited by replay

Loading

slim-bean Sep 17, 2021

slim-bean Sep 17, 2021

trevorwhitney commented Sep 17, 2021

09jvilla commented Sep 20, 2021 •

edited

Loading

slim-bean commented Sep 20, 2021

replay commented Sep 20, 2021

owen-d commented Sep 29, 2021

trevorwhitney commented Sep 29, 2021

replay left a comment

owen-d left a comment

owen-d Sep 30, 2021

owen-d Sep 30, 2021

trevorwhitney Sep 30, 2021 •

edited

Loading

Loki: Common Config #4347

Loki: Common Config #4347

Conversation

slim-bean commented Sep 17, 2021 • edited by replay Loading

slim-bean Sep 17, 2021

Choose a reason for hiding this comment

slim-bean Sep 17, 2021

Choose a reason for hiding this comment

trevorwhitney commented Sep 17, 2021

09jvilla commented Sep 20, 2021 • edited Loading

slim-bean commented Sep 20, 2021

replay commented Sep 20, 2021

owen-d commented Sep 29, 2021

trevorwhitney commented Sep 29, 2021

replay left a comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

owen-d Sep 30, 2021

Choose a reason for hiding this comment

owen-d Sep 30, 2021

Choose a reason for hiding this comment

trevorwhitney Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

slim-bean commented Sep 17, 2021 •

edited by replay

Loading

09jvilla commented Sep 20, 2021 •

edited

Loading

trevorwhitney Sep 30, 2021 •

edited

Loading