Support for validate subcommand for otel configuration #4023

michalpristas · 2024-01-05T13:37:00Z

performs validation of config passed to the command using flags supported also in otel subcommand (--config and --set)

…idate

elasticmachine · 2024-01-05T13:37:03Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

elasticmachine · 2024-01-05T13:37:03Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

michalpristas · 2024-01-08T07:50:25Z

UT for logic not for boilerplate results in low soundqube rating

ycombinator · 2024-01-10T13:39:03Z

With this PR, we are adding a top-level sub-command to the Agent, so users will execute elastic-agent validate. But this command only supports OTel config validation, which could be confusing.

I really think we need to implement #3903 first and move this validate sub-command under that otel sub-command for clarity, so users will execute elastic-agent otel validate. If, in the future, we support validation for OTel and native Agent configuration both, we can promote the validate sub-command back to the top-level.

ycombinator · 2024-01-10T13:39:33Z

internal/pkg/agent/cmd/testdata/otel/elastic-agent.yml

@@ -0,0 +1,301 @@
+###################### Agent Configuration Example #########################


WDYT about removing all the comments from this file, so this file also becomes more like the otel.yml file? Just thinking that the comments are not adding much value to test fixture files.

ycombinator · 2024-01-10T13:43:58Z

With this PR, the syntax for running OTel configuration validation becomes elastic-agent validate. However, the syntax for validating OTel configuration with the vanilla OTel collector is otelcol validate --config /path/to/first.yml --config /path/to/first.yml ....

To ease migration, I think we'd want Agent (with embedded OTel Collector) to be as much of a drop-in replacement for the vanilla OTel collector. So I think we need to support the same syntax, that is, something like elastic-agent validate --config /path/to/first.yml --config /path/to/first.yml ..., or better yet, elastic-agent otel validate --config /path/to/first.yml --config /path/to/first.yml ....

michalpristas · 2024-01-10T14:01:26Z

once i introduce otel subcommand i will move this under otel subcommand and add multiple files and other flags. for now i prefer to stay with flags we have and not support multiple files using --config as normal agent does not understand that.

the order in my mind is
#4047 to go in
introduce otel
bring this even with normal collector CLI

internal/pkg/agent/cmd/validate.go

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

ycombinator · 2024-01-10T14:09:43Z

I built Agent with this PR, extracted it into a folder, and ran ./elastic-agent validate. I got this error:

"/private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/elastic-agent.yml" is not an otel config. file should be named 'otel.yml', 'otlp.yml' or 'otelcol.yml'

There is no /private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/elastic-agent.yml file mentioned in the error message:

$ ls -l /private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/elastic-agent.yml
ls: /private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/elastic-agent.yml: No such file or directory

And there is actually an otel.yml file in that folder:

$ ls -l /private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/otel.yml
-rw-------  1 shaunak  wheel  425 Jan 10 05:29 /private/tmp/elastic-agent-8.13.0-SNAPSHOT-darwin-aarch64/otel.yml

michalpristas · 2024-01-10T14:30:10Z

elastic-agent.yml is a default config value if not provided by --config flag. may be it

…t into otel/validate

michalpristas · 2024-01-11T09:27:07Z

fixed

…idate

…t into otel/validate

ycombinator · 2024-01-11T16:35:57Z

internal/pkg/agent/cmd/validate.go

+	if runAsOtel := otel.IsOtelConfig(ctx, cfgFile); !runAsOtel {
+		return fmt.Errorf("%q is not an otel config. file should be named 'otel.yml', 'otlp.yml' or 'otelcol.yml'", cfgFile)
+	}


After this PR is merged but before we change elastic-agent validate to elastic-agent otel validate, the user experience would be that elastic-agent validate only works for OTel configurations. To me, this is a bit awkward as validate is a top-level sub-command and, therefore, suggests that it can validate both OTel and Agent configurations.

So my suggestion for this PR would be to return early from this function with nil if otel.IsOtelConfig(ctx, cfgFile) returns false and also make DefaultConfigName the default value for the -c/--config global CLI options.

