-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Enhance Profile API to Show Star Tree Pre-computation Times #19527
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
base: main
Are you sure you want to change the base?
Enhance Profile API to Show Star Tree Pre-computation Times #19527
Conversation
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 6e2309a: 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? |
…utation in aggregation profiling tree Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for f008d6a: 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? |
|
Thanks @ajleong623 for spending time on this. Before looking into the specific code changes, do you have a before/after view of how the profile output looks from view of a search user. |
|
@sandeshkr419 not yet, but I will make sure to test it out and show the results before tomorrow. It should be very close to the example you shared in the issue. |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 5267341: 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? |
|
❌ Gradle check result for 02e5b2e: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 7059a4e: 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? |
|
❌ Gradle check result for 4eb8843: 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? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19527 +/- ##
============================================
- Coverage 73.21% 73.20% -0.02%
- Complexity 71254 71308 +54
============================================
Files 5766 5768 +2
Lines 325470 325725 +255
Branches 47084 47108 +24
============================================
+ Hits 238296 238434 +138
- Misses 68043 68135 +92
- Partials 19131 19156 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for f12a3a5: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for b460a83: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 7b0f837: null 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for e790eba: 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? |
Description
This change enables star tree profiling and also profiles the
pre-computation phasewhen profiling aggregations. Sometimes when getting building leaf collector, the precomputation phase is part of that, therefore, we want to separate the rest of the precomputation phase from the logic intryPrecomputeAggregationForLeaf.A change I made was to update
ProfilingAggregator.javaso that now it can override the logic oftryPrecomputeAggregationForLeafto account for timing. Additionally, inAggregator.java, I moved the method of precomputing from theAggregatorBase.javatoAggregatorso thatProfilingAggregatorcould override that method.For collecting the profiling information for the star tree sub-phases, I noticed that all aggregators that use star tree both scan the star tree index to find the matched star tree document ids and add those ids into the buckets. All star tree pre-computation supporting aggregators implement
StarTreePreComputeCollector, therefore, I added the methods into that class so that the aggregators could implement them. The reason for this is to separate the logic for scanning the star tree and filling those buckets.The tricky part was understanding the
InternalAggregationProfileTreeand the profiling framework. TheInternalAggregationProfileTreeis the backing that keeps track of the current position in the aggregation and stores and updates the breakdown structures. A problem with theInternalAggregationProfileTreeis that with aggregation profiling, we can only add breakdowns for other aggregations as nodes in the tree. Additionally inSearchProfileShardResults, theInternalAggregationProfileTreewhich extendsAbstractInternalProfileTreeis referenced for serializing profiling results.I knew that the objective would be to have profiling results for star tree precomputation phases, therefore, I created a new class
StarTreeProfileBreakdown.javato hold the profiling information from star tree precomputation.For adding a
StarTreeProfileBreakdownas a child node for an aggregation profile breakdown, I could either rewrite theInternalAggregationProfileTreeto accommodateStarTreeProfileBreakdownnodes or edit theInternalAggregationProfileTree. Luckily, I noticed that when serializing the profile results, I could override the functioncreateProfileResultinAbstractInternalProfileTree. I ended up rewriting that result and storing theStarTreeProfileBreakdownwithinAggregationProfileBreakdownfor serialization since each aggregation can have at most oneStarTreeProfileBreakdown.Finally, for collecting the breakdown results, I create a new breakdown
StarTreeProfileBreakdown.javawhich represents a breakdown for the star tree phase. InAggregationProfileBreakdown.java, I added a few methods to detect whether aStarTreeProfileBreakdownhas been attached and to also keep track of the breakdown. EachAggregationProfileBreakdowncould have at most oneStarTreeProfileBreakdownattached to it. Additionally, inInternalAggregationProfileTree.java, I added some logic to add the star tree profiling result as a child to thatAggregationProfileBreakdown.An example of a an aggregation breakdown is
{ "aggregations": [ { "type": "NumericTermsAggregator", "description": "response_codes", "time_in_nanos": 4649501, "breakdown": { "build_aggregation": 443583, "build_aggregation_count": 1, "build_leaf_collector": 4678959, "build_leaf_collector_count": 1, "collect": 0, "collect_count": 0, "initialize": 3750, "initialize_count": 1, "post_collection": 1250, "post_collection_count": 1, "pre_compute": 4649501, "pre_compute_count": 1, "reduce": 0, "reduce_count": 0 }, "debug": { "total_buckets": 1, "result_selection_strategy": "select_all", "result_strategy": "double_terms" }, "children": [ { "type": "StarTree", "description": "Pre-computation using star-tree index", "time_in_nanos": 4649501, "breakdown": { "build_buckets_from_star_tree": 933584, "build_buckets_from_star_tree_count": 1, "scan_star_tree_segments": 3715917, "scan_star_tree_segments_count": 1 } } ] } ], }The part that changed is the breakdown
For concurrent aggregations, the breakdown looks like:
In aggregation breakdown itself, the
pre_computefield is now available.Related Issues
Resolves #19295
Check List
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.