-
Notifications
You must be signed in to change notification settings - Fork 62
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 API spec #625
Query Insights API spec #625
Conversation
Changes AnalysisCommit SHA: 9609a02 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11473076195/artifacts/2091621613 API Coverage
|
Spec Test Coverage Analysis
|
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! Some asks below, iterate till tests/lints pass, add to CHANGELOG.
Signed-off-by: Chenyang Ji <cyji@amazon.com>
Signed-off-by: Chenyang Ji <cyji@amazon.com>
f6ce283
to
0a3d14b
Compare
Signed-off-by: Chenyang Ji <cyji@amazon.com>
0a3d14b
to
459f89c
Compare
Hi @dblock! Thanks a lot for your review. I just pushed a commit based on your comments, let me know if it looks good to you! |
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.
Good. You do have to add query_insights
to the CI/CD matrix.
.github/workflows/test-spec.yml
Outdated
@@ -40,6 +40,8 @@ jobs: | |||
tests: plugins/streaming | |||
- version: 2.17.0 | |||
tests: plugins/notifications | |||
- version: 2.18.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.
2.18 hasn't been released so you need to have a ref
and hub
like below for a 2.18 build if you want to use that, but if query insights was added in 2.15 you should use 2.17, the latest stable release here.
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.
Got it. Just updated to 2.17. Thank you!
00c9b30
to
0070d84
Compare
Signed-off-by: Chenyang Ji <cyji@amazon.com>
0070d84
to
9609a02
Compare
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.
👏 Good work, thanks for hanging in here with me!
@ansjcy I also see an intermittent CI failure, e.g. in https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11487125212/job/31970965603?pr=637 I can reproduce it on a new docker start from tests/plugins/query_insights.
Here's the response:
|
Description
API spec for query insights, more specifically, for the top n queries API.
Related API: https://opensearch.org/docs/latest/observing-your-data/query-insights/top-n-queries/
Issues Resolved
#624
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.