Add benchmark validate command for static validation #1718

flash1293 · 2024-03-08T16:12:14Z

Context: I'm mostly working on this to familiarize myself with the elastic-package project (and golang in the first place).

Related to #1610

This PR adds a validate command to benchmarks to statically validate the benchmark generator is configured properly:

In this example there is a missing { for the data_stream object in the template of one out of multiple templates:

> elastic-package benchmark validate
Run benchmark asset validation
Error: validation failed: [access-benchmark] failed to unmarshal json event: invalid character ':' after object key:value pair, generated output:
{
    "@timestamp": "2024-03-08T16:55:55.248+01:00",
    "agent": {
        "ephemeral_id": "mistycarver",
        "id": "ef5e274d-4b53-45e6-943a-a5bcf1a6f523",
        "name": "cottonhugger",
        "type": "filebeat",
        "version": "8.5.1"
    },
    "data_stream":
        "dataset": "nginx.access",
        "namespace": "ep",
        "type": "logs"
    },
...

It generates 10 events using each generator and checks whether the result is valid JSON.

This does not require a running elasticsearch instance, it's purely checking the benchmark configuration. This implementation validates the following things:

All config files are in their expected location
The template is valid
The generated output is valid JSON

Implementation details

As this is very close to the existing stream logic, I made it part of that module. I pondered to split the necessary bits out into a common and validation-specific bit, but it seemed overkill.

The existing setUp routine is talking to Elasticsearch already, so I split out the static parts into an initialize function which can be called for the validation before actually querying the generators.

@ruflin feel free to add the right people as reviewers

internal/benchrunner/runners/stream/runner.go

ruflin · 2024-03-11T14:21:23Z

This is a great first step for validation. Did you test what kind of error is returned if you made an error in the template itself that the template is invalid instead of the JSON?

…idation

flash1293 · 2024-03-13T12:05:35Z

Did you test what kind of error is returned if you made an error in the template itself that the template is invalid instead of the JSON?

@ruflin In this case the error looks like this:

Run benchmark asset validation
Error: validation failed: can't initialize generator: [access-benchmark]: template: generator:35: undefined variable "$remoteUser"

ruflin · 2024-03-13T15:18:55Z

@flash1293 Neat, that already gives a lot more details on what goes wrong and makes it easy to debug.

jsoriano

Thanks for adding this! This is going to be really useful when preparing benchmarks.

I wonder if we could add this as part of elastic-package test static, this is used now to validate sample documents, and the stream subcommand is kind-of an automation for the same thing. We could also validate fields there.

internal/benchrunner/runners/stream/runner.go

jsoriano · 2024-03-18T12:53:24Z

cmd/benchmark.go

+	validateCmd := getValidateCommand()
+	cmd.AddCommand(validateCmd)


Instead of adding this as a new subcommand, I think we could add this as part of one of the subcommands we have now for validation. This would have the benefit of being executed by CI.

I think a good place could be in elastic-package test static, there we are only checking the sample events, but checking the events generated by stream could also fit there.

Other place could be elastic-package check, but this is more concerned about the structure of the package than its behavior.

It makes sense to me to test as much as possible via a central command - I added this to test static, but IMHO there is value in having a dedicated command as well to be able to iterate on only the benchmark config.

What do you think about the most recent version that wires up the same logic in both places?

there is value in having a dedicated command as well to be able to iterate on only the benchmark config.

We don't have this kind of granularity in other cases, and the test asset subcommand is pretty fast, so even when iterating on data generation only the asset tests can still be used to test them.

Additionally, we had discussions regarding whether data generation features, such as the stream subcommand, belong to benchmark, because these features could be used in other cases.

So I would prefer to add it only in test static. We can consider adding an specific subcommand later it there is some reason that makes test static unsuitable for this.

jsoriano · 2024-03-18T12:58:00Z

internal/benchrunner/runners/stream/runner.go

+			// check whether the generated event is valid json
+			var event map[string]any
+			err = json.Unmarshal(buf.Bytes(), &event)
+			if err != nil {
+				return fmt.Errorf("[%s] failed to unmarshal json event: %w, generated output: %s", scenarioName, err, buf.String())
+			}


It would be also nice to validate the fields, as we do in tests and when validating the sample documents, for example here:

elastic-package/internal/testrunner/runners/static/runner.go

Line 117 in a3e3a80

multiErr := fieldsValidator.ValidateDocumentBody(content)

I looked into this, but the fields validator partially depends on test configurations like numeric_keyword_fields which might not be available / can't be matched properly to the benchmark config.

I would like to split that part out of this PR if you don't mind.

Ok, we can add this later. We would indeed need to find a solution for the settings you mention.

paolafrancesca · 2024-03-19T02:26:58Z

internal/benchrunner/runners/stream/runner.go

 	"github.com/elastic/elastic-package/internal/signal"
 )

