Promtail config conversion - part 2

[thampiotr]

PR Description

• Adds further support for promtail config conversions
  • All the SD features
  • Some of the logs producing elements, e.g. loki push api
• Increases the testing coverage by attempting to load Flow config
• Fixes an issue with prometheus.discovery.relabel using wrong exported field name
• Fixes an issue with cloudflare typo

Notes to the Reviewer

Work still left (see TODO comments):

• Remaining logs producing features
• Some top-level features
• Remaining loki.process stages

[thampiotr]

Found this issue thanks to adding an attempt to load flow config in testing.go.

[thampiotr]

Also found this thanks to attempting to load Flow config in testing.go.

[thampiotr]

Prints all the diags that are left, so one can more quickly fix the expectations.

[thampiotr]

Catches the case where there is a typo in file names.

[erikbaranowski]

Looking solid. There are some questions on consistency between the converters I'd like to see us come to a consensus on so that we can jump between the converters in code with as little context switching as we can.

[erikbaranowski]

I like the builder concept a lot and I'm thinking about it some more. It's not in this changeset, but I wonder if the build.GlobalContext should have the cfg *promtailcfg.Config on it rather than the more specific TargetSyncPeriod. You can then use that as the context for anything you need from the entire config rather than specific properties that have to be wired in. WDYT?

[thampiotr]

I think that if we have been using more fields (e.g. more than 3?) from the global context, that would certainly make sense.

Currently I suspect this will be quite limited with just a few fields and adding a large struct with everything in it such as cfg *promtailcfg.Config may result in larger cognitive load when reading this IMO. Seeing that there are only a few fields in the GlobalContext, I can quickly see that only few things are considered global.

[thampiotr]

I like the builder concept a lot and I'm thinking about it some more.

Thanks! One thing that builder is good for is a long pipeline where some steps are optional. I can have discovery.* -> discovery.relabel -> local.file_match -> loki.source.file -> loki.process -> loki.write and some of these intermediate components are optional. With the builder pattern I can skip a stage in the pipeline without too much code.

[erikbaranowski]

prometheus has a similar case too chaining together potential components up to 5

[erikbaranowski]

I'm commenting on this line just to get the conversation going.

I think we should discuss and narrow in a consistent approach is for adding diags during the conversion. It seems like we currently have too much variance in how we do things which makes it hard for me to follow along if I'm looking at the converter validations:

• a general validate.go in the *convert package
• various validate* funcs in flow component specific files, ie. prometheusconvert.validatePrometheusScrape which returns diag.Diagnostics
• adding them directly to the property on a builder, ie. build.AppendKubernetesSDs
• passing a *diag.Diagnostics into another func, ie. build.convertStage

Not to suggest this is the best final pattern especially since I am interested in the builder pattern but the current pattern I have in place for prom/static converters exists like this:

• Any diags for the main parsing is done in the top level prometheusconvert.Convert func
• validation diags aren't added while appending components (maybe this is too strict for all cases, but it does separate the responsibilities)
• validation.go provides a top level validate func that returns all validation diags not covered by prometheusconvert.Convert
  • The validation is ordered by the order of the properties in the source config type
  • Any non-component specific validation exists in this file
  • Any component specific validation calls a validation func which exists in that component's specific convert file

[thampiotr]

Yeah, I've noticed these differences too and I agree we could bring things to be a bit more consistent (this or preferably a future PR).

However there are a few reasons why I am hesitant about a one big validate step followed by building the config separately:

• It spreads the code around a bit... I would prefer if adding a new feature support was ideally in one place. This is somewhat alleviated by the convention of a validate* function though in the same file as the conversion code.
• However, it is still possible to forget to call validation or to forget to write it.
• In one-step validation approach, some issues to validate would require iterating over arrays and layers of nil checking and reaching deep into the config struct, for example this one is in scrape_configs[*].journal.max_age. Since we'll already be walking through this config later to convert it, I found it redundant to walk through it in order to validate it.

Here's an idea how I think we could make the validation and conversion more consistent:

We could enforce the interface for config fragments conversions and force via API always validating when converting - this way it is harder to forget to call validate. An example interface could be like this:

// Converts config from F to T after validating and providing diagnostics.
type ConfigConverter[F interface{}, T interface{}] interface {
	Convert(fCfg F) (T, diag.Diagnostics)
}

And an example implementation and using of it:

type KubernetesSDConverter struct {
}

func (k KubernetesSDConverter) Convert(fCfg promkubernetes.SDConfig) (kubernetes.Arguments, diag.Diagnostics) {
	// here goes the current implementation
	panic("implement me")
}

func actualConversionCode(fCfg promkubernetes.SDConfig, f *builder.File, diags *diag.Diagnostics) {
	args, newDiags := KubernetesSDConverter{}.Convert(fCfg)
	diags.AddAll(newDiags)
	f.Body().AppendBlock(common.NewBlockWithOverride([]string{"discovery", "kubernetes"}, "default", args))
}

Alternatively I can refactor my code to match the pattern we have in the other converters - it may end up being a bit more verbose, but it'd be a feasible option too.

Lastly we can leave the mixed approach - I agree being consistent is better, but calling validation is also not the most complex part around here, so I think it is not too much of an issue.

[erikbaranowski]

for future us: we talked offline about moving forward as is for now and thinking about a refactor on some of this stuff after things settle. both approaches are very functional, and we can easily test any refactor.

[erikbaranowski]

LGTM