Rally benchmark aws.billing #8403
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
|
/test |
🌐 Coverage report
|
| @@ -0,0 +1,51 @@ | |||
| - name: timestamp | |||
There was a problem hiding this comment.
@aspacca Could we read this also directly from the dataset itself? How is this exactly used in addition to the dataset template fields?
Happy to keep it for now if it is required.
There was a problem hiding this comment.
we cannot use from dataset template fields. because they are schema-c: fields might be different that what we have in schema-b, also some complex integrations might need extra fields to apply logic in the shape of the events (see https://github.com/elastic/elastic-integration-corpus-generator-tool/blob/main/assets/templates/aws.ec2_metrics/schema-b/gotext.tpl for an example)
There was a problem hiding this comment.
Thanks for the reminder, makes sense. Two follow up questions:
- Do we need it for all fields or could we for example assume "keyword" by default?
- Could we add it directly to config.yml. I remember there was a reason it is separate but don't remember why.
There was a problem hiding this comment.
- Do we need it for all fields or could we for example assume "keyword" by default?
we need an entry in fields only if we want to generate a value for it (meaning that there's no need for a dataset field if we can just write in the template {"dataset":"aws.billing"}), we could default the type to "keyword" if not type is defined. but I'd rather keep it explicit
we don't need an entry in config for every entry in fields if we don't want to configure any special generation behavior (range, period, etc)
- Could we add it directly to config.yml. I remember there was a reason it is separate but don't remember why.
the historical reason was that there were no fields.yml in the beginning, since the generator started with schema-c and we already had it in the package. so only the config was required. we kept them separated once we moved to other schema.
There was a problem hiding this comment.
the historical reason was that there were no fields.yml in the beginning, since the generator started with schema-c and we already had it in the package. so only the config was required. we kept them separated once we moved to other schema.
I'm tempted to recommend to have only one file as currently I'm jumping forth and back between config.yml and fields.yml but not top priority at the moment. If I remember correctly, we do a merge anyways of the two files into 1? If yes, does it mean potentially having if in config.yml directly would already work, thinking of the migration path.
There was a problem hiding this comment.
i think having a required type field in config.yaml would be better than two files
There was a problem hiding this comment.
If I remember correctly, we do a merge anyways of the two files into 1?
indeed no: we unmarshal fields.yml in a Field struct and config.yml in a ConfigField struct
at least one definition is present for both: value (in case you want to use a fixed value for a field. image the dataset).
it's not a problem to merge the two struct in a single one and remove the repetition of the definitions, but this will work only for non-schema-c data.
if we still want to support schema-c data (I know that horde uses them) we end up with two different type of configurations: separated fields.yml and config.yml for schema-c and a single yml file for the rest
There was a problem hiding this comment.
For schema c, why do we need the fields.yml? Isn't this already defined by the dataset fields.yml?
Lots of connected things here. Lets get the tracks in and then interate.
There was a problem hiding this comment.
For schema c, why do we need the fields.yml? Isn't this already defined by the dataset fields.yml?
we don't create a fields.yml for schema-c, since we use the ones defined by the dataset, but still it they are separated from config.yml: what's what I meant
There was a problem hiding this comment.
What will help here will be a quick write up on "why" we have each file somewhere in the docs. The files can be used in different scenarios and everyone only looking at generation of schema B will miss that there is a bigger picture. Having this write up will also simplify any future refactoring and to bring everyone up-to-speed.
| - name: metricset.period | ||
| value: 86400 | ||
| - name: aws.billing.group_definition.key | ||
| # NOTE: repeated values are needed to produce 10% cases with "" value |
There was a problem hiding this comment.
a weights option would really help here, we have lots of these types of enums eh
|
@tommyers-elastic all good here? :) |
|
@elastic/ecosystem I'd need your CR |
Enhancement
Proposed commit message
Add artifacts for
elastic-packagerally benchmarkChecklist
- [ ] I have added an entry to my package'schangelog.ymlfile.- [ ] I have verified that Kibana version constraints are current according to guidelines.Author's Checklist
How to test this PR locally
Checkout branch from elastic/elastic-package#1522, build
elastic-packageand execute fromawspackage root (remember to bring up the elastic-package stack before):./elastic-package benchmark rally --benchmark billing-benchmark -vRelated issues
Screenshots