Replace Fluentbit config generation with modular implementation #164

quentinmit · 2021-08-24T06:30:47Z

No description provided.

igorpeshansky

Some preliminary comments (only on the commits that are on top of #163).

confgenerator/confgenerator.go

igorpeshansky · 2021-08-24T17:48:07Z

confgenerator/fluentbit/modular.go

Would help to name the output parameters…

That's not what named output parameters are for. Go guidance is that you should not name output parameters unless you specifically need them.

I didn't find any explicit guidance in either the Go style or the Go best practices on the named parameters. However, Go is supposed to be all about not surprising readers, and not specifying which of the strings is which opens up a possibility of mixing them up. Would you mind pointing me at the guidance about output parameters that you're referencing?

The relevant style entry is https://golang.org/doc/effective_go#named-results (plus more internal guidance that I can't link to in a public PR). I agree that's not a strong recommendation; I have personally been given the explicit feedback in readability reviews that named result parameters should not be used unless those names are needed inside the function.

This function's return parameters will obviously be documented with a comment.

I'm confused. The guidance you linked actually says:

The names are not mandatory but they can make code shorter and clearer: they're documentation. If we name the results of nextInt it becomes obvious which returned int is which.

which seems very similar to the current situation. As I understand it, you don't even have to use a bare return with named results, though you could.

I can see that the names would not be used inside the function, but the documentation value alone would, IMO, be sufficient. Comments tend to fall out of sync with the code — having the facility to do this in the code itself is something that should be taken advantage of.

That said, I may be missing the experience to recognize the right idioms here. Go style recommendations have been nonsensical to me in the past, and this could be yet another one of those.

My understanding from going through the readability process is that naming result parameters makes it possible to use a bare return, and if you're not actually using that in the function body, you should not use named result parameters to prevent an accidental bare return.

I agree this seems like a fairly weak argument. I think I'm fine going against that guidance if you really want. (Note that this function signature is not new and it didn't have named result parameters before...)

At a minimum, add a function comment describing the return values.

