-
Notifications
You must be signed in to change notification settings - Fork 85
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
[Feature branch] Add Neural Stats API #1208
[Feature branch] Add Neural Stats API #1208
Conversation
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Yes. One use case that I can think of is that, when operator want to grep stats containing processor, this whole flat option can be handy.
|
src/main/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/settings/NeuralSearchSettingsAccessor.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandlerIT.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandlerIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andy Qin <[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.
Addressed review comments, What's left is changing the format of the response according to the discussion above (breaking down high level categories as unflat like all_nodes
), adding examples, and adding BWCs.
.map(String::toLowerCase) | ||
.collect(Collectors.toSet()); | ||
|
||
private NeuralSearchSettingsAccessor settingsAccessor; |
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.
Don't static fields typically go before instance fields?
src/main/java/org/opensearch/neuralsearch/processor/util/RestActionUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/stats/events/EventStatsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/stats/events/TimestampedEventStat.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandlerIT.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/rest/RestNeuralStatsHandlerIT.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/settings/NeuralSearchSettingsAccessor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Added BWC tests. Currently they don't run since there's no backwards versions, but included them to have basis for future 3.x versions. Updated response formatting: Default
Flatten
Flatten w/ metadata
|
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
720ea81
to
647d730
Compare
Signed-off-by: Andy Qin <[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.
Looks good to me, thanks Andy. please address my comment regarding the rebasing on latest main
qa/build.gradle
Outdated
} | ||
testRuntimeOnly group: 'net.minidev', name:'json-smart', version: "${versions.json_smart}" |
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.
you need to rebase on latest main and remove this
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.
My fork branch is up to date with main with latest commits, I think this diff is from the feature branch on this repo being out of date. I don't have write permissions to sync it, could you try syncing it and this should disappear? thanks @junqiu-lei!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/neural-stats-api #1208 +/- ##
==============================================================
- Coverage 81.83% 80.90% -0.93%
+ Complexity 2607 1423 -1184
==============================================================
Files 190 115 -75
Lines 8922 5001 -3921
Branches 1520 803 -717
==============================================================
- Hits 7301 4046 -3255
+ Misses 1028 643 -385
+ Partials 593 312 -281 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8648e13
to
82504ce
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.
Looks good, Nice job Andy
@heemin32 if your all comments are resolved then can we merge this? |
I think my comments are all resolved. G2G. |
89e6932
into
opensearch-project:feature/neural-stats-api
Description
Implementing Neural Stats API framework design proposed in #1196. This initial PR sets up the foundation for the framework to track event and state stats throughout the neural search plugin and exposed their values via API.
See RFC for more details.
Initial implementation includes 3 stats:
Related Issues
Resolves #1196 #1104
Related: #1146
Check List
--signoff
.Example requests
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.