-
Notifications
You must be signed in to change notification settings - Fork 73
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/extensions] Get detector stats api #857
[Feature/extensions] Get detector stats api #857
Conversation
Signed-off-by: Varun Jain <varunudr@amazon.com>
Signed-off-by: Varun Jain <varunudr@amazon.com>
Signed-off-by: Varun Jain <varunudr@amazon.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## feature/extensions #857 +/- ##
========================================================
+ Coverage 35.96% 36.00% +0.03%
- Complexity 1916 1926 +10
========================================================
Files 299 299
Lines 17615 17647 +32
Branches 1862 1862
========================================================
+ Hits 6335 6353 +18
- Misses 10825 10838 +13
- Partials 455 456 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
*/ | ||
public class RestStatsAnomalyDetectorAction extends BaseRestHandler { | ||
public class RestStatsAnomalyDetectorAction extends AbstractAnomalyDetectorAction { |
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 extend the AbstractAnomalyDetectorAction
instead of BaseExtensionRestHandler
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 like it gets that base class by inheritance, and adds access to several settings in the process that will get updated via settings update consumers.
However, later on in this class we're just fetching the setting directly and not taking advantage of the inheritance.
So either way is good but we should be consistent!
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.
Okay. I am replacing it with BaseExtensionRestHandler to maintain the consistency
*/ | ||
public class RestStatsAnomalyDetectorAction extends BaseRestHandler { | ||
public class RestStatsAnomalyDetectorAction extends AbstractAnomalyDetectorAction { |
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 like it gets that base class by inheritance, and adds access to several settings in the process that will get updated via settings update consumers.
However, later on in this class we're just fetching the setting directly and not taking advantage of the inheritance.
So either way is good but we should be consistent!
public class ADStatsNodesTransportAction extends | ||
TransportNodesAction<ADStatsRequest, ADStatsNodesResponse, ADStatsNodeRequest, ADStatsNodeResponse> { | ||
|
||
public class ADStatsNodesTransportAction extends TransportAction<ADStatsNodeRequest, ADStatsNodesResponse> { |
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.
What are we losing by removing this inheritance vs. replacing it with an SDK equivalent? This approach will require all the classes which inherit from this same superclass to have to make the same changes, while if you update (or replace) the superclass, then that's a one-time change that all the subclasses can use.
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 have to dig into this. I will post my findings 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.
When ADStatsNodesTransportAction is used in
-
Plugin Architecture: It extends TransportNodesAction. The transportNodesAction Accepts 4 parameters
(BaseNodesRequest, NodesResponse, NodeRequest, NodeResponse). It can perform Async operation on multiple nodes.
P.S.: NodeRequest extends TransportRequest -
Extension Architecture: It extends TransportAction. The transport action accepts 2 parameters (NodeRequest and NodesResponse). It can only perform request on only 1 node. Therefore in order to provide the functionality similar to TransportNodesAction I can open up a new issue.
As the TransportNodesAction counterpart for extensions needs more understanding and overall scope is beyond GetDetectorStats API.
What would you suggest? Shall I open a new issue?
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.
Yeah, this is probably worth a new issue. Summing it up:
TransportNodesAction
is on OpenSearch meant for plugins, so we can't/shouldn't change it for extensions, but- There are some methods that add some functionality to the inheritance from
TransportAction
, specifically thenewResponse
method and the abstract methods.
I'm not sure we need a SDKTransportNodesAction
but we should at least look into whether it makes sense.
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.
As discussed, I will be opening a new issue for 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.
Let's change the request to ADStatsRequest
Signed-off-by: Varun Jain <varunudr@amazon.com>
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.
Let's try to minimize the diff for migration by handling the new methods in SDK. This PR adds a lot of new changes. As part of migration we should focus on the above.
Also, get stats has these 2 APIs.
GET _plugins/_anomaly_detection/stats
GET _plugins/_anomaly_detection/<nodeId>/stats
.
Can you add checklist in your PR description that you tested both and add the response of the APIs in the issue on SDK repo like?
import org.opensearch.ad.transport.ADJobRunnerTransportAction; | ||
import org.opensearch.ad.transport.AnomalyDetectorJobAction; | ||
import org.opensearch.ad.transport.AnomalyDetectorJobTransportAction; | ||
import org.opensearch.ad.transport.*; |
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.
Eh. Wildcard import
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.
ACk
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.*; |
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.
Wildcard import
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.
ACK
@@ -230,4 +236,49 @@ public static boolean isProperExceptionToReturn(Throwable e) { | |||
private static String coalesceToEmpty(@Nullable String s) { | |||
return s == null ? "" : s; | |||
} | |||
|
|||
public static final String unrecognized(RestRequest request, Set<String> invalids, Set<String> candidates, String detail) { |
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 method was a part of BaseRestHandler
to reduce the diff and maintain the consistency we should add it in BaseExtensionRestHandler
if not present already and pull it from there.
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.
ACK
@@ -73,6 +74,9 @@ enum IndexCreation { | |||
@Mock | |||
protected IndexNameExpressionResolver indexNameResolver; | |||
|
|||
@Mock | |||
protected OpenSearchAsyncClient openSearchAsyncClient; |
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.
Here and above. I know it doesn't align with the variable naming but the wrapper is actually from java client which we created in SDK and for better understanding.
protected OpenSearchAsyncClient openSearchAsyncClient; | |
protected OpenSearchAsyncClient javaAsyncClient; |
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 agree with your suggestion.
src/main/java/org/opensearch/ad/transport/StatsAnomalyDetectorTransportAction.java
Show resolved
Hide resolved
List<FailedNodeException> failures | ||
) { | ||
return new ADStatsNodesResponse(clusterService.getClusterName(), responses, failures); | ||
public ADStatsNodesResponse newResponse(List<ADStatsNodeResponse> responses, List<FailedNodeException> failures) { |
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.
Let's not change the method arguments here because the tests would be affected. Old method had ADStatsRequest
as one of the param
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.
ACK
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.*; |
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.
Wildcard
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.
ACK
.orTimeout(AnomalyDetectorSettings.REQUEST_TIMEOUT.get(settings).getMillis(), TimeUnit.MILLISECONDS) | ||
.join(); | ||
|
||
logger.info("Stats Response is ********************** " + statsAnomalyDetectorResponse.toString()); |
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 don't have to log the response here. Leftover?
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.
yes. Sorry it was left by mistake. Will remove it in the new iteration.
if (adStatsNodeResponse != null) { | ||
responses.add(adStatsNodeResponse); | ||
} | ||
listener.onResponse(newResponse(responses, failures)); |
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.
Based on this we might won't need doExecute
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.
As of now, we need doExecute method as We have to cater all the aspects needed to implement the above referenced work.
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.
Let's add a TODO and link the issue 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.
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.
Done added in PR as well
|
||
/** | ||
* ADStatsNodeRequest to get a nodes stat | ||
*/ | ||
public class ADStatsNodeRequest extends TransportRequest { | ||
public class ADStatsNodeRequest extends ActionRequest { |
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 are we changing the request 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.
Because as per the changes made by dan in TransportRequest, it now accepts the request which extends the ActionRequest and response which extends ActionResponse
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.
ActionRequest
extends TransportRequest
so this doesn't break anything but does add some functionality relying on ActionRequest
signature. Is this being triggered remotely from JS, etc. and that's why we need the ActionRequest
?
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.
No this request is not being triggered from JS
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.
Same question as Owais, if its not necessary to change then lets revert this change
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.
As suggested by @dbwiddis I will open a new issue where we will cater the blast radius of TransportNodesAction and will do the required work there.
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 you get any error if you keep the request as TransportRequest
?
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.
yes I got an error, TransportAction only accepts the requests which extends ActionRequest.
Signed-off-by: Varun Jain <varunudr@amazon.com>
Signed-off-by: Varun Jain <varunudr@amazon.com>
Signed-off-by: Varun Jain <varunudr@amazon.com>
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.
LGTM generally. Will review again after @owaiskazi19's comments are addressed.
public String getName() { | ||
return STATS_ANOMALY_DETECTOR_ACTION; | ||
} | ||
|
||
@Override | ||
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { | ||
public List<RouteHandler> routeHandlers() { |
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 implementation works for now and no need to hold up this PR. But heads up that with SDK #665 we'll be able to go back to the replaced routs format of existing rest handler classes. So stay tuned!
|
||
/** | ||
* ADStatsNodeRequest to get a nodes stat | ||
*/ | ||
public class ADStatsNodeRequest extends TransportRequest { | ||
public class ADStatsNodeRequest extends ActionRequest { |
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.
ActionRequest
extends TransportRequest
so this doesn't break anything but does add some functionality relying on ActionRequest
signature. Is this being triggered remotely from JS, etc. and that's why we need the ActionRequest
?
.toXContent(JsonXContent.contentBuilder(), ToXContent.EMPTY_PARAMS); | ||
ExtensionRestResponse response = new ExtensionRestResponse(request, RestStatus.OK, statsAnomalyDetectorResponseBuilder); | ||
|
||
return response; |
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.
(Style) Why not just return new ExtensionRestResponse(...)
here instead of creating a new object and returning it on the next line? Yeah, the compiler optimizes this out, just reads better to me. :)
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.
ACK
@@ -94,6 +94,8 @@ public static String getTooManyCategoricalFieldErr(int limit) { | |||
public static String FAIL_TO_GET_STATS = "Fail to get stats"; | |||
public static String FAIL_TO_SEARCH = "Fail to search"; | |||
|
|||
public static String FAIL_TO_GET_NODE_STATS = "Fail to get node stats"; |
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.
Nit 1. I know you're coping from the previous error messages on lines 85-93, but this should be "failed" rather than "fail". (As should all the rest.)
Nit 2. All the others are together without whitespace in between them, this one isn't special. Save that CR byte from the file size!
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.
ACK
public class ADStatsNodesTransportAction extends | ||
TransportNodesAction<ADStatsRequest, ADStatsNodesResponse, ADStatsNodeRequest, ADStatsNodeResponse> { | ||
|
||
public class ADStatsNodesTransportAction extends TransportAction<ADStatsNodeRequest, ADStatsNodesResponse> { |
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.
Yeah, this is probably worth a new issue. Summing it up:
TransportNodesAction
is on OpenSearch meant for plugins, so we can't/shouldn't change it for extensions, but- There are some methods that add some functionality to the inheritance from
TransportAction
, specifically thenewResponse
method and the abstract methods.
I'm not sure we need a SDKTransportNodesAction
but we should at least look into whether it makes sense.
Signed-off-by: Varun Jain <varunudr@amazon.com>
curl -X GET "localhost:9200/_extensions/_ad-extension-1/stats/"
|
curl -X GET "localhost:9200/_extensions/_ad-extension-1/stats/ad_hc_execute_failure_count"
|
curl -X GET "localhost:9200/_extensions/_ad-extension-1/_ad-extension-1/stats/ad_hc_execute_failure_count"
|
curl -X GET "localhost:9200/_extensions/_ad-extension-1/_ad-extension-1/stats"
|
Signed-off-by: Varun Jain <varunudr@amazon.com>
@vibrantvarun I am curious to see the behavior once a detector is created and started. You can refer to this guide on how to generate test data and how to create/run a detector. Edit : I realized you wont be able to test out start detector until this PR is merged in. Let's just test out the behavior after creating single and multi-entity detectors for now |
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.
LGTM. I see some comments from @owaiskazi19 that should be answered.
Response of detector stats after triggering create detector api
|
@vibrantvarun can you add the response and API in the issue like I mentioned above? |
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.
Almost there. Plus the comments above and this. Thanks
|
||
/** | ||
* ADStatsNodeRequest to get a nodes stat | ||
*/ | ||
public class ADStatsNodeRequest extends TransportRequest { | ||
public class ADStatsNodeRequest extends ActionRequest { |
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 you get any error if you keep the request as TransportRequest
?
if (adStatsNodeResponse != null) { | ||
responses.add(adStatsNodeResponse); | ||
} | ||
listener.onResponse(newResponse(responses, failures)); |
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.
Let's add a TODO and link the issue here.
|
||
public static final String NODE_ID = "nodeId"; | ||
|
||
public static final String STAT = "stat"; |
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 generally add the constant for rest handler URI but it's fine to add for request param as well. Can you remove the extra lines 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.
ACK
// SDKClusterService.SDKClusterSettings clusterSettings = sdkClusterService.new SDKClusterSettings( | ||
// Settings.EMPTY, Collections.unmodifiableSet(new HashSet<>(Arrays.asList(MAX_MODEL_SIZE_PER_NODE))) | ||
// ); | ||
// when(sdkClusterService.getClusterSettings()).thenReturn(clusterSettings); |
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.
Do we need this commented code here anymore?
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.
ACK
@@ -73,6 +74,9 @@ enum IndexCreation { | |||
@Mock | |||
protected IndexNameExpressionResolver indexNameResolver; | |||
|
|||
@Mock | |||
protected OpenSearchAsyncClient javaAsyncClient; |
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.
Can you mock the client in setUp
like:
sdkJavaAsyncClient = mock(OpenSearchAsyncClient.class);
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.
ACk
Signed-off-by: Varun Jain <varunudr@amazon.com>
Signed-off-by: Varun Jain <varunudr@amazon.com>
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 much cleaner now. Few small suggestions
Set<String> validStats = adStats.getStats().keySet(); | ||
|
||
ADStatsRequest adStatsRequest = null; | ||
ADStatsRequest adStatsRequest; |
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 can initialize with null
|
||
import com.google.inject.Inject; | ||
|
||
// TODO: https://github.com/opensearch-project/opensearch-sdk-java/issues/683 (multi node support for extensions) |
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.
Can you add more details on why multi node support is needed here?
@@ -12,8 +12,7 @@ | |||
package org.opensearch.ad.transport.handler; | |||
|
|||
import static org.mockito.ArgumentMatchers.any; | |||
import static org.mockito.Mockito.doAnswer; | |||
import static org.mockito.Mockito.when; | |||
import static org.mockito.Mockito.*; |
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.
Wildcard import :(
@@ -73,6 +73,9 @@ enum IndexCreation { | |||
@Mock | |||
protected IndexNameExpressionResolver indexNameResolver; | |||
|
|||
@Mock | |||
protected static OpenSearchAsyncClient sdkJavaAsyncClient; |
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 are mocking it already below. No need to mock here. Just move this after line 60 and say:
protected OpenSearchAsyncClient sdkJavaAsyncClient;
Signed-off-by: Varun Jain <varunudr@amazon.com>
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.
LGTM! Thanks for addressing the feedback
Description
Get Detector Stats API
The api's Provides information about how the plugin is performing.
Issues Resolved
opensearch-project/opensearch-sdk-java#382
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.