You're welcome to name the return values too even if you're not doing bare return. (And definitely don't use bare return. They should get rid of that feature.)

Right. I'll leave this up to you — IMO, the named return parameters would be an aid to readability, but I may be projecting from other languages.

There's already a function comment documenting the return values.

confgenerator/fluentbit/modular.go

confgenerator/logging_processors.go

confgenerator/confgenerator.go

confgenerator/logging_receivers.go

quentinmit · 2021-08-24T23:21:37Z

I pushed the golden diffs, so I would appreciate a review of those looking for any bugs. Then I can rip out all the now-unused code.

igorpeshansky

The golden diffs look fine to me. Have some questions about the implementation.

confgenerator/confgenerator.go

...ta/valid/linux/all-backward_compatible_with_explicit_exporters/golden_fluent_bit_parser.conf

confgenerator/confgenerator.go

...ta/valid/linux/all-backward_compatible_with_explicit_exporters/golden_fluent_bit_parser.conf

confgenerator/confgenerator.go

confgenerator/testdata/valid/linux/logging-processor_order/golden_fluent_bit_parser.conf

...ta/valid/linux/all-backward_compatible_with_explicit_exporters/golden_fluent_bit_parser.conf

confgenerator/testdata/valid/linux/logging-processor_order/golden_fluent_bit_parser.conf

igorpeshansky · 2021-08-25T04:17:52Z

confgenerator/fluentbit/modular.go

I didn't find any explicit guidance in either the Go style or the Go best practices on the named parameters. However, Go is supposed to be all about not surprising readers, and not specifying which of the strings is which opens up a possibility of mixing them up. Would you mind pointing me at the guidance about output parameters that you're referencing?

confgenerator/confgenerator.go

quentinmit · 2021-08-25T07:03:32Z

I've toggled the draft bit off now, PTAL.

igorpeshansky · 2021-08-25T17:41:44Z

I'll do a more thorough review later, but at the moment there seems to be way too much fluent-bit-specific knowledge spread through the confgenerator code, which is probably better off confined to the fluentbit package…

davidbtucker

One initial comment.

confgenerator/confgenerator.go

quentinmit · 2021-08-25T19:20:24Z

I'll do a more thorough review later, but at the moment there seems to be way too much fluent-bit-specific knowledge spread through the confgenerator code, which is probably better off confined to the fluentbit package…

This is consistent with what happens on the OT side - each receiver or processor is responsible for generating its own OT or fluentbit configuration, using helpers when there is code to be shared, and then fluentbit/otel is responsible for serializing that config into a config file.

I'm not sure what you mean by "spread through" - the only code that touches fluentbit is in the GenerateFluentBitConfigs and generateFluentbitComponents methods in confgenerator.go, and the receiver/processor specific code in logging*.go. Note that some of the latter will be broken off into the apps package as well.

qingling128

LGTM functionality wise. Will defer Golang readability to others.

confgenerator/config.go

davidbtucker · 2021-08-25T19:58:16Z

confgenerator/fluentbit/modular.go

At a minimum, add a function comment describing the return values.

You're welcome to name the return values too even if you're not doing bare return. (And definitely don't use bare return. They should get rid of that feature.)

davidbtucker · 2021-08-25T20:46:55Z

confgenerator/logging.go

+
+// setLogNameComponents generates a series of components that rewrites the tag on log entries tagged `tag` to be `logName`.
+func setLogNameComponents(tag, logName string) []fluentbit.Component {
+	// TODO: Can we just set log_name_key in the output plugin and avoid this mess?


One note: The output plugin uses the tag to populate LogEntry.logName, which may be the reason for some of this mess.

Right, but the log_name_key setting overrides it. I still think we ought to just use the default value for that key, and add a record field called logging.googleapis.com/logName…

That's what my TODO is all about, but I don't want to make that functional change in the same PR.

confgenerator/logging.go

...ta/valid/linux/all-backward_compatible_with_explicit_exporters/golden_fluent_bit_parser.conf

igorpeshansky · 2021-08-25T20:52:17Z

confgenerator/fluentbit/modular.go

Right. I'll leave this up to you — IMO, the named return parameters would be an aid to readability, but I may be projecting from other languages.

confgenerator/config.go

confgenerator/logging.go

igorpeshansky · 2021-08-25T21:19:25Z

confgenerator/fluentbit/modular.go

 		lines = append(lines, fmt.Sprintf("    %-*s %s", maxLen, k, v))
 	}
 	sort.Strings(lines)
 	return fmt.Sprintf("[%s]\n%s\n", c.Kind, strings.Join(lines, "\n"))


Sorry, I should asked for this in #163, but can we use "" for v in line 40 above if v is an empty string? If we don't want to filter out blank values, we should at least quote them properly…

I do not see any evidence that fluentbit supports quoting config values like that.

You're correct, fluentbit simply splits the string on a space and then trims the key and the value. It also requires the value to be non-empty, so it's not possible to generate a valid config with a blank value.

confgenerator/config.go

igorpeshansky · 2021-08-25T21:27:17Z

confgenerator/logging.go

+
+// setLogNameComponents generates a series of components that rewrites the tag on log entries tagged `tag` to be `logName`.
+func setLogNameComponents(tag, logName string) []fluentbit.Component {
+	// TODO: Can we just set log_name_key in the output plugin and avoid this mess?


Right, but the log_name_key setting overrides it. I still think we ought to just use the default value for that key, and add a record field called logging.googleapis.com/logName…

confgenerator/logging.go

...nerator/testdata/valid/linux/logging-pipeline_multiple_pipelines/golden_fluent_bit_main.conf

confgenerator/testdata/valid/linux/logging-processor_order/golden_fluent_bit_main.conf

confgenerator/testdata/valid/linux/logging-processor_order/golden_fluent_bit_parser.conf

davidbtucker · 2021-08-25T21:50:27Z

...ata/valid/windows/logging-receiver_files_type_multiple_receivers/golden_fluent_bit_main.conf

    Buffer_Chunk_Size 512k
    Buffer_Max_Size   5M
-    DB                ${buffers_dir}/ops-agent-fluent-bit
+    DB                ${buffers_dir}/pipeline1_log_source_id1


Btw, does it matter that DB is changing? Will that break any users on upgrade?

It would be a problem if we change DB. But is it actually changing? I see a reordering of these blocks, but the DB directories are still called pipeline1_sample_logs, pipeline2_sample_logs, and ops-agent-fluent-bit.

igorpeshansky

LGTM

igorpeshansky · 2021-08-26T04:25:41Z

confgenerator/fluentbit/modular.go

 		lines = append(lines, fmt.Sprintf("    %-*s %s", maxLen, k, v))
 	}
 	sort.Strings(lines)
 	return fmt.Sprintf("[%s]\n%s\n", c.Kind, strings.Join(lines, "\n"))


You're correct, fluentbit simply splits the string on a space and then trims the key and the value. It also requires the value to be non-empty, so it's not possible to generate a valid config with a blank value.

…odular

igorpeshansky suggested changes Aug 24, 2021

View reviewed changes

quentinmit added 4 commits August 24, 2021 16:15

Code to generate modular inputs and processors from logging config

0ec7282

Generate output plugin

b769676

Proces internal log files

c185d04

Reduce diffs

bbb4269

quentinmit force-pushed the quentin-fluentbit-modular branch from 14f0fa2 to bbb4269 Compare August 24, 2021 20:16

quentinmit added 5 commits August 24, 2021 17:11

Sort fluentbit config by kind to reduce diff size

cbfed5d

Re-add support for legacy builtin processors

4c772af

Fix syslog default processor

a1a2b5d

Further reduce diffs

2faf4b5

Switch to modular configs and update golden files

fcad309

quentinmit requested a review from davidbtucker August 24, 2021 23:27

igorpeshansky suggested changes Aug 24, 2021

View reviewed changes

qingling128 reviewed Aug 25, 2021

View reviewed changes

confgenerator/confgenerator.go Outdated Show resolved Hide resolved

...ta/valid/linux/all-backward_compatible_with_explicit_exporters/golden_fluent_bit_parser.conf Show resolved Hide resolved

qingling128 reviewed Aug 25, 2021

View reviewed changes

confgenerator/confgenerator.go Outdated Show resolved Hide resolved

confgenerator/testdata/valid/linux/logging-processor_order/golden_fluent_bit_parser.conf Show resolved Hide resolved

igorpeshansky suggested changes Aug 25, 2021

View reviewed changes

quentinmit added 8 commits August 25, 2021 01:12

Stop gathering the (nonexistent) collectd logs

d4d29a4

Make the processor_order test more reliable

bc65c1c

Remove unused code in confgenerator.go

e8b2282

Rename GenerateFluentBitConfigsModular

ebc65cd

Remove last vestiges of exporter types

346a602

Remove no-longer-used templating code and corresponding tests

6717a40

Consolidate WindowsEventLog receiver

91b2d23

Consolidate LoggingReceiverSyslog

847eda3

igorpeshansky reviewed Aug 25, 2021

View reviewed changes

confgenerator/confgenerator.go Show resolved Hide resolved

quentinmit added 4 commits August 25, 2021 02:28

Conssolidate LoggingReceiverFiles

e4478d2

Eliminate Stackdriver struct

98cd7ff

Eliminate inputs.go

0cb5ad4

Split logging helpers into a separate file

5f28a0e

quentinmit added 2 commits August 25, 2021 02:48

Document exported functions

872e7cb

Remove even more unused code

e6bc64f

quentinmit marked this pull request as ready for review August 25, 2021 07:03

davidbtucker reviewed Aug 25, 2021

View reviewed changes

confgenerator/confgenerator.go Outdated Show resolved Hide resolved

Review comments

6a18307

qingling128 approved these changes Aug 25, 2021

View reviewed changes

davidbtucker reviewed Aug 25, 2021

View reviewed changes

igorpeshansky suggested changes Aug 25, 2021

View reviewed changes

davidbtucker approved these changes Aug 25, 2021

View reviewed changes

quentinmit added 2 commits August 25, 2021 19:30

Repeat extra redundancy.

7f35bdd

Rearrange processors

f418847

igorpeshansky approved these changes Aug 26, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into quentin-fluentbit-m…

102e3e1

…odular

quentinmit merged commit 6c39ea5 into master Aug 26, 2021

quentinmit deleted the quentin-fluentbit-modular branch August 26, 2021 16:12

Replace Fluentbit config generation with modular implementation #164

Replace Fluentbit config generation with modular implementation #164

Uh oh!

Conversation

quentinmit commented Aug 24, 2021

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quentinmit commented Aug 24, 2021

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

quentinmit commented Aug 25, 2021

Uh oh!

igorpeshansky commented Aug 25, 2021

Uh oh!

davidbtucker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

quentinmit commented Aug 25, 2021

Uh oh!

qingling128 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment