-
Notifications
You must be signed in to change notification settings - Fork 456
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
Rally benchmark aws.sqs #8429
Rally benchmark aws.sqs #8429
Conversation
🌐 Coverage report
|
"ephemeral_id": "cdaaaabb-be7e-432f-816b-bda019fd7c15", | ||
"version": "8.3.2" | ||
}, | ||
"elastic_agent": { "id": "2d4b09d0-cdb6-445e-ac3f-6415f87b9864", "version": "8.3.2", "snapshot": false }, |
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.
Having the templates "expanded" with all fields would make it more readable / more reviewable. The problem I hit when I tried to do it myself is that tis is not "valid" JSON so pretty does not do it directly for you without removing the template parts.
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.
yes. I know, not sure we can tweak this problem completely
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.
One way of partially solving is putting at least the examples we have at the moment "expanded" by doing the manual work. That will make it easier for other just for example copy the elastic-agent part and arleady have it in the right format :-)
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.
@endorama a while ago proposed to have some importable templates for this kind of "common boilerplate".
it can be done just with a function indeed, something like
...
"agent": {{ evenAgentMetadata }},
...
or even only
{{ evenAgentMetadata }},
also for event
: this would require to override the configs with some runtime values
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.
I like {{ evenAgentMetadata }},
as it makes it really easy to add these blocks. We could have the same for host and k8s and then even have a some events with one, some with the other based on a if rand() % 2 { {{host}} } else { {{k8s}} }}
.
I assume this would be some "hardcoded" variables somewhere in the tooling everyone could use?
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.
I assume this would be some "hardcoded" variables somewhere in the tooling everyone could use?
yes, we need to use some variables (not for every field, but a few of them): either we add a way to pass "runtime fields and config" to the generator package, or we could simply "hack" inside elastic-package
dynamically adding the fields and config we need for this helpers once we have parsed the yaml files
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.
Do we have a Github issue or similar to track this idea?
{{- $queueName := generate "QueueName" -}} | ||
{{- $timestamp := generate "timestamp" -}} | ||
{ | ||
"@timestamp": "{{ $timestamp.Format "2006-01-02T15:04:05.999999Z07:00" }}", |
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.
Would this also work if $timestamp.Format '2006-01-02T15:04:05.999999Z07:00'
would be used? Mainly asking in the context of the pretty question below.
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.
it's gotext template: you can use:
$timestamp.Format `2006-01-02T15:04:05.999999Z07:00`
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.
Will this solve the JSON pretty problem?
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.
(ignoring the first 2 lines)
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.
Will this solve the JSON pretty problem?
only for that specific line
json with integer like: {"pc": {{.pct}} }
, or the end/if syntax will still break it
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.
Yeah, there is too much other "non json" in there. The parallel discussion we had around validating the outcome of the template might work better to get quick feedback if there is a typo inside.
/fest |
Enhancement
Proposed commit message
Add artifacts for
elastic-package
rally benchmarkChecklist
- [ ] I have added an entry to my package'schangelog.yml
file.- [ ] I have verified that Kibana version constraints are current according to guidelines.Author's Checklist
How to test this PR locally
From
aws
package root (remember to bring up the elastic-package stack before):./elastic-package benchmark rally --benchmark sqs-benchmark -v
Related issues
Screenshots