+const numberOfEvents = 10


beware that 10 events might not be enough to check an error in a template

for example this template: https://github.com/elastic/integrations/blob/main/packages/aws/_dev/benchmark/rally/ec2metrics-benchmark/template.ndjson#L117-L210

here we render different content according the the value of $dimensionType: https://github.com/elastic/integrations/blob/main/packages/aws/_dev/benchmark/rally/ec2metrics-benchmark/config.yml#L4-L12

there is 1/40 possibility that a specific content will be rendered

I bumped it to 100, the generation is fairly quick, it shouldn't matter. Is this enough or should we go even higher?

…idation

flash1293 · 2024-03-22T10:15:58Z

Thanks @aspacca and @jsoriano for review, could you have another look?

flash1293 · 2024-03-22T10:40:05Z

I'm seeing the same test failure on other PRs a well, I'm not sure it is related.

jsoriano · 2024-03-25T20:00:34Z

/test

jsoriano

Looking to this change makes me think that we should move benchmarks from the root of the package to each data stream, as they are so far specific to data streams. Having benchmarks disconnected from data streams is going to cause problems, as the ones I mention of running repeated tests if we integrate this validation into test subcommands.

An option to progress with this change before these refactors could be to add this StaticValidation as a fail fast mechanism to benchmark rally and benchmark stream. Developers could use benchmark stream to quickly validate the generated data.

jsoriano · 2024-03-25T20:15:03Z

cmd/benchmark.go

+	validateCmd := getValidateCommand()
+	cmd.AddCommand(validateCmd)


there is value in having a dedicated command as well to be able to iterate on only the benchmark config.

We don't have this kind of granularity in other cases, and the test asset subcommand is pretty fast, so even when iterating on data generation only the asset tests can still be used to test them.

Additionally, we had discussions regarding whether data generation features, such as the stream subcommand, belong to benchmark, because these features could be used in other cases.

So I would prefer to add it only in test static. We can consider adding an specific subcommand later it there is some reason that makes test static unsuitable for this.

jsoriano · 2024-03-25T20:17:13Z

internal/benchrunner/runners/stream/runner.go

+			// check whether the generated event is valid json
+			var event map[string]any
+			err = json.Unmarshal(buf.Bytes(), &event)
+			if err != nil {
+				return fmt.Errorf("[%s] failed to unmarshal json event: %w, generated output: %s", scenarioName, err, buf.String())
+			}


Ok, we can add this later. We would indeed need to find a solution for the settings you mention.

internal/testrunner/runners/static/runner.go

jsoriano · 2024-03-25T20:49:31Z

internal/testrunner/runners/static/runner.go

+	ctx, stop := signal.Enable(ctx, logger.Info)
+	defer stop()
+
+	err := stream.StaticValidation(ctx, stream.NewOptions(withOpts...))


Could you add a test package where this validation fails? This would need to be added to the false_positives directory, where expected errors can be defined.

jsoriano · 2024-03-25T20:52:52Z

internal/testrunner/runners/static/runner.go

