Clean up configuration generation for otel configs. #145

igorpeshansky · 2021-08-06T22:53:21Z

Extracted from #141.

…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.

quentinmit · 2021-08-13T22:35:38Z

I rebased this on master now that #143 is merged. Still quite a bit of cleanup I'd like to do, but I guess we could split that into a separate PR if you'd like to review this so far.

quentinmit · 2021-08-16T06:17:07Z

I finished the work from Friday to make otel config generation completely modular. The diff is now 12 files changed, 874 insertions(+), 1820 deletions(-), so a net decrease of almost 1,000 LOC.

Please review, @davidbtucker

davidbtucker

First round of comments.

davidbtucker · 2021-08-17T14:18:57Z

confgenerator/agentmetrics.go

+	Version string
+}
+
+func (r MetricsReceiverAgent) Pipeline() otel.Pipeline {


Possibly *otel.Pipeline.

No, I don't think there's any reason to pass by reference (and if we do, that brings up the possibility of accidentally changing the struct)

confgenerator/agentmetrics.go

confgenerator/metric_processors.go

confgenerator/agentmetrics.go

confgenerator/otel/modular.go

confgenerator/confgenerator.go

confgenerator/metric_processors.go

confgenerator/otel/modular.go

quentinmit · 2021-08-17T21:31:19Z

I just pushed a couple commits to make the golden file diffs smaller. Here's a command you can use to diff them:

pip install yq
sudo apt install jq
for i in testdata/valid/*/*/golden_otel.conf; do echo "--- $i"; diff -U500 <(git cat-file blob origin/master:./$i | sed 's/metric_name:/include:/' | ~/.local/bin/yq -y -S | sed 's/null/{}/') <(git cat-file blob HEAD:./$i | ~/.local/bin/yq -y -S); done

…of metric_name

igorpeshansky · 2021-08-17T22:19:57Z

confgenerator/config.go

 type Metrics struct {
 	Receivers  metricsReceiverMap  `yaml:"receivers" validate:"dive,keys,startsnotwith=lib:"`
 	Processors metricsProcessorMap `yaml:"processors" validate:"dive,keys,startsnotwith=lib:"`
 	// Exporters are deprecated and ignored, so do not have any validation.
-	Exporters metricsExporterMap `yaml:"exporters,omitempty"`
-	Service   *MetricsService    `yaml:"service"`
+	Exporters map[string]interface{} `yaml:"exporters,omitempty"`


We will now allow arbitrary YAML in the exporters field, and it's completely unvalidated. Is this where we want to be with respect to customer configs? Or do we want to restrict what they put in there before we ignore it?

We already dropped validation on this field, both partially before and now even more. It's a lot of work to maintain validation on a field that's already ignored and deprecated. Anyone who is specifying this field has been doing it since pre-2.0, so they already had a chance to get the field validated. I don't think it's worth the effort to maintain unused validation.

igorpeshansky

LGTM

qingling128

LGTM. Integration tests passed: http://b/195762251#comment4

qingling128 · 2021-08-17T23:32:10Z

confgenerator/confgenerator.go

-					stackdriverList = append(stackdriverList, &stackdriver)
-					exportNameMap[eID] = "googlecloud/" + eID
+			for i, receiverPipeline := range receiver.Pipelines() {
+				prefix := fmt.Sprintf("%s_%s", strings.ReplaceAll(pID, "_", "__"), strings.ReplaceAll(rID, "_", "__"))


Just curious: what is the reason behind replacing _ with __? The resulting prefix looks like:

agentmetrics/default__pipeline_hostmetrics_0

If the purpose is to make them look grouped, shouldn't it be

agentmetrics/default_pipeline__hostmetrics__0

Replacing _ with __ prevents collisions between different pipelines. If we went the other direction, there would be no way to distinguish between (pipeline default__pipeline receiver hostmetrics) and (pipeline default receiver pipeline__hostmetrics).

We could certainly introduce smarter anti-collision logic though (e.g. default to the bare pipeline name and add suffixes only when there would be a collision). But I'd like to save that for later PRs.

qingling128 · 2021-08-18T00:02:42Z

confgenerator/otel/modular.go

@@ -44,7 +44,7 @@ func (c Component) name(suffix string) string {
 	return c.Type
 }

-// configToYaml converts a tree of structss into a YAML file.
+// configToYaml converts a tree of structs into a YAML file.


My MBP keyboard introduces spurious s keypresses all over the place :(

igorpeshansky requested review from quentinmit, qingling128 and davidbtucker August 6, 2021 22:53

This was referenced Aug 6, 2021

Clean up configuration generation for fluentbit configs. #146

Merged

WIP: configuration cleanup. #141

Closed

quentinmit force-pushed the igorpeshansky-config-custom-unmarshaler branch from d23609a to 12bf723 Compare August 11, 2021 18:17

igorpeshansky and others added 11 commits August 13, 2021 18:23

Inline extract.*Factories functions.

1f3f3de

Eliminate otel receiver factories.

3973c51

Eliminate otel processor factories.

b49616e

Eliminate fluentbit receiver factories.

3c8de31

Don't mangle otel entity ids.

30ba537

Cosmetic: reorder otel structs for consistency.

bf11ebe

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

6d18fce

…ver interface.

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

c2f1c87

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

223d05d

…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.

61de2c6

Generate receivers in order

903c862

quentinmit force-pushed the igorpeshansky-config-cleanup-generation-otel branch from d2ffcfd to 903c862 Compare August 13, 2021 22:32

quentinmit changed the base branch from igorpeshansky-config-custom-unmarshaler to master August 13, 2021 22:33

quentinmit added 7 commits August 13, 2021 22:44

Generate OTel YAML from Go code instead of text templates

8a89807

Add agent metrics unconditionally

5d6025f

Convert hostmetrics config

1af3e55

Fix filter processor config generation

05cb83f

Update golden files

7b63495

Add resourcedetection processor

ee7dbd2

Remove now-unused code from the confgenerator package

30e9de8

quentinmit force-pushed the igorpeshansky-config-cleanup-generation-otel branch from c910356 to b24888f Compare August 16, 2021 20:29

Fix config from testing against real OT

1880902

quentinmit force-pushed the igorpeshansky-config-cleanup-generation-otel branch from b24888f to 1880902 Compare August 16, 2021 20:43

Fix copy+paste error

0383858

davidbtucker reviewed Aug 17, 2021

View reviewed changes

quentinmit added 3 commits August 17, 2021 15:01

Review comments

4ead00c

Remove unused Modular{Receiver,Processor} types

7a940a9

More review comments

67aec08

qingling128 reviewed Aug 17, 2021

View reviewed changes

confgenerator/metric_processors.go Outdated Show resolved Hide resolved

qingling128 reviewed Aug 17, 2021

View reviewed changes

confgenerator/otel/modular.go Show resolved Hide resolved

quentinmit added 2 commits August 17, 2021 17:13

Move OT processor config helpers into the otel package

3658356

Add comment to configToYaml

9d963e1

quentinmit force-pushed the igorpeshansky-config-cleanup-generation-otel branch from aac139d to 9d963e1 Compare August 17, 2021 21:14

quentinmit added 2 commits August 17, 2021 17:24

Reduce diff against old OT golden configs

33d3016

Update YAML library to include float fix

3bc51f3

Fix MSSQL metric names, and use non-deprecated include field instead …

1f8e8b8

…of metric_name

igorpeshansky commented Aug 17, 2021

View reviewed changes

Apple keyboards...

0c30a7f

igorpeshansky commented Aug 17, 2021

View reviewed changes

igorpeshansky requested review from qingling128 and removed request for quentinmit August 17, 2021 22:34

qingling128 approved these changes Aug 18, 2021

View reviewed changes

quentinmit merged commit 8d6ed85 into master Aug 18, 2021

quentinmit deleted the igorpeshansky-config-cleanup-generation-otel branch August 18, 2021 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up configuration generation for otel configs. #145

Clean up configuration generation for otel configs. #145

igorpeshansky commented Aug 6, 2021 •

edited

Loading

quentinmit commented Aug 13, 2021

quentinmit commented Aug 16, 2021

davidbtucker left a comment

davidbtucker Aug 17, 2021

quentinmit Aug 17, 2021

quentinmit commented Aug 17, 2021 •

edited

Loading

igorpeshansky Aug 17, 2021

quentinmit Aug 17, 2021

igorpeshansky left a comment

qingling128 left a comment

qingling128 Aug 17, 2021

quentinmit Aug 18, 2021

qingling128 Aug 18, 2021

quentinmit Aug 18, 2021

Clean up configuration generation for otel configs. #145

Clean up configuration generation for otel configs. #145

Conversation

igorpeshansky commented Aug 6, 2021 • edited Loading

quentinmit commented Aug 13, 2021

quentinmit commented Aug 16, 2021

davidbtucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quentinmit commented Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

qingling128 left a 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

quentinmit commented Aug 17, 2021 •

edited

Loading