Split conf to built-in conf and user conf and merge them. #111

qingling128 · 2021-06-23T23:39:54Z

Implements b/191888008.

qingling128 · 2021-06-23T23:41:27Z

Still fixing some tests on the Windows side, but the rough idea is ready for review.

punya · 2021-06-23T23:43:47Z

What's the user experience like when there's a syntax error (malformed YAML or something that doesn't pass validation)? Does it point to the specific file that has the error?

(Asking because we were discussing this very thing in OTel SIG today.)

confgenerator/confmerger.go

qingling128 · 2021-06-23T23:58:10Z

What's the user experience like when there's a syntax error (malformed YAML or something that doesn't pass validation)? Does it point to the specific file that has the error?

(Asking because we were discussing this very thing in OTel SIG today.)

Currently it writes out the built-in config and the merged config to a folder /etc/google-cloud-ops-agent/debugging (Detailed naming is TBD). The agent does not load configs from these two files. It's purely an output from the agent for debugging purpose. The errors can happen in 2 phases:

When the user config gets merged with the built-in config. If things fails here, users get an error complaining the merge did not succeeded with the error (e.g. not a valid yaml; trying to merge a struct that does not match the original type).
When the merged config is not valid, that is handled by the ops agent config validation logic. This validation will emit error messages that calls out which receiver/processor/exporter and which parameters are problematic: e.g. https://github.com/GoogleCloudPlatform/ops-agent/blob/master/confgenerator/testdata/invalid/linux/logging-receiver_files_type_unsupported_parameter_listen_host/golden_error. Users can trace back to the config by receiver/processor/exporter ID.

When in the future we need to support more config files (right now user config is all in one file), we'll need to add additional logic to map the receiver/processor/exporter ID to a certain file, and include that file path in the error message.

cmd/ops_agent_windows/run_windows.go

cmd/google_cloud_ops_agent_engine/main.go

confgenerator/confgenerator_test.go

confgenerator/testdata/valid/linux/empty_log_service/input.yaml

confgenerator/confmerger.go

confgenerator/testdata/valid/linux/empty_log_service/input.yaml

qingling128

This is ready for review now.

There are a few TODOs as seen in the code that I'm still cleaning up on the side, while this PR is being reviewed. But it's not a GA blocker, so if the PR is ready to ship before I get there, I will send a followup PR for them instead.

cmd/ops_agent_windows/run_windows.go

confgenerator/confgenerator.go

confgenerator/confgenerator_test.go

confgenerator/config.go

confgenerator/confmerger.go

qingling128 · 2021-06-29T15:38:56Z

All unit and integration tests passed for this PR at commite1162fd563a2de1508e811050a5aed6436b2045b except a known / unrelated issue on CentOS 8: b/191888008#comment3

cmd/google_cloud_ops_agent_engine/main.go

cmd/ops_agent_windows/run_windows.go

confgenerator/confgenerator.go

confgenerator/testdata/valid/linux/logging-multiple_google_exporters/input.yaml

confgenerator/testdata/valid/linux/logging-no_conf/input.yaml

confgenerator/testdata/valid/linux/metrics-no_conf/input.yaml

confgenerator/testdata/valid/linux/metrics-custom_collection_interval/input.yaml

confgenerator/windows-default-config.yaml

quentinmit · 2021-06-29T21:33:13Z

confgenerator/confmerger.go

How are these not the same case?

The code to handle them are exactly the same. Just wanna document the subtle choices we made:

Difference between an empty list and nil

For a non-empty list, we only overrides the whole thing instead of trying to append to the existing list.

I don't understand the "difference". Appending to an empty list is the same as replacing it.

The nil check is only for the overrides list, not for the original list now: https://github.com/GoogleCloudPlatform/ops-agent/blob/1f661798d2c136c156ec3fd45c429269fc00cfc5/confgenerator/confmerger.go#L196

quentinmit · 2021-06-29T21:33:53Z

confgenerator/confmerger.go

Why doesn't merging this at the pipeline level just like the other object types produce exactly the same behavior in fewer lines of code?

Because of cases like:

$ cat testdata/valid/windows/metrics-turn_off_iis/input.yaml metrics: service: pipelines: default_pipeline: receivers: [hostmetrics,mssql]

I don't see how that is different. That looks like it would merge the same either way.

If it's at the pipeline level, users would have to specify the full default_pipeline (receivers, processors, exporters), right?

Sample case: https://github.com/GoogleCloudPlatform/ops-agent/blob/1f661798d2c136c156ec3fd45c429269fc00cfc5/confgenerator/testdata/valid/linux/metrics-exclude_metrics_by_prefixes/input.yaml

metrics: processors: metrics_filter: type: exclude_metrics metrics_pattern: - agent.googleapis.com/processes/* - agent.googleapis.com/cpu/* service: pipelines: default_pipeline: processors: [metrics_filter]

The alternative is for users to always repeat receivers:

metrics: processors: metrics_filter: type: exclude_metrics metrics_pattern: - agent.googleapis.com/processes/* - agent.googleapis.com/cpu/* service: pipelines: default_pipeline: receivers: [hostmetrics] processors: [metrics_filter]

confgenerator/config.go

cmd/ops_agent_windows/run_windows.go

confgenerator/confgenerator_test.go

cmd/google_cloud_ops_agent_engine/main.go

cmd/ops_agent_windows/run_windows.go

igorpeshansky · 2021-06-30T18:18:27Z

confgenerator/built-in-config-linux.yaml

We did discuss moving these into testdata and making these files symlinks to the testdata ones, to remove the special case for update_golden.

* Move string to struct. * Clean up mergeConfigs

* Add a test for overrides default pipeline's processors * Add a test for deleted config file corner case.

* Rename no_config to all-user_config_file_deleted and remove special logic * Make built-in-conf.yaml and merged-conf.yaml file locations non configurable. * Remove defaultConfig param from platformConfig and combine default-config.yaml and windows-default-config.yaml * Rename WriteConfigFile to writeConfigFile

igorpeshansky

LGTM

igorpeshansky · 2021-07-02T19:42:49Z

confgenerator/files.go

 	if err != nil {
 		return err
 	}
-	uc, err := ParseUnifiedConfig(data, hostInfo.OS)


Note: this change disabled config validation on Linux. Fixed in #133.

qingling128 requested a review from davidbtucker June 23, 2021 23:41

punya reviewed Jun 23, 2021

View reviewed changes

confgenerator/confmerger.go Outdated Show resolved Hide resolved

confgenerator/confmerger.go Outdated Show resolved Hide resolved

qingling128 requested a review from igorpeshansky June 24, 2021 00:00

qingling128 marked this pull request as draft June 24, 2021 00:02

davidbtucker reviewed Jun 24, 2021

View reviewed changes

davidbtucker approved these changes Jun 24, 2021

View reviewed changes

quentinmit requested changes Jun 24, 2021

View reviewed changes

confgenerator/confmerger.go Outdated Show resolved Hide resolved

confgenerator/confmerger.go Outdated Show resolved Hide resolved

confgenerator/testdata/valid/linux/empty_log_service/input.yaml Show resolved Hide resolved

qingling128 force-pushed the lingshi-viper branch from 79e1f30 to 041ff67 Compare June 28, 2021 15:23

qingling128 changed the title ~~Split conf to built-in conf and user conf and use Viper to merge them.~~ Split conf to built-in conf and user conf and merge them. Jun 28, 2021

qingling128 force-pushed the lingshi-viper branch 2 times, most recently from 79864d6 to 106334a Compare June 28, 2021 22:07

qingling128 marked this pull request as ready for review June 28, 2021 22:08

qingling128 commented Jun 28, 2021

View reviewed changes

qingling128 force-pushed the lingshi-viper branch from fcf994f to e1162fd Compare June 29, 2021 06:28

igorpeshansky suggested changes Jun 29, 2021

View reviewed changes

qingling128 force-pushed the lingshi-viper branch from 733a5ca to de3d0c3 Compare June 29, 2021 21:37

quentinmit requested changes Jun 29, 2021

View reviewed changes

qingling128 force-pushed the lingshi-viper branch 8 times, most recently from b61fecd to 1f66179 Compare June 30, 2021 06:38

igorpeshansky suggested changes Jun 30, 2021

View reviewed changes

qingling128 added 5 commits June 30, 2021 20:41

Split conf to built-in conf and user conf and merge them.

2e5b25b

Code review feedback - round 1

9741a8e

* Move string to struct. * Clean up mergeConfigs

Add more tests

383ca5d

* Add a test for overrides default pipeline's processors * Add a test for deleted config file corner case.

minor feedback

428aa74

qingling128 force-pushed the lingshi-viper branch from f331a48 to 428aa74 Compare June 30, 2021 20:48

igorpeshansky approved these changes Jun 30, 2021

View reviewed changes

quentinmit approved these changes Jun 30, 2021

View reviewed changes

qingling128 merged commit e3a5f3a into master Jun 30, 2021

qingling128 deleted the lingshi-viper branch June 30, 2021 20:53

igorpeshansky mentioned this pull request Jun 30, 2021

Add empty metrics_filter by default #127

Merged

igorpeshansky reviewed Jul 2, 2021

View reviewed changes

igorpeshansky mentioned this pull request Jul 2, 2021

Explicitly validate config as part of the main service startup. #133

Merged

qingling128 mentioned this pull request Jul 21, 2021

Rename valid test folders to be consistent with naming convention of invalid test folders. #135

Merged

Split conf to built-in conf and user conf and merge them. #111

Split conf to built-in conf and user conf and merge them. #111

Uh oh!

Conversation

qingling128 commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qingling128 commented Jun 23, 2021

Uh oh!

punya commented Jun 23, 2021

Uh oh!

Uh oh!

Uh oh!

qingling128 commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

qingling128 commented Jun 29, 2021

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!

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!

qingling128 Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

qingling128 commented Jun 23, 2021 •

edited

Loading

qingling128 commented Jun 23, 2021 •

edited

Loading

qingling128 Jun 30, 2021 •

edited

Loading