Once we have the follow up PR to move elastic-agent validate to elastic-agent otel validate, it would make sense to error out if otel.IsOtelConfig(ctx, cfgFile) returns false.

internal/pkg/agent/cmd/validate.go

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

blakerouse · 2024-01-16T14:54:21Z

It would be really nice to see this do a simple validation of the elastic-agent.yml as well. Providing a nice clean path to validate both. Just an idea, not a block or anything.

pierrehilbert · 2024-02-01T17:38:04Z

@michalpristas should we put this back on draft for now?

michalpristas · 2024-02-02T09:37:24Z

@pierrehilbert after otel subcommand went in, this is up to date and ready for review

ycombinator · 2024-02-02T12:19:18Z

If I run ./elastic-agent otel validate (without any --config options), the command succeeds with a 0 exit code.

As comparison, if I run the OTel Collector binary's validate sub-command without any --config options, that command fails like so:

$ ./bin/otelcorecol_darwin_arm64 validate
Error: at least one config flag must be provided
2024/02/02 04:18:29 collector server run finished with error: at least one config flag must be provided

Would it be possible to mimic the OTel Collector binary's behavior in ./elastic-agent otel validate? Just trying to make this command a drop-in replacement for the native OTel Collector binary as much as possible.

ycombinator

Apart from my request/suggestion in #4023 (comment), which I would say is a nice to have, this PR LGTM.

cmacknz · 2024-02-02T15:06:59Z

testing/integration/otel_test.go

@@ -133,6 +133,11 @@ func TestOtelFileProcessing(t *testing.T) {
 		`"stringValue":"system.log"`, // system.log is being processed
 	})

+	// check `elastic-agent otel validate` command works for otel config
+	out, err := fixture.Exec(ctx, []string{"otel", "validate", "--config", cfgFilePath})


I would like to see a test (here or elsewhere) that the validate command actually detects errors in otel configuration files.

That is the real value of the validate command, lets make sure it works. This test would pass even if we never called the otel validate function and just parsed the command line arguments.

this is now addressed at e432f98

i pulled it outside into separate function for readability but kept within same test to spare some resources

…idate

elastic-sonarqube · 2024-02-02T18:21:36Z

Quality Gate failed

Failed conditions

9.1% 9.1% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

cmacknz · 2024-02-02T21:53:48Z

LGTM, fine to force merge this. Will wait until Michal is back online before doing so.

michalpristas added 3 commits January 4, 2024 16:58

validate subcommand working

d71eca4

Validate subcommand for otel mode

ff7711d

Merge branch 'main' of github.com:elastic/elastic-agent into otel/val…

617d4ff

…idate

michalpristas added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team backport-skip skip-changelog labels Jan 5, 2024

michalpristas self-assigned this Jan 5, 2024

michalpristas requested a review from a team as a code owner January 5, 2024 13:37

michalpristas requested review from blakerouse and faec January 5, 2024 13:37

michalpristas added 3 commits January 5, 2024 15:05

fmt

4f82c45

Merge branch 'main' into otel/validate

db80c84

Merge branch 'main' into otel/validate

b9071e4

michalpristas added 2 commits January 9, 2024 09:54

Merge branch 'main' into otel/validate

be10a47

Merge branch 'main' into otel/validate

377bbdd

michalpristas requested a review from ycombinator January 10, 2024 08:22

ycombinator reviewed Jan 10, 2024

View reviewed changes

internal/pkg/agent/cmd/validate.go Outdated Show resolved Hide resolved

Update internal/pkg/agent/cmd/validate.go

cf30bf3

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

otel default config

b8a6688

Merge branch 'otel/validate' of github.com:michalpristas/elastic-agen…

93565f8

…t into otel/validate

michalpristas added 4 commits January 11, 2024 11:57

Merge branch 'main' into otel/validate

e15aa21

cleaner config

e730d01

Merge branch 'main' of github.com:elastic/elastic-agent into otel/val…

931a4e4

…idate

Merge branch 'otel/validate' of github.com:michalpristas/elastic-agen…

5ef4b4d

…t into otel/validate

