Add benchmarks to pipeline tests #958

marc-gr · 2022-09-07T08:36:39Z

Recreated from #906

xUnit format doesn't allow multiple results in a single file.

mergify · 2022-09-07T08:37:06Z

⚠️ The sha of the head commit of this PR conflicts with #906. Mergify cannot evaluate rules on this PR. ⚠️

marc-gr · 2022-09-07T08:38:08Z

@Mergifyio refresh

mergify · 2022-09-07T08:38:15Z

refresh

✅ Pull request refreshed

elasticmachine · 2022-09-07T09:05:00Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-09-08T13:48:58.596+0000
Duration: 26 min 13 sec

Test stats 🧪

Test	Results
Failed	0
Passed	823
Skipped	0
Total	823

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2022-09-07T09:05:11Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`34/34`)	💚
Files	66.929% (`85/127`)	👎 -0.284
Classes	61.667% (`111/180`)	👎 -1.275
Methods	48.693% (`354/727`)	👎 -1.235
Lines	32.074% (`3201/9980`)	👎 -1.28
Conditionals	100.0% (`0/0`)	💚

jsoriano

Thanks Marc for continuing with this! I have added some comments, but mostly questions and nitpicking.

I think that this is mostly fine, maybe we can merge it and continue with follow ups. It looks quite useful already as is.

cmd/benchrunner.go

docs/howto/pipeline_benchmarking.md

jsoriano · 2022-09-07T08:57:37Z

docs/howto/pipeline_benchmarking.md

+        },
+        {
+            "@timestamp": "2016-10-25T12:49:34.000Z",
+            "message": "127.0.0.1 - - [07/Dec/2016:11:05:07 +0100] \"GET /taga HTTP/1.1\" 404 169 \"-\" \"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:49.0) Gecko/20100101 Firefox/49.0\"\n"


As we are going to repeat documents to reach num_docs, we may want to add some templatization here, for example to generate dates. In case ES has some kind of cache when parsing certain things.

But to be done as a follow up if needed/wanted.

Yes I was thinking about this, maybe even some sort of fuzzing or property based document generation at some point.

internal/benchrunner/benchmark.go

internal/benchrunner/runners/pipeline/benchmark.go

jsoriano · 2022-09-07T09:19:12Z

internal/benchrunner/runners/pipeline/test_case.go

+// does not default to unmarshaling numeric values to float64 in order to
+// prevent low bit truncation of values greater than 1<<53.
+// See https://golang.org/cl/6202068 for details.
+func jsonUnmarshalUsingNumber(data []byte, v interface{}) error {


Maybe we should move this to some common place, as is used also in test runner. But it is fine here by now.

docs/howto/pipeline_benchmarking.md

mtojek

Mostly minor things.

Left a comment about the similarity with the test runner to verify if we should do something about this.

README.md

internal/benchrunner/reporters/formats/human.go

internal/benchrunner/runners/pipeline/runner.go

mtojek · 2022-09-07T11:25:25Z

internal/benchrunner/benchrunner.go

+}
+
+// FindBenchmarkFolders finds benchmark folders for the given package and, optionally, benchmark type and data streams
+func FindBenchmarkFolders(packageRootPath string, dataStreams []string, benchType BenchType) ([]testrunner.TestFolder, error) {


I see that a lot of the benchrunner logic is similar to the testrunner. Frankly speaking, we consider that part of elastic-package as a candidate to be refactored :) I'm wondering if you can extract some common elements of both runners.

If it's a stupid idea, feel free to decline. I know that the logic is tangled here.

The reason I preferred to duplicate it in this case is because I am not sure of how this feature might evolve in the near future, so is possible that the logic starts diverging more as time goes by. I am happy to do that if you prefer so now, but given the logic is quite coupled I think it was safer to just assume some level of duplication given both features are isolated and are not intended to evolve together at this point.

- Rename benchrunner.go -> benchmark.go - Add data-streams as a persistent flag - Add num-top-procs flag - Implement Stringer for BenchmarkValue - Rename benchFmtd -> benchFormatted - Make constants global

jsoriano

Thanks!

mtojek

LGTM!

Nice job @adriansr and @marc-gr :) Please email the internal group to share more details on how to use it!

mtojek · 2022-09-08T13:46:06Z

@dependabot merge upstream

adriansr and others added 20 commits July 22, 2022 01:30

Add benchmarks to pipeline tests

d86dd39

gofumpt

9364cff

Minor rename

ded422d

Generate benchmark output files per datastream

e097ac1

xUnit format doesn't allow multiple results in a single file.

Filter-out detailed reports in xUnit

f828106

Cleanup config options

ea3760c

Move benchmark code to its own command

5c16fe2

Extract common ingest pipeline code

8f756bc

Remove unused code from benchmark runner

1f4c474

Benchmark runner reporting

26ba4cc

Make benchmarks have a dedicated _dev config folder

254a262

Add doc

689bb4b

Merge remote-tracking branch 'upstream/main' into bench_pipelines

b836bf9

Remove unused method after merge

4d0f3d3

Re-generate readme

0d81b4f

Fix benchmark commands in doc

0456c35

Add fallback to use pipeline test samples

1ba3790

Add CI testing for benchmarks

3c50c8e

Add output to doc, undo script change, change test packages PR number

a029a95

Merge remote-tracking branch 'upstream/main' into bench_pipelines

51eafe2

marc-gr mentioned this pull request Sep 7, 2022

Add benchmarks to pipeline tests #906

Closed

jsoriano reviewed Sep 7, 2022

View reviewed changes

mtojek reviewed Sep 7, 2022

View reviewed changes

Make suggested changes:

cf2d739

- Rename benchrunner.go -> benchmark.go - Add data-streams as a persistent flag - Add num-top-procs flag - Implement Stringer for BenchmarkValue - Rename benchFmtd -> benchFormatted - Make constants global

ruflin mentioned this pull request Sep 8, 2022

Better debugging of Ingest Pipelines in Integrations elastic/integrations#4150

Closed

marc-gr requested review from jsoriano and mtojek September 8, 2022 09:19

jsoriano approved these changes Sep 8, 2022

View reviewed changes

mtojek approved these changes Sep 8, 2022

View reviewed changes

marc-gr added 2 commits September 8, 2022 12:49

readme update

0124831

Merge remote-tracking branch 'upstream/main' into bench_pipelines

b66b39a

jsoriano approved these changes Sep 8, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into bench_pipelines

f5e8b8e

mtojek merged commit e0f49ac into elastic:main Sep 8, 2022

marc-gr deleted the bench_pipelines branch September 8, 2022 14:29

epixa mentioned this pull request Sep 9, 2022

Performance testing for ingest pipelines #855

Closed

3 tasks

epixa assigned marc-gr Sep 15, 2022

Add benchmarks to pipeline tests #958

Add benchmarks to pipeline tests #958

Uh oh!

Conversation

marc-gr commented Sep 7, 2022

Uh oh!

mergify bot commented Sep 7, 2022

Uh oh!

marc-gr commented Sep 7, 2022

Uh oh!

mergify bot commented Sep 7, 2022

✅ Pull request refreshed

Uh oh!

elasticmachine commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoriano Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

marc-gr Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoriano Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtojek Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

marc-gr Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mtojek commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elasticmachine commented Sep 7, 2022 •

edited

Loading

elasticmachine commented Sep 7, 2022 •

edited

Loading