Skip to content
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 insights plugin implementation #11903

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

ansjcy
Copy link
Member

@ansjcy ansjcy commented Jan 17, 2024

Description

This PR implements the basic Query Insights framework as described in #11429

Hightlight of this PR is adding asynchronous processing and exporting capability in the plugin to handle the data for future query insight features. At the first iteration, the processor is now able to handle query latency data and enqueue to the aggregator and also export the aggregated data asynchronously to an OpenSearch index. This framework can potentially be used by other query insight features in the future to avoid adding blocking logic in core search path.
Also added unit tests for the framework.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

Copy link
Contributor

❌ Gradle check result for a9c4e06: 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?

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Compatibility status:

Checks if related components are compatible with change df7381c

Incompatible components

Incompatible components: [https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

@ansjcy ansjcy force-pushed the query-insights-plugin branch from a9c4e06 to f3969f4 Compare January 17, 2024 06:24
Copy link
Contributor

❌ Gradle check result for f3969f4: 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?

Copy link
Contributor

github-actions bot commented Feb 5, 2024

❌ Gradle check result for d06e132:

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?

@deshsidd
Copy link
Contributor

deshsidd commented Feb 6, 2024

@deshsidd I believe those are marked as flaky tests https://github.com/opensearch-project/OpenSearch/issues?q=is%3Aopen+is%3Aissue+label%3A%22flaky-test%22, which are not related to our changes.

Will need them to succeed to merge this right? Maybe a retry will help

@ansjcy ansjcy force-pushed the query-insights-plugin branch from d06e132 to df7381c Compare February 6, 2024 01:01
Copy link
Contributor

github-actions bot commented Feb 6, 2024

❕ Gradle check result for df7381c: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@msfroh msfroh merged commit 3cbf54e into opensearch-project:main Feb 6, 2024
29 of 30 checks passed
@msfroh msfroh added the backport 2.x Backport to 2.x branch label Feb 6, 2024
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-11903-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3cbf54e7358fd707a0d73e6c016b04ce78884d30
# Push it to GitHub
git push --set-upstream origin backport/backport-11903-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-11903-to-2.x.

ansjcy added a commit to ansjcy/OpenSearch that referenced this pull request Feb 6, 2024
* Query insights plugin implementation

Signed-off-by: Chenyang Ji <[email protected]>

* Increase JavaDoc coverage and update PR based comments

Signed-off-by: Chenyang Ji <[email protected]>

* Refactor record and service to make them generic

Signed-off-by: Chenyang Ji <[email protected]>

* refactor service for improving multithreading efficiency

Signed-off-by: Chenyang Ji <[email protected]>

---------

Signed-off-by: Chenyang Ji <[email protected]>
(cherry picked from commit 3cbf54e)
msfroh pushed a commit that referenced this pull request Feb 6, 2024
* Query insights plugin implementation

Signed-off-by: Chenyang Ji <[email protected]>

* Increase JavaDoc coverage and update PR based comments

Signed-off-by: Chenyang Ji <[email protected]>

* Refactor record and service to make them generic

Signed-off-by: Chenyang Ji <[email protected]>

* refactor service for improving multithreading efficiency

Signed-off-by: Chenyang Ji <[email protected]>

---------

Signed-off-by: Chenyang Ji <[email protected]>
(cherry picked from commit 3cbf54e)
peteralfonsi pushed a commit to peteralfonsi/OpenSearch that referenced this pull request Mar 1, 2024
* Query insights plugin implementation

Signed-off-by: Chenyang Ji <[email protected]>

* Increase JavaDoc coverage and update PR based comments

Signed-off-by: Chenyang Ji <[email protected]>

* Refactor record and service to make them generic

Signed-off-by: Chenyang Ji <[email protected]>

* refactor service for improving multithreading efficiency

Signed-off-by: Chenyang Ji <[email protected]>

---------

Signed-off-by: Chenyang Ji <[email protected]>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
* Query insights plugin implementation

Signed-off-by: Chenyang Ji <[email protected]>

* Increase JavaDoc coverage and update PR based comments

Signed-off-by: Chenyang Ji <[email protected]>

* Refactor record and service to make them generic

Signed-off-by: Chenyang Ji <[email protected]>

* refactor service for improving multithreading efficiency

Signed-off-by: Chenyang Ji <[email protected]>

---------

Signed-off-by: Chenyang Ji <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
* Query insights plugin implementation

Signed-off-by: Chenyang Ji <[email protected]>

* Increase JavaDoc coverage and update PR based comments

Signed-off-by: Chenyang Ji <[email protected]>

* Refactor record and service to make them generic

Signed-off-by: Chenyang Ji <[email protected]>

* refactor service for improving multithreading efficiency

Signed-off-by: Chenyang Ji <[email protected]>

---------

Signed-off-by: Chenyang Ji <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
@AmiStrn
Copy link
Contributor

AmiStrn commented May 23, 2024

#11429 (comment)

@ansjcy @msfroh was there a particular reason to create the plugin inside the core project?

@reta I looked and couldn't see anywhere that we specify that plugins should be outside the core project (to reduce bloat, to remove the concept of hierarchy of plugins to name a few reasons). wdyt? should we add such a recommendation?

@reta
Copy link
Collaborator

reta commented May 23, 2024

@reta I looked and couldn't see anywhere that we specify that plugins should be outside the core project (to reduce bloat, to remove the concept of hierarchy of plugins to name a few reasons). wdyt?

@AmiStrn I am 100% with you here, I believe the non-essential plugins (which are not modules) should be considered standalone projects and should not be in core.

should we add such a recommendation?

No doubts we should (in my opinion), thanks for bringing this one up. @ansjcy @msfroh since we are far from the release cut, I think we should address @AmiStrn concern and move this plugin out of core into separate repository. @dblock @andrross folks what do you think?

@AmiStrn
Copy link
Contributor

AmiStrn commented May 29, 2024

@reta I looked and couldn't see anywhere that we specify that plugins should be outside the core project (to reduce bloat, to remove the concept of hierarchy of plugins to name a few reasons). wdyt?

@AmiStrn I am 100% with you here, I believe the non-essential plugins (which are not modules) should be considered standalone projects and should not be in core.

should we add such a recommendation?

No doubts we should (in my opinion), thanks for bringing this one up. @ansjcy @msfroh since we are far from the release cut, I think we should address @AmiStrn concern and move this plugin out of core into separate repository. @dblock @andrross folks what do you think?

continued conversation here : #11429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed Search:Query Insights v2.12.0 Issues and PRs related to version 2.12.0
Projects
Status: Done
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants