WIP: configuration cleanup. #141

igorpeshansky · 2021-08-04T19:57:38Z

We can pick-and-choose individual commits from this draft PR into separate PRs, as needed. I've tried to keep the commits small, and it may make sense to review them one at a time.

…nux.

Rename fluentbit package to "fluentbit".

…arameters.

…ver interface.

…l's Stackdriver into the otel.Exporter interface. Use the otel.Processor and otel.Exporter interfaces instead of the processor/exporter name maps.

… replicates what we have in the ops agent config.

This is a pure reordering, with no functional changes.

qingling128

Not done with the full review yet, but the first 8 commits seem pretty straight forward and LGTM with one minor question (They could potentially be broken to a separate PR):

10a44bc (Add copyright notice to the otel test.)

207bf72 (Fix invalid receiver type in test input.)

81d08a7 (Fix typos in exporters warnings.)

045a1e9 (Add missing test for invalid logging and metrics receiver types on Linux.)

f503293 (Add flow annotations to make the generated configs a bit nicer.)

5f31d91 (Move fluentbit and otel config generation under confgenerator.)

1201e3d (Annotate optional fields with the config:"optional" tag.)

9311420 (Rename "component" to "kind" everywhere.)

confgenerator/config.go

qingling128

Just reviewed the rest (Will defer Golang readability to other folks).

The following commits LGTM:

