-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds system benchmarks command #1232
Conversation
test/packages/benchmarks/system_benchmark/_dev/benchmark/system/20mb-logs-benchmark.yml
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,40 @@ | |||
- name: IP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it by design that we don't use here names "similar" to ECS like source.ip
? I'm aware this is Schema A so not ECS yet at the same time using ECS could make it easy to read (if possible).
This is just an example for a benchmark tests, I'm mostly worried it sets a precedence on how it should be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case since this represents a Schema A (as it comes from the source), I thought it made more sense to let the field names represent what they are in the original logs, since not always there will even be an ECS field to represent the original data (as it comes after transformations, compositions from several fields, etc.). Even though I do not have a strong opinion on this one I think in this case it might not be too troublesome tbh.
Testing this with the commands provided above and also did some changes to the corpora size for testing. Overall, this looks great and I think we should get this rather soonish. I did not review the code on my end only if the command works as expected. The following I noticed: It tells me in the report the corpora was generated under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just a note about it would be needed to re-generate the README, there is still a reference to pipeline-bench
Pending CI to be successful run, there are some lint errors related to the error messages:
https://buildkite.com/elastic/elastic-package/builds/968#0188aec7-c0b5-48ca-a612-9c62fe86407a/188-330
All comments in the PR are addressed now, it relies on some pending spec changed added in elastic/package-spec#526 |
💚 Build Succeeded
History
cc @marc-gr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the comments!
LGTM
Namespace string `json:"namespace"` | ||
Revision int `json:"revision,omitempty"` | ||
MonitoringEnabled []string `json:"monitoring_enabled"` | ||
MonitoringOutputID string `json:"monitoring_output_id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-gr it looks like this field is not available in old versions of Kibana, it also seems that it is not used here. Was it needed?
Integrations CI is failing for something related to this field elastic/integrations#6673.
Trying to fix it by adding omitempty
in #1320.
cc @mrodm
This adds the ability to define system (end to end) benchmarks at the package level.
benchrunner
to decouple reports presentation from pipeline benchmarks internalsbenchmark system
commandDepends on elastic/package-spec#512
Closes #1164
TODO in the future(in no specific order)
Example of usage:
With a benchmark scenario defined as
<package root>/_dev/benchmark/system/100mb-logs-benchmark.yml
We then run:
To generate a benchmark report:
How to run this example locally: