-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Search] Add shard delay aggregation #77423
Conversation
So far this makes sense to me based on our discussion, though we'll need to add an expression function for this agg type as well in order for it to work with the near-future version of esaggs. (Not necessary now, but will be necessary in 7.11, so might be worth getting it out of the way at the same time) |
Pinging @elastic/kibana-app-arch (Team:AppArch) |
@lukeelmers Added the expression function, could you give this another look? |
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.
Have a few minor comments but overall this makes sense to me!
It would be nice if we could add a few comments either where the agg type is registered, or in the agg type definition, or both. Just in case someone runs across this later and isn't sure why it is being handled differently.
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.
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.
Looks good.
As this flag doesn't even appear in kibana.yml, I feel we need some internal documentation on this flag.
@@ -105,13 +112,26 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> { | |||
registerUsageCollector(usageCollection, this.initializerContext); | |||
} | |||
|
|||
const aggs = this.aggsService.setup({ registerFunction }); | |||
|
|||
this.initializerContext.config |
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.
Could we pass config in instead of the whole initializer context?
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 think the general pattern for plugins/services is to pass in the entire initializer context in the constructor. (I didn't change the signature here to add an additional parameter, just specified what the config schema should look like. I did, however, change the signature on the public service, which matches the autocomplete service.)
@lizozom I added a comment in the config file explaining what it does. Other than that I'm not sure exactly where we should/would document the setting. Do you have any suggestions? |
@lukasolson the comment is really clear, thanks! |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
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
* [Search] [WIP] Add shard delay aggregation * Add expression functions * Register function * Fix test * Add comment Co-authored-by: Elastic Machine <[email protected]>
Summary
Waiting on https://github.com/elastic/elasticsearch/pull/62082/files.
Adds support for the
shard_delay
agg type for use in future manual and/or functional tests.In elastic/elasticsearch#62082, support for a new aggregation,
shard_delay
, is being added. It is only available in snapshots (not in production). It allows slow queries to be simulated, which is helpful in efforts to ensure Kibana runs smoothly in environments where queries are slow. This PR adds a newAggType
for this aggregation which allows you to specify the delay for the query to run.This new
AggType
is only registered when the kibana.yml setting,data.search.aggs.shardDelay.enabled
is set totrue
. By default, it isfalse
.Checklist