5fd4832 (Rename type map to registry; embed subagent and kind in the registry.)
6441afb (Embed Type in LoggingReceiver.)
3872f37 (Move custom parameter validation for logging receivers into ValidateParameters.)
1291686 (Reorder functions to group metrics generation and logging generation.)
2698657 (Inline extract.*Factories functions.)
Eliminate factories
- c387012 (Eliminate otel receiver factories.)
- c2cb23a (Eliminate otel processor factories.)
- 01fd6d3 (Eliminate fluentbit receiver factories.)
294d2a8 (Don't mangle otel entity ids.)
24d757d (Cosmetic: reorder otel structs for consistency.)
5d3a61a (Unify otel's HostMetrics, IIS, and MSSQL structs under the otel.Receiver interface.)
29c4b96 (Use the otel.Receiver interface instead of the receiver name map.)
eca881b (Wrap otel's ExcludeMetrics into the otel.Processor interface, and otel's Stackdriver into the otel.Exporter interface.)
ccd3a5d (Use lists instead of strings in otel.Service representation.)
2a23768 (Reorder fluentbit templates to match the corresponding config type.)
211673a (Render fluentbit config from a single set of templates.)
084affa (Use fluentbit.Config structs to generate configuration.)
cfe8e45 (Remove internal validation from the fluentbit config generator, as it replicates what we have in the ops agent config.)
e34f779 (Use consistent ordering of options in fluent-bit configs.)

Left a comment for the change that are achieved by the commits below. There is some UX concern as the error message is less helpful after the change.

5288ebf (Use a custom unmarshaler for the logging receiver types.)
cce48d9 (Use a custom unmarshaler for the logging processor types.)
dd625cc (Use a custom unmarshaler for the logging exporter types.)
261dbb5 (Use a custom unmarshaler for the metrics receiver types.)
cfbbafe (Use a custom unmarshaler for the metrics processor types.)
c123eaa (Use a custom unmarshaler for the metrics exporter types.)
3440618 (Factor out common type registration code.)

Left a minor comment for this commit:

a8e6f6d (Sort input fluentbit config entities instead of the generated text.)

qingling128 · 2021-08-06T00:48:00Z

...ata/invalid/linux/logging-receiver_files_type_unsupported_parameter_listen_host/golden_error

@@ -1 +1,2 @@
-parameter "listen_host" in "files" type logging receiver "receiver_1" is not supported. Supported parameters: [include_paths, exclude_paths].


Hmm... This feels like degraded user experience, as the error message no longer has an ID that users can use to easily refer back to a specific section of the configuration file. It sounds like this might be addressed later though? (@quentinmit mentioned at standup today that you two found a way to print the few lines of the config that is problematic.

Yes, this is a step backward. I actually have an experimental branch where I get closer to the original error messages, with two caveats: our errors expect to know the key in the map (i.e., the entity id), and our errors rely on knowing the platform we're verifying for. neither of those is possible to propagate into the custom unmarshaler with the standard yaml libraries. @quentinmit found an alternative yaml library that may offer more control, but switching will incur a cost.

Yeah, I'm going to experiment with that. See my separate comment thread about struct tag syntax for a link to validation. This yaml parser emits detailed parse errors with line and column numbers: https://pkg.go.dev/github.com/goccy/go-yaml#readme-4-pretty-formatted-errors

The issue is not even the line numbers in the yaml source, but also the information about the platform (linux vs windows) and the map keys (entity ids) that our original error messages expect to be able to provide to the users. For a preliminary attempt to reconstruct the original errors, see the igorpeshansky-config-cleanup-errors branch.

Folded that branch into the PR.

confgenerator/fluentbit/conf.go

qingling128 · 2021-08-06T01:17:41Z

confgenerator/confgenerator.go

@@ -286,6 +287,17 @@ func (uc *UnifiedConfig) GenerateFluentBitConfigs(logsDir string, stateDir strin
 			return "", "", err
 		}
 	}
+
+	// make sure all collections are sorted so that generated configs are consistently generated
+	sort.Slice(fbTails, func(i, j int) bool { return fbTails[i].Tag < fbTails[j].Tag })


Might worth a shared method for the repeated logic here? The only caveat seems to be fbFilterParserGroups that has slightly different logic.

I suspect these will go away altogether in a future commit, because we can generate these in source config order.

… fluentbit.Input interface. Unify fluentbit's filters under the fluentbit.Filter interface, and fluentbit's parsers under the fluentbit.Parser interface. Wrap fluentbit's Stackdriver into the fluentbit.Output interface. Use the fluentbit.Input, fluentbit.Filter, fluentbit.Output, and fluentbit.Parser interfaces instead of the individual structs. Template expansion whitespace control is hell, so there are no longer two blank lines at the end of the config. No other changes to the generated configs.

quentinmit · 2021-08-06T05:50:50Z

confgenerator/confgenerator.go

@@ -96,55 +94,272 @@ func (uc *UnifiedConfig) GenerateOtelConfig(hostInfo *host.InfoStat) (string, er
 	return otelConfig, nil
 }

-func generateOtelServices(receiverNameMap map[string]string, exporterNameMap map[string]string, processorNameMap map[string]string, pipelines map[string]*MetricsPipeline) ([]*otel.Service, error) {
+func generateOtelReceivers(receivers map[string]MetricsReceiver, pipelines map[string]*MetricsPipeline) ([]otel.Receiver, map[string]otel.Receiver, error) {


All of this still needs to go away, though I understand biting off one thing at a time.

I'm slowly working up towards that goal. The idea is to be able to eventually produce these auxiliary structs from methods on the config structs, rather than these switches, and go from there.

These auxiliary structs should eventually go away entirely, not be generated by a different place.

That may be possible for otel, by making the otel config representation structs from the library the auxiliary structs here. For fluentbit, we would still need the auxiliary structs.

quentinmit · 2021-08-06T05:52:09Z

confgenerator/confgenerator.go

+// GenerateFluentBitConfigs generates FluentBit configuration from unified agents configuration
+// in yaml. GenerateFluentBitConfigs returns empty configurations without an error if `logging`
+// does not exist as a top-level field in the input yaml format.
+func (uc *UnifiedConfig) GenerateFluentBitConfigs(logsDir string, stateDir string, hostInfo *host.InfoStat) (string, string, error) {


This is an awful lot of repeated boilerplate code. Why does this need a separate struct for each of these? Can't we emit a single struct, or at least one struct for filters, another for inputs, etc.?

Getting there.

quentinmit · 2021-08-06T05:53:30Z

confgenerator/config.go

+type component interface {
+	Type() string
+	ValidateType(subagent string, component string, id string, platform string) error
+	ValidateParameters(subagent string, component string, id string) error


Validation shouldn't be a required part of the interface; it should be optional, only if the validation cannot be accomplished with struct tags alone.

This is in part due to the validation being specific to the platform that the config is generated for. Using a library that would allow passing a context object through would remove the need for separate functions in the interface.

The validation library I linked above allows that.

But the yaml library doesn't seem to — at least I didn't find a way to propagate the context into the unmarshaler. Looking forward to seeing your prototype.

confgenerator/config.go

quentinmit · 2021-08-06T05:57:34Z

confgenerator/otel/conf.go

 type IIS struct {
 	IISID              string
 	CollectionInterval string
 }

-type HostMetrics struct {
-	HostMetricsID      string
+func (r *IIS) Generate() (string, error) {


I want to replace this with just returning the otel config structs directly, but happy to defer that to a separate PR. Passing around snippets of YAML text is begging for trouble.

Agreed on the "begging for trouble" bit, but let's defer to a separate PR.

quentinmit · 2021-08-06T05:58:55Z

...ata/invalid/linux/logging-receiver_files_type_unsupported_parameter_listen_host/golden_error

@@ -1 +1,2 @@
-parameter "listen_host" in "files" type logging receiver "receiver_1" is not supported. Supported parameters: [include_paths, exclude_paths].


Yeah, I'm going to experiment with that. See my separate comment thread about struct tag syntax for a link to validation. This yaml parser emits detailed parse errors with line and column numbers: https://pkg.go.dev/github.com/goccy/go-yaml#readme-4-pretty-formatted-errors

…nfig:"optional"`).

…ime.

igorpeshansky · 2021-08-06T19:17:55Z

#142 has been merged. Rebased the rest on top of the new master and created #143 for the config representation commits, #145 for the otel config generation ones, and #146 for the fluentbit config generation ones.

qingling128 · 2021-08-23T21:48:39Z

Is this one still relevant or all extracted and merged now?

github-actions · 2025-01-25T03:02:59Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

igorpeshansky requested a review from quentinmit August 4, 2021 19:57

igorpeshansky added 24 commits August 4, 2021 16:08

Add copyright notice to the otel test.

10a44bc

Fix invalid receiver type in test input.

207bf72

Fix typos in exporters warnings.

81d08a7

Add missing test for invalid logging and metrics receiver types on Li…

045a1e9

…nux.

Add flow annotations to make the generated configs a bit nicer.

f503293

Move fluentbit and otel config generation under confgenerator.

5f31d91

Rename fluentbit package to "fluentbit".

Annotate optional fields with the config:"optional" tag.

1201e3d

Rename "component" to "kind" everywhere.

9311420

Use a custom unmarshaler for the logging receiver types.

5288ebf

Rename type map to registry; embed subagent and kind in the registry.

5fd4832

Embed Type in LoggingReceiver.

6441afb

Move custom parameter validation for logging receivers into ValidateP…

3872f37

…arameters.

Use a custom unmarshaler for the logging processor types.

cce48d9

Use a custom unmarshaler for the logging exporter types.

dd625cc

Use a custom unmarshaler for the metrics receiver types.

261dbb5

Use a custom unmarshaler for the metrics processor types.

cfbbafe

Use a custom unmarshaler for the metrics exporter types.

c123eaa

Factor out common type registration code.

3440618

Reorder functions to group metrics generation and logging generation.

1291686

Inline extract.*Factories functions.

2698657

Eliminate otel receiver factories.

c387012

Eliminate otel processor factories.

c2cb23a

Eliminate fluentbit receiver factories.

01fd6d3

Don't mangle otel entity ids.

294d2a8

igorpeshansky force-pushed the igorpeshansky-config-cleanup branch from 04fa52b to 5d4be27 Compare August 4, 2021 20:20

igorpeshansky added 2 commits August 4, 2021 16:26

Cosmetic: reorder otel structs for consistency.

24d757d

Unify otel's HostMetrics, IIS, and MSSQL structs under the otel.Recei…

5d3a61a

…ver interface.

igorpeshansky force-pushed the igorpeshansky-config-cleanup branch from 5d4be27 to 5d3a61a Compare August 4, 2021 20:27

Use the otel.Receiver interface instead of the receiver name map.

29c4b96

igorpeshansky added 8 commits August 4, 2021 21:48

Wrap otel's ExcludeMetrics into the otel.Processor interface, and ote…

eca881b

…l's Stackdriver into the otel.Exporter interface. Use the otel.Processor and otel.Exporter interfaces instead of the processor/exporter name maps.

Use lists instead of strings in otel.Service representation.

ccd3a5d

Reorder fluentbit templates to match the corresponding config type.

2a23768

Render fluentbit config from a single set of templates.

211673a

Use fluentbit.Config structs to generate configuration.

084affa

Remove internal validation from the fluentbit config generator, as it…

cfe8e45

… replicates what we have in the ops agent config.

Use consistent ordering of options in fluent-bit configs.

e34f779

This is a pure reordering, with no functional changes.

Sort input fluentbit config entities instead of the generated text.

a8e6f6d

qingling128 reviewed Aug 6, 2021

View reviewed changes

confgenerator/config.go Outdated Show resolved Hide resolved

qingling128 reviewed Aug 6, 2021

View reviewed changes

igorpeshansky added 3 commits August 5, 2021 22:06

Switch to explicit template rendering for fluentbit config components.

e6cda07

Standardize on the Diff library and the order of diffs.

c08fcf2

quentinmit reviewed Aug 6, 2021

View reviewed changes

igorpeshansky added 3 commits August 6, 2021 12:41

Use validate:"required" to indicate required fields (instead of `co…

ef84963

…nfig:"optional"`).

WIP: Attempt to reconstruct previous validation error messages.

14d94c3

Emit a more neutral error message on validation errors at unmarshal t…

7e78333

…ime.

This was referenced Aug 6, 2021

Some initial cleanups of the config code to prepare for the larger refactoring. #142

Merged

Cleanup configuration representation. #143

Merged

This was referenced Aug 18, 2021

Clean up configuration generation for otel configs. #145

Merged

Clean up configuration generation for fluentbit configs. #146

Merged

github-actions bot added the Stale label Jan 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: configuration cleanup. #141

WIP: configuration cleanup. #141

igorpeshansky commented Aug 4, 2021

qingling128 left a comment

qingling128 left a comment •

edited

Loading

qingling128 Aug 6, 2021

igorpeshansky Aug 6, 2021 •

edited

Loading

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021 •

edited

Loading

igorpeshansky Aug 6, 2021

qingling128 Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky Aug 6, 2021

quentinmit Aug 6, 2021

igorpeshansky commented Aug 6, 2021 •

edited

Loading

qingling128 commented Aug 23, 2021

github-actions bot commented Jan 25, 2025

		@@ -1 +1,2 @@
		parameter "listen_host" in "files" type logging receiver "receiver_1" is not supported. Supported parameters: [include_paths, exclude_paths].

WIP: configuration cleanup. #141

Are you sure you want to change the base?

WIP: configuration cleanup. #141

Conversation

igorpeshansky commented Aug 4, 2021

qingling128 left a comment

Choose a reason for hiding this comment

qingling128 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky Aug 6, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

igorpeshansky commented Aug 6, 2021 • edited Loading

qingling128 commented Aug 23, 2021

github-actions bot commented Jan 25, 2025

qingling128 left a comment •

edited

Loading

igorpeshansky Aug 6, 2021 •

edited

Loading

igorpeshansky Aug 6, 2021 •

edited

Loading

igorpeshansky commented Aug 6, 2021 •

edited

Loading