Profiling Extensibility

[neuenfeldttj]

Description

Single profile tree implementation that resolves #18460. Each query has an associated query breakdown and plugin profile breakdown (if there's a plugin associated with it). A plugin/extension may provide multiple breakdowns. Each breakdown is contextual--the plugin profile breakdown is returned to the plugin/extension based on the query context given. The plugin/extension would get the plugin breakdown very similar to how ProfileWeight does it; as long as there's a ContextIndexSearcher instance and a context, profiling information can be generated from the plugin. Aggregation is handled how the current implementation does it -- aggregations of plugin extensions aren't supported.

Related Issues

Resolves #18460 Resolves #17146

Check List

• [x] Functionality includes testing.
• [ ] API changes companion pull request created, if applicable.
• [ ] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions[bot]]

:x: Gradle check result for 40f8987ce00d1327296c9134b2ba2c38fa0f9f93: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[reta]

Thanks for the work, @neuenfeldttj I briefly look at it, have a few high level comments:

• the SearchPlugin should probably expose QueryProfiler (or similar abstraction), focusing just on breakdown part is limiting
• the query profiler injection should be contextual, based on search context (in my opinion), so the plugin / extension could make the decision dynamically
• the existing, let's call it default query profiler, should probably not be aware of any additional profilers, the Profilers already supports the list of QueryProfiler, it seems logical that we should be adding new profilers there
• the getImportantMetrics() it really looking off and shouldn't be there (in my opinion)
• the usual policy within same release line (not sure if it still holds anymore), there should be no breaking changes in publicly exposed APIs (those annotated as @PublicApi)

Hope it makes sense, thank you.

[neuenfeldttj]

Thanks @reta for looking!

the existing, let's call it default query profiler, should probably not be aware of any additional profilers, the Profilers already supports the list of QueryProfiler, it seems logical that we should be adding new profilers there

This would produce multiple profile trees so when the output is generated, there will be a tree-like output for each plugin + the default query profiler. Wouldn't it be better to make all plugins' profiling breakdowns be a part of the same tree the default query profiler outputs?

[reta]

Thanks @reta for looking!

the existing, let's call it default query profiler, should probably not be aware of any additional profilers, the Profilers already supports the list of QueryProfiler, it seems logical that we should be adding new profilers there

This would produce multiple profile trees so when the output is generated, there will be a tree-like output for each plugin + the default query profiler. Wouldn't it be better to make all plugins' profiling breakdowns be a part of the same tree the default query profiler outputs?

That one is not a given - I think we should have a single breakdown, aggregated between all profilers. Does it make sense? Thank you.

[neuenfeldttj]

I tried making the SearchPlugin expose a QueryProfiler but it's a bit more complex than what we think. It worked for the most part, but it won't work for concurrency because all profilers need to know how many slices there are. This happens in a specific function (ContextIndexSearcher.searchLeaf() that for some reason doesn't get called for every child query.