ycombinator reviewed Jan 11, 2024

View reviewed changes

internal/pkg/agent/cmd/validate.go Outdated Show resolved Hide resolved

Update internal/pkg/agent/cmd/validate.go

f98a7d7

Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>

michalpristas added 3 commits February 2, 2024 09:28

resolved conflicts

7ddde48

Moved validate under otel subcommand

9518c33

conflicts

a222d53

michalpristas requested a review from ycombinator February 2, 2024 09:37

revert common defaults to match main

de3e05b

ycombinator approved these changes Feb 2, 2024

View reviewed changes

validate expects at least one config

db69fde

cmacknz reviewed Feb 2, 2024

View reviewed changes

michalpristas added 3 commits February 2, 2024 17:25

validate integration test checking invalid config

e432f98

Merge branch 'main' of github.com:elastic/elastic-agent into otel/val…

a163e65

…idate

add wait for otel to TestOtelAPMIngestion:

6115934

michalpristas requested a review from cmacknz February 2, 2024 20:57

cmacknz approved these changes Feb 2, 2024

View reviewed changes

cmacknz merged commit 6f7a116 into elastic:main Feb 5, 2024
8 of 9 checks passed

michalpristas mentioned this pull request Feb 6, 2024

[OTel Collector] Support for validate subcommand #3871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for validate subcommand for otel configuration #4023

Support for validate subcommand for otel configuration #4023

michalpristas commented Jan 5, 2024 •

edited

Loading

elasticmachine commented Jan 5, 2024

elasticmachine commented Jan 5, 2024

michalpristas commented Jan 8, 2024

ycombinator commented Jan 10, 2024

ycombinator Jan 10, 2024

ycombinator commented Jan 10, 2024

michalpristas commented Jan 10, 2024

ycombinator commented Jan 10, 2024

michalpristas commented Jan 10, 2024

michalpristas commented Jan 11, 2024

ycombinator Jan 11, 2024 •

edited

Loading

blakerouse commented Jan 16, 2024

pierrehilbert commented Feb 1, 2024

michalpristas commented Feb 2, 2024 •

edited

Loading

ycombinator commented Feb 2, 2024 •

edited

Loading

ycombinator left a comment

cmacknz Feb 2, 2024

michalpristas Feb 2, 2024

michalpristas Feb 2, 2024 •

edited

Loading

elastic-sonarqube bot commented Feb 2, 2024

cmacknz commented Feb 2, 2024

		@@ -0,0 +1,301 @@
		###################### Agent Configuration Example #########################

Support for validate subcommand for otel configuration #4023

Support for validate subcommand for otel configuration #4023

Conversation

michalpristas commented Jan 5, 2024 • edited Loading

elasticmachine commented Jan 5, 2024

elasticmachine commented Jan 5, 2024

michalpristas commented Jan 8, 2024

ycombinator commented Jan 10, 2024

ycombinator Jan 10, 2024

Choose a reason for hiding this comment

ycombinator commented Jan 10, 2024

michalpristas commented Jan 10, 2024

ycombinator commented Jan 10, 2024

michalpristas commented Jan 10, 2024

michalpristas commented Jan 11, 2024

ycombinator Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

blakerouse commented Jan 16, 2024

pierrehilbert commented Feb 1, 2024

michalpristas commented Feb 2, 2024 • edited Loading

ycombinator commented Feb 2, 2024 • edited Loading

ycombinator left a comment

Choose a reason for hiding this comment

cmacknz Feb 2, 2024

Choose a reason for hiding this comment

michalpristas Feb 2, 2024

Choose a reason for hiding this comment

michalpristas Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

elastic-sonarqube bot commented Feb 2, 2024

Quality Gate failed

Failed conditions

cmacknz commented Feb 2, 2024

michalpristas commented Jan 5, 2024 •

edited

Loading

ycombinator Jan 11, 2024 •

edited

Loading

michalpristas commented Feb 2, 2024 •

edited

Loading

ycombinator commented Feb 2, 2024 •

edited

Loading

michalpristas Feb 2, 2024 •

edited

Loading