-
Notifications
You must be signed in to change notification settings - Fork 7
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
Query grouping framework for Top N queries and group by query similarity #66
Conversation
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.
The overall class designs looks good to me.
But I have concerns on the correctness of the logic in QueryGroupingService to implement algorithms proposed in opensearch-project/OpenSearch#13357 (comment). Please see the individual comments for details.
Also, a lot of the heap operations are O(n)
and O(total possible number of groups)
, which is not acceptable. Please refer to the comment: opensearch-project/OpenSearch#13357 (comment) to resolve it.
src/main/java/org/opensearch/plugin/insights/rules/model/DimensionType.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/Measurement.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/Measurement.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/Measurement.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/Measurement.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/SearchQueryRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/SearchQueryRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/plugin/insights/QueryInsightsTestUtils.java
Show resolved
Hide resolved
src/test/java/org/opensearch/plugin/insights/core/service/QueryGroupingServiceTests.java
Outdated
Show resolved
Hide resolved
Please also add integration tests for this feature - We are already lacking integration test coverage for many features in Query Insights. |
We can run some benchmarks to view the performance here and keep this feature as experimental/beta and also limit the number of groups. If needed we can use an indexed priority queue from here as followup changes. Let me know your thoughts. |
I don't think this is a good idea, the whole algorithm mentioned in opensearch-project/OpenSearch#13357 (comment) is based on using indexed pq to store the groups. Otherwise we are storing all the queries groups, updating / deletion can take O(total possible number of groups) in a worst case scenario, which is not acceptable. |
Made all the required refactoring based on the comments. Highlights include:
Only major open question is regarding the java priority queue verses indexed priority queue.
Lets discuss more and figure out a path forward! |
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.
Overall the logic for query grouping seems complex, and I am wondering if there is a way to simplify some of it by making some reasonable assumptions
src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryShape.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGroupingService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGroupingService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/TopQueriesService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/rules/model/AggregationType.java
Outdated
Show resolved
Hide resolved
f36d2a9
to
886ca2f
Compare
Ran some benchmarks to figure out a reasonable number for the cardinality of the groups and here are the results:
Note that window size set to : IMHO it might be reasonable to having a setting |
src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java
Show resolved
Hide resolved
If only query metrics is enabled we always add the records. Not sure what you are referring to here? |
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java
Show resolved
Hide resolved
As discussed added interfaces and implementation for the following:
|
src/main/java/org/opensearch/plugin/insights/core/service/TopQueriesService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/TopQueriesService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/grouper/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/grouper/QueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/grouper/MinMaxHeapQueryGrouper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/grouper/MinMaxHeapQueryGrouper.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Siddhant Deshmukh <[email protected]>
Signed-off-by: Siddhant Deshmukh <[email protected]>
Signed-off-by: Siddhant Deshmukh <[email protected]>
Signed-off-by: Siddhant Deshmukh <[email protected]>
Signed-off-by: Siddhant Deshmukh <[email protected]>
Signed-off-by: Siddhant Deshmukh <[email protected]>
src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Siddhant Deshmukh <[email protected]>
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.
The security based integ tests are failing for the change: #85
Let's double check if we are missing anything for this change on the permission side before merging.
Signed-off-by: Siddhant Deshmukh <[email protected]>
Thanks for checking! The integration test PR build is failing due to grouping settings not found. This PR needs to be merged for the builds to pass there. The security ITs are run in the checks for this PR. Also ran the security ITs locally and they are passing. |
Signed-off-by: Siddhant Deshmukh <[email protected]>
if (maxHeapQueryStore.size() > 0) { | ||
addToMaxPQPromoteToMinPQ(aggregateSearchQueryRecord, groupId); | ||
} else { | ||
addToMinPQOverflowToMaxPQ(aggregateSearchQueryRecord, groupId); | ||
} |
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.
Why do we need to do this if/else
here? Can't we simply always add to max/min queue and then do a swap top
?
Also the "else" logic looks effectiveness to me. If the execution enters this else, that means we have already removed a record from the min queue, and max queue is also empty. So when we do addToMinPQOverflowToMaxPQ
we are adding the previous (but updated) record back it to the min queue again and we won't overflow anything to max queue.
} | ||
|
||
private boolean checkMaxGroupsLimitReached(String groupId) { | ||
if (maxGroups <= maxHeapQueryStore.size() && minHeapTopQueriesStore.size() >= topNSize) { |
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.
We should emit a metric for this as well.
public static final GroupingType DEFAULT_GROUPING_TYPE = GroupingType.NONE; | ||
public static final int DEFAULT_GROUPS_EXCLUDING_TOPN_LIMIT = 100; | ||
|
||
public static final int MAX_GROUPS_EXCLUDING_TOPN_LIMIT = 10000; |
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.
How much memory would 10000 records consume based on the benchmark results?
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.
Did not capture this and we did not reach anywhere close to the 10000 limit in the benchmarks.
If a search query record is around 1kb then 10000 groups means we will consume 10mb memory at most for this feature, which should be fine. but we still need to watch out on the memory consumption here for queries with large source.
This analysis seems reasonable but we would need to keep watch out for the memory consumption here.
The benchmark numbers looks good, I think it's a good idea to confirm the reasonable upper bound for number of groups as well so that we won't consume too much memory. |
* @return return the search query record that represents the group | ||
*/ | ||
@Override | ||
public SearchQueryRecord add(SearchQueryRecord searchQueryRecord) { |
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.
This grouper can be simplied with something like:
public SearchQueryRecord add(SearchQueryRecord searchQueryRecord) {
if (!groupIdToAggSearchQueryRecord.containsKey(groupId)) {
boolean maxGroupsLimitReached = checkMaxGroupsLimitReached(groupId);
if (maxGroupsLimitReached) {
return null;
}
aggregateSearchQueryRecord = searchQueryRecord;
aggregateSearchQueryRecord.setGroupingId(groupId);
aggregateSearchQueryRecord.setMeasurementAggregation(metricType, aggregationType);
addToMinPQ(aggregateSearchQueryRecord, groupId);
} else {
aggregateSearchQueryRecord = groupIdToAggSearchQueryRecord.get(groupId).v1();
boolean isPresentInMinPQ = groupIdToAggSearchQueryRecord.get(groupId).v2();
if (isPresentInMinPQ) {
minHeapTopQueriesStore.remove(aggregateSearchQueryRecord);
} else {
maxHeapTopQueriesStore.remove(aggregateSearchQueryRecord);
}
addAndPromote(searchQueryRecord, aggregateSearchQueryRecord, groupId);
}
return aggregateSearchQueryRecord;
}
private void addToMinPQ(SearchQueryRecord searchQueryRecord, String groupId) {
minHeapTopQueriesStore.add(searchQueryRecord);
groupIdToAggSearchQueryRecord.put(groupId, new Tuple<>(searchQueryRecord, true));
overflow();
}
private void addAndPromote(SearchQueryRecord searchQueryRecord, SearchQueryRecord aggregateSearchQueryRecord, String groupId) {
Number measurementToAdd = searchQueryRecord.getMeasurement(metricType);
aggregateSearchQueryRecord.addMeasurement(metricType, measurementToAdd);
addToMinPQ(aggregateSearchQueryRecord, groupId);
if (maxHeapQueryStore.isEmpty()) {
return;
}
if (SearchQueryRecord.compare(maxHeapQueryStore.peek(), minHeapTopQueriesStore.peak(), metricType) > 0) {
SearchQueryRecord recordMovedFromMaxToMin = maxHeapQueryStore.poll();
addToMinPQ(recordMovedFromMaxToMin, recordMovedFromMaxToMin.getGroupingId());
}
}
private void overflow() {
if (minHeapTopQueriesStore.size() > topNSize) {
SearchQueryRecord recordMovedFromMinToMax = minHeapTopQueriesStore.poll();
maxHeapQueryStore.add(recordMovedFromMinToMax);
groupIdToAggSearchQueryRecord.put(recordMovedFromMinToMax.getGroupingId(), new Tuple<>(recordMovedFromMinToMax, false));
}
}
Overall it looks good and I'm fine approving it. But I still have some concerns and we need follow-ups to resolve them.
|
measurements = new HashMap<>(); | ||
in.readMap(MetricType::readFromStream, StreamInput::readGenericValue) | ||
.forEach(((metricType, o) -> measurements.put(metricType, metricType.parseValue(o)))); | ||
if (in.getVersion().onOrAfter(Version.V_2_17_0)) { |
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'm wondering why is this needed? SearchQueryRecord is only used internally and we are not providing any clients that could cause version mismatch.
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.
This is used when getting the top queries : https://github.com/opensearch-project/query-insights/blob/main/src/main/java/org/opensearch/plugin/insights/rules/action/top_queries/TopQueries.java#L36
Signed-off-by: Siddhant Deshmukh <[email protected]>
Thanks @ansjcy.
|
…ity (#66) * Query grouping framework and group by query similarity Signed-off-by: Siddhant Deshmukh <[email protected]> * Spotless apply Signed-off-by: Siddhant Deshmukh <[email protected]> * Build fix Signed-off-by: Siddhant Deshmukh <[email protected]> * Properly configure settings update consumer Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Refactor unit tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Decouple Measurement and MetricType Signed-off-by: Siddhant Deshmukh <[email protected]> * Aggregate type NONE will ensure no aggregations computed Signed-off-by: Siddhant Deshmukh <[email protected]> * Perform renaming Signed-off-by: Siddhant Deshmukh <[email protected]> * Integrate query shape library with grouping Signed-off-by: Siddhant Deshmukh <[email protected]> * Spotless Signed-off-by: Siddhant Deshmukh <[email protected]> * Create and consume string hashcode interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Health checks in code Signed-off-by: Siddhant Deshmukh <[email protected]> * Fix tests and spotless apply Signed-off-by: Siddhant Deshmukh <[email protected]> * Minor fixes Signed-off-by: Siddhant Deshmukh <[email protected]> * Max groups setting and unit tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Create query grouper interface and top query store interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Removed unused interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Rebase main and spotless Signed-off-by: Siddhant Deshmukh <[email protected]> * Renaming variable Signed-off-by: Siddhant Deshmukh <[email protected]> * Remove TopQueriesStore interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Drain top queries service on group change Signed-off-by: Siddhant Deshmukh <[email protected]> * Rename max groups setting and allow minimum 0 Signed-off-by: Siddhant Deshmukh <[email protected]> * Make write/read from io backword compatible Signed-off-by: Siddhant Deshmukh <[email protected]> * Minor fix Signed-off-by: Siddhant Deshmukh <[email protected]> * Refactor query grouper Signed-off-by: Siddhant Deshmukh <[email protected]> --------- Signed-off-by: Siddhant Deshmukh <[email protected]> (cherry picked from commit 65e4489) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ity (#66) (#86) (cherry picked from commit 65e4489) Signed-off-by: Siddhant Deshmukh <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ity (#66) * Query grouping framework and group by query similarity Signed-off-by: Siddhant Deshmukh <[email protected]> * Spotless apply Signed-off-by: Siddhant Deshmukh <[email protected]> * Build fix Signed-off-by: Siddhant Deshmukh <[email protected]> * Properly configure settings update consumer Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Refactor unit tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Decouple Measurement and MetricType Signed-off-by: Siddhant Deshmukh <[email protected]> * Aggregate type NONE will ensure no aggregations computed Signed-off-by: Siddhant Deshmukh <[email protected]> * Perform renaming Signed-off-by: Siddhant Deshmukh <[email protected]> * Integrate query shape library with grouping Signed-off-by: Siddhant Deshmukh <[email protected]> * Spotless Signed-off-by: Siddhant Deshmukh <[email protected]> * Create and consume string hashcode interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Health checks in code Signed-off-by: Siddhant Deshmukh <[email protected]> * Fix tests and spotless apply Signed-off-by: Siddhant Deshmukh <[email protected]> * Minor fixes Signed-off-by: Siddhant Deshmukh <[email protected]> * Max groups setting and unit tests Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Create query grouper interface and top query store interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Address review comments Signed-off-by: Siddhant Deshmukh <[email protected]> * Removed unused interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Rebase main and spotless Signed-off-by: Siddhant Deshmukh <[email protected]> * Renaming variable Signed-off-by: Siddhant Deshmukh <[email protected]> * Remove TopQueriesStore interface Signed-off-by: Siddhant Deshmukh <[email protected]> * Drain top queries service on group change Signed-off-by: Siddhant Deshmukh <[email protected]> * Rename max groups setting and allow minimum 0 Signed-off-by: Siddhant Deshmukh <[email protected]> * Make write/read from io backword compatible Signed-off-by: Siddhant Deshmukh <[email protected]> * Minor fix Signed-off-by: Siddhant Deshmukh <[email protected]> * Refactor query grouper Signed-off-by: Siddhant Deshmukh <[email protected]> --------- Signed-off-by: Siddhant Deshmukh <[email protected]> (cherry picked from commit 65e4489) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ity (#66) (#104) * Query grouping framework and group by query similarity * Spotless apply * Build fix * Properly configure settings update consumer * Address review comments * Refactor unit tests * Decouple Measurement and MetricType * Aggregate type NONE will ensure no aggregations computed * Perform renaming * Integrate query shape library with grouping * Spotless * Create and consume string hashcode interface * Health checks in code * Fix tests and spotless apply * Minor fixes * Max groups setting and unit tests * Address review comments * Address review comments * Create query grouper interface and top query store interface * Address review comments * Removed unused interface * Rebase main and spotless * Renaming variable * Remove TopQueriesStore interface * Drain top queries service on group change * Rename max groups setting and allow minimum 0 * Make write/read from io backword compatible * Minor fix * Refactor query grouper --------- (cherry picked from commit 65e4489) Signed-off-by: Siddhant Deshmukh <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
For Top N queries by latency, we can encounter scenarios where some (or most) of the Top N queries contain duplicate queries. Say the same dashboard query is triggered continuously and happens to be the most expensive query in terms of latency - in this scenario all the Top N queries by latency will likely be spammed by the same query. To overcome such scenarios and to get a more detailed view of the Top N query patterns we have implemented Grouping Top N queries by similarity. As a followup we can also use this framework to implement grouping top N queries by frequency, user_id, etc.
Major changes:
Issues Resolved
addresses #13357
Configure Grouping
Configure Grouping Error Response
Get Top N Queries by latency with grouping enabled, group_by SIMILARITY
Get Top N queries with group_by NONE
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.
Uploading Screen Recording 2024-08-28 at 4.35.10 PM.mov…