+
+func (r runner) verifyStreamConfig(ctx context.Context, packageRootPath string) []testrunner.TestResult {
+	resultComposer := testrunner.NewResultComposer(testrunner.TestResult{
+		Name:       "Verify benchmark config (if available)",


It would be nice if these results are only in the output if they are actually available.

This looks now like this in packages without benchmarks:

╭─────────┬─────────────┬───────────┬────────────────────────────────────────┬────────┬──────────────╮ │ PACKAGE │ DATA STREAM │ TEST TYPE │ TEST NAME │ RESULT │ TIME ELAPSED │ ├─────────┼─────────────┼───────────┼────────────────────────────────────────┼────────┼──────────────┤ │ apache │ access │ static │ Verify benchmark config (if available) │ PASS │ 508.237µs │ │ apache │ access │ static │ Verify sample_event.json │ PASS │ 84.524208ms │ │ apache │ error │ static │ Verify benchmark config (if available) │ PASS │ 638.916µs │ │ apache │ error │ static │ Verify sample_event.json │ PASS │ 76.622388ms │ │ apache │ status │ static │ Verify benchmark config (if available) │ PASS │ 642.803µs │ │ apache │ status │ static │ Verify sample_event.json │ PASS │ 64.9535ms │ ╰─────────┴─────────────┴───────────┴────────────────────────────────────────┴────────┴──────────────╯

At the moment all benchmarks are named <data stream>-benchmark. We could maybe follow this convention here to check if the config is available and to run the checks per data-stream.

Update: they are actually also bound to data streams in their configs, a data stream name is mandatory:

elastic-package/internal/benchrunner/runners/stream/scenario.go

Lines 73 to 75 in a44250e

if c.DataStream.Name == "" {

return nil, errors.New("can't read data stream name from benchmark configuration: empty")

}

ruflin · 2024-03-27T08:01:01Z

@jsoriano It would be nice to still have the option for a quick win here without requiring refactoring to data streams to have validation.

jsoriano · 2024-03-27T10:10:19Z

@jsoriano It would be nice to still have the option for a quick win here without requiring refactoring to data streams to have validation.

Yep, agree, this is why I propose to do the validation as a pre-check in the commands that use these files, it is pretty fast.

An option to progress with this change before these refactors could be to add this StaticValidation as a fail fast mechanism to benchmark rally and benchmark stream. Developers could use benchmark stream to quickly validate the generated data.

flash1293 · 2024-03-27T10:49:00Z

Thanks for the discussion @jsoriano and @ruflin

Agreed that having the benchmark config defined on the stream level looks like it makes sense, but of course it's a bigger refactoring.

An option to progress with this change before these refactors could be to add this StaticValidation as a fail fast mechanism to benchmark rally and benchmark stream. Developers could use benchmark stream to quickly validate the generated data.

We can start with that, but I really like the point you brought up about this being checked in CI - having it as part of rally and stream would only marginally speed up these commands failing.

Two other thoughts:

What do you think about making the static runner work on both packages and data streams? This is not a category right now, but we could add it by a similar flag like CanRunPerDataStream defined on each runner: CanRunForPackageRoot - the static testrunner would return true for both and its run method would be called for both the package and each individual data stream (and the test runner would decide based on that what static checks to run). This seems like it could be useful more universally as well.

What do you think about adding this check to a new test command (test benchmark) which can then be run in CI along with the static test command?

jsoriano · 2024-03-27T12:58:43Z

What do you think about making the static runner work on both packages and data streams?

What do you think about adding this check to a new test command (test benchmark) which can then be run in CI along with the static test command?

I would be cautious about adding these features, we would be circumventing current tech debt, by adding more tech debt.

Current benchmark scenarios are already bound to data streams, they require a data stream name in their configs, we don't really need to support them at the package level. test static looks like a good place to add this, because it is already being used to test sample documents, and runs per data stream.

What do you think about my comment here?

We could read the scenarios, and from them know if there is any scenario for a given data stream, and if there are, validate only them. This would allow to include this in the current test static.

flash1293 · 2024-03-27T13:25:40Z

What do you think about my comment #1718 (comment)?

We could read the scenarios, and from them know if there is any scenario for a given data stream, and if there are, validate only them. This would allow to include this in the current test static.

As they are already bound to the data stream this way I agree, let's do it like that. I will update the PR.

…idation

flash1293 · 2024-04-08T08:18:40Z

@jsoriano could you take another look?

jsoriano

Looks great now, thanks!

internal/benchrunner/runners/stream/runner.go

flash1293 · 2024-04-09T09:53:53Z

Thanks @jsoriano , updated

jsoriano

Thanks!

elasticmachine · 2024-04-09T10:30:49Z

💚 Build Succeeded

Buildkite Build
Commit: ca081ee

History

💚 Build #2737 succeeded 9a6ad9f
💔 Build #2736 failed bec28e4
💚 Build #2686 succeeded b6c36eb
💔 Build #2666 failed b6c36eb
💚 Build #2585 succeeded 9cd964a
💚 Build #2578 succeeded 3410aaf

flash1293 added 2 commits March 8, 2024 17:03

first version

5bdae70

remove unncessary stuff@

68679c2

flash1293 requested a review from ruflin March 8, 2024 16:12

flash1293 added 2 commits March 8, 2024 17:14

terminate setUp early in case of initialize error

4ff5e2a

update readme

3410aaf

ruflin requested review from jsoriano and paolafrancesca March 11, 2024 14:13

ruflin reviewed Mar 11, 2024

View reviewed changes

internal/benchrunner/runners/stream/runner.go Outdated Show resolved Hide resolved

flash1293 added 2 commits March 13, 2024 13:03

review comments

fa87a81

Merge remote-tracking branch 'upstream/main' into 1610-add-static-val…

9cd964a

…idation

ruflin requested a review from mrodm March 13, 2024 15:19

jsoriano reviewed Mar 18, 2024

View reviewed changes

paolafrancesca approved these changes Mar 19, 2024

View reviewed changes

paolafrancesca reviewed Mar 19, 2024

View reviewed changes

flash1293 added 3 commits March 22, 2024 10:53

Merge remote-tracking branch 'upstream/main' into 1610-add-static-val…

f453187

…idation

bump number of generated events

c8038ce

review comments

b6c36eb

jsoriano reviewed Mar 25, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 1610-add-static-val…

062dc76

…idation

flash1293 added 2 commits April 5, 2024 17:39

comment reviews

bec28e4

update readme

9a6ad9f

jsoriano reviewed Apr 8, 2024

View reviewed changes

internal/benchrunner/runners/stream/runner.go Outdated Show resolved Hide resolved

internal/benchrunner/runners/stream/runner.go Outdated Show resolved Hide resolved

internal/benchrunner/runners/stream/runner.go Outdated Show resolved Hide resolved

review comment

ca081ee

jsoriano approved these changes Apr 9, 2024

View reviewed changes

flash1293 merged commit 9315e0b into elastic:main Apr 9, 2024

mrodm mentioned this pull request Aug 13, 2024

Add expected errors for benchmark test package [false positive test] #2017

Merged

		validateCmd := getValidateCommand()
		cmd.AddCommand(validateCmd)

	if c.DataStream.Name == "" {
	return nil, errors.New("can't read data stream name from benchmark configuration: empty")
	}

Add benchmark validate command for static validation #1718

Add benchmark validate command for static validation #1718

Uh oh!

Conversation

flash1293 commented Mar 8, 2024

Implementation details

Uh oh!

Uh oh!

ruflin commented Mar 11, 2024

Uh oh!

flash1293 commented Mar 13, 2024

Uh oh!

ruflin commented Mar 13, 2024

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsoriano Mar 18, 2024 • 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!

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!

flash1293 commented Mar 22, 2024

Uh oh!

flash1293 commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsoriano commented Mar 25, 2024

Uh oh!

jsoriano left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano Mar 27, 2024 • 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!

ruflin commented Mar 27, 2024

Uh oh!

jsoriano commented Mar 27, 2024

Uh oh!

flash1293 commented Mar 27, 2024

Uh oh!

jsoriano commented Mar 27, 2024

Uh oh!

flash1293 commented Mar 27, 2024

Uh oh!

flash1293 commented Apr 8, 2024

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flash1293 commented Apr 9, 2024

Uh oh!

jsoriano left a comment

jsoriano Mar 18, 2024 •

edited

Loading

flash1293 commented Mar 22, 2024 •

edited

Loading

jsoriano Mar 27, 2024 •

edited

Loading