Cleanup configuration representation. #143

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

Extracted from #141 and rebased on top of the merged #142. Can split up further if needed.

…arameters.

Missing functionality: - entity id is not known at YAML unmarshal time, so replaced with question marks - platform is not known at YAML unmarshal time, so all registered types are considered valid

…ime.

…ug fix

davidbtucker

Some suggested comments to add. More to come.

confgenerator/config.go

davidbtucker

Some more config.go comments.

confgenerator/config.go

qingling128 · 2021-08-11T22:48:57Z

confgenerator/confgenerator_test.go

 			if err != nil {
 				t.Fatalf("ReadFile(%q) got: %v", userSpecifiedConfPath, err)
 			}
+			t.Logf("merged config:\n%s", data)


[Optional] Can we log the built-in config and user specified config as well? This helps with diagnosing old agent issues when the config might have been edited already, and customer might want to trace back to the config content at the time of incident.

This is a unit test... there's no old customer config in a unit test :P It's not easy to add logging of the pre-merge configs in the unit test for now. Hopefully will be possible when I do more refactoring.

As for logging these in the real daemon, yeah, we should do that but that will come later.

qingling128 · 2021-08-12T00:10:42Z

confgenerator/config.go

+			}
+		}
+		sort.Strings(supportedTypes)
+		return fmt.Errorf(`%s %s %q with type %q is not supported. Supported %s %s types: [%s].`,


We probably need to remove the "???" part for now if receiver / processor / exporter ID is not available at this point.

These error messages are all going to get cleaned up later in a separate PR. But I removed the ??? for now so it's not as confusing.

qingling128 · 2021-08-12T00:23:43Z

.../linux/logging-receiver_syslog_type_invalid_parameter_listen_port_is_not_an_int/golden_error

@@ -1,2 +1 @@
-the agent config file is not valid YAML. detailed error: yaml: unmarshal errors:
-  line 6: cannot unmarshal !!str `abc` into uint16
+the agent config file is not valid. detailed error: cannot unmarshal string into Go struct field UnifiedConfig.Logging of type uint16


This one looks a bit different from others as it does not print the config snippet. Is that expected?

No, I don't know why it's missing the line number and snippet. Nor why it says unmarshal string into [...] uint16. Worth investigating later.

qingling128 · 2021-08-12T00:25:16Z

confgenerator/testdata/invalid/linux/logging-processor_reserved_id_prefix/golden_error

@@ -1 +1,9 @@
-logging processor id "lib:processor_1" is not allowed because prefix 'lib:' is reserved for pre-defined processors.
+the agent config file is not valid. detailed error: [9:20] Key: 'Logging.processors[lib:processor_1]' Error:Field validation for 'processors[lib:processor_1]' failed on the 'startsnotwith' tag


The new message does not mention what prefix is forbidden. Not too bad though as we could add a note to the troubleshooting guide.

Improving all of these error messages is a TODO. We can easily add that back and more, but I'm prioritizing getting a stable interface ready for BM first.

davidbtucker

Finished reviewing the remaining files.

confgenerator/config.go

confgenerator/confgenerator.go

confgenerator/testdata/invalid/linux/logging-pipeline_reserved_id_prefix/golden_error

davidbtucker · 2021-08-12T02:18:29Z

...alid/linux/logging-receiver_files_type_unsupported_parameter_transport_protocol/golden_error

@@ -1 +1,9 @@
-parameter "transport_protocol" in "files" type logging receiver "receiver_1" is not supported. Supported parameters: [include_paths, exclude_paths].
+the agent config file is not valid. detailed error: [8:7] unknown field "transport_protocol"


We can remove some of these validation tests as redundant. Follow-up work.

Which one(s) are redundant?

A bunch of these tests have the form "invalid field X for type Y", because we had to explicitly check each field in the old code.

Now with the validate tags, I think we can just have a single such test.

...valid/linux/metrics-processor_exclude_metrics_invalid_parameter_metric_prefixes/golden_error

igorpeshansky requested review from davidbtucker, qingling128 and quentinmit August 6, 2021 19:17

igorpeshansky mentioned this pull request Aug 6, 2021

WIP: configuration cleanup. #141

Closed

igorpeshansky force-pushed the igorpeshansky-config-custom-unmarshaler branch from 84471f1 to 0472e6f Compare August 6, 2021 22:45

igorpeshansky changed the title ~~Cleanup configuration representation and generation.~~ Cleanup configuration representation. Aug 6, 2021

igorpeshansky mentioned this pull request Aug 6, 2021

Clean up configuration generation for otel configs. #145

Merged

igorpeshansky and others added 21 commits August 11, 2021 14:16

Use a custom unmarshaler for the logging receiver types.

595857a

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

ea51419

Embed Type in LoggingReceiver.

f0516eb

Move custom parameter validation for logging receivers into ValidateP…

c68fea5

…arameters.

Use a custom unmarshaler for the logging processor types.

8f456da

Use a custom unmarshaler for the logging exporter types.

5a4d60d

Use a custom unmarshaler for the metrics receiver types.

c7b625e

Use a custom unmarshaler for the metrics processor types.

d5f9c12

Use a custom unmarshaler for the metrics exporter types.

7c1d8e7

Factor out common type registration code.

4b85790

Attempt to reconstruct previous validation error messages.

8791a65

Missing functionality: - entity id is not known at YAML unmarshal time, so replaced with question marks - platform is not known at YAML unmarshal time, so all registered types are considered valid

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

3464d76

…ime.

Split into per-application files

d7c9888

Switch to go-yaml in preparation for simpler validation

6f0d14d

Use validator with go-yaml

f36e6f2

Remove redundant parameter validation

0b50905

Remove per-receiver validation in favor of struct tags

b418361

Fix validation of slices

e48b8a5

Use the YAML field name in validation failures

9cd5fd3

Validate component IDs and platform

b47cb25

Fix platform validation

12bf723

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

Remove redundant call to UnmarshalContext now that upstream has the b…

ed40648

…ug fix

Tidy module files

9a8c4c8

davidbtucker reviewed Aug 11, 2021

View reviewed changes

qingling128 reviewed Aug 12, 2021

View reviewed changes

davidbtucker reviewed Aug 12, 2021

View reviewed changes

quentinmit added 8 commits August 12, 2021 17:54

Review comments 1/n

618dd32

Rename ComponentType field to Type and remove unnecessary accessor

8e926b5

Remove unused slice

9dbdd4c

Remove unused regexp

d1cd30d

Rename/add comments for clarity

f294bea

Review comments 2/n

bfa0649

Remove ??? from error message

7a3f21f

Review comments 3/n

f62fde3

davidbtucker approved these changes Aug 13, 2021

View reviewed changes

quentinmit merged commit 014c2da into master Aug 13, 2021

igorpeshansky deleted the igorpeshansky-config-custom-unmarshaler branch August 18, 2021 01:38

		@@ -1 +1,9 @@
		logging processor id "lib:processor_1" is not allowed because prefix 'lib:' is reserved for pre-defined processors.
		the agent config file is not valid. detailed error: [9:20] Key: 'Logging.processors[lib:processor_1]' Error:Field validation for 'processors[lib:processor_1]' failed on the 'startsnotwith' tag

		@@ -1 +1,9 @@
		parameter "transport_protocol" in "files" type logging receiver "receiver_1" is not supported. Supported parameters: [include_paths, exclude_paths].
		the agent config file is not valid. detailed error: [8:7] unknown field "transport_protocol"

Cleanup configuration representation. #143

Cleanup configuration representation. #143

Uh oh!

Conversation

igorpeshansky commented Aug 6, 2021

Uh oh!

davidbtucker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidbtucker 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!

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!

davidbtucker 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants