Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adding new Search Detector Info API #286

Merged
merged 5 commits into from
Oct 22, 2020
Merged

Adding new Search Detector Info API #286

merged 5 commits into from
Oct 22, 2020

Conversation

saratvemulapalli
Copy link
Contributor

@saratvemulapalli saratvemulapalli commented Oct 22, 2020

*Issue #195 *

Description of changes:
Adding new Search Detector Info API to query total number of detectors and match name with existing detectors.
This API will search across all the detectors in detector index irrespective of User context.

Count Request:
GET _opendistro/_anomaly_detection/detectors/count
Response:
{ "count" : 4, "match" : false }

Match Request:
GET _opendistro/_anomaly_detection/detectors/match?name=opendistro_ad
Response:
{ "count" : 0, "match" : false }

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #286 into master will increase coverage by 1.20%.
The diff coverage is 76.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #286      +/-   ##
============================================
+ Coverage     70.15%   71.36%   +1.20%     
- Complexity     1821     1871      +50     
============================================
  Files           190      195       +5     
  Lines          8900     9020     +120     
  Branches        757      764       +7     
============================================
+ Hits           6244     6437     +193     
+ Misses         2300     2218      -82     
- Partials        356      365       +9     
Flag Coverage Δ Complexity Δ
#cli 79.27% <ø> (ø) 0.00 <ø> (ø)
#plugin 70.67% <76.04%> (+1.32%) 1871.00 <22.00> (+50.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...stroforelasticsearch/ad/util/RestHandlerUtils.java 86.84% <ø> (ø) 14.00 <0.00> (ø)
...h/ad/rest/RestSearchAnomalyDetectorInfoAction.java 50.00% <50.00%> (ø) 4.00 <4.00> (?)
...port/SearchAnomalyDetectorInfoTransportAction.java 63.63% <63.63%> (ø) 4.00 <4.00> (?)
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java 93.44% <100.00%> (+0.05%) 13.00 <0.00> (ø)
.../ad/transport/SearchAnomalyDetectorInfoAction.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...ad/transport/SearchAnomalyDetectorInfoRequest.java 100.00% <100.00%> (ø) 6.00 <6.00> (?)
...d/transport/SearchAnomalyDetectorInfoResponse.java 100.00% <100.00%> (ø) 6.00 <6.00> (?)
...asticsearch/ad/cluster/ADClusterEventListener.java 88.00% <0.00%> (-4.00%) 13.00% <0.00%> (-1.00%)
...ransport/DeleteAnomalyDetectorTransportAction.java 57.95% <0.00%> (-3.41%) 16.00% <0.00%> (-1.00%)
...earch/ad/stats/suppliers/ModelsOnNodeSupplier.java 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
... and 12 more

Comment on lines 96 to 101
boolean nameExists = false;
if (searchResponse.getHits().getTotalHits().value > 0) {
nameExists = true;
} else {
nameExists = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: could condense this into one line: boolean nameExists = searchResponse.getHits().getTotalHits().value > 0

Copy link
Contributor Author

@saratvemulapalli saratvemulapalli Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thats a nice idea.

@ohltyler
Copy link
Contributor

Have you discussed with the tech writer about adding these to the AD Open Distro docs?

@saratvemulapalli
Copy link
Contributor Author

Have you discussed with the tech writer about adding these to the AD Open Distro docs?

Good question, I will work with the tech writer as we need to update bunch of other information as well for FGAC.

@ohltyler
Copy link
Contributor

Have you discussed with the tech writer about adding these to the AD Open Distro docs?

Good question, I will work with the tech writer as we need to update bunch of other information as well for FGAC.

Cool, don't see any other blockers, will approve.

@ohltyler
Copy link
Contributor

One more edge case here - in the case of the .opendistro-anomaly-detectors index not existing yet, can we return 0 here rather than an error? Tested in Kibana dev console

import org.elasticsearch.action.ActionType;

public class SearchAnomalyDetectorInfoAction extends ActionType<SearchAnomalyDetectorInfoResponse> {
public static final SearchAnomalyDetectorInfoAction INSTANCE = new SearchAnomalyDetectorInfoAction();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you switched NAME and INSTANCE sequence in this PR #284 , why we need to do that? Should we do the same thing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure makes sense.
I''ll rebase my changes and update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw the first part of the question, JVM is not smart enough, since INSTANCE depends on ACTION it must be defined first and if defined later string concat does not work. So I had to move it above.


public class SearchAnomalyDetectorInfoAction extends ActionType<SearchAnomalyDetectorInfoResponse> {
public static final SearchAnomalyDetectorInfoAction INSTANCE = new SearchAnomalyDetectorInfoAction();
public static final String NAME = "cluster:admin/opendistro/ad/detector/info";
Copy link
Contributor

@ylwu-amzn ylwu-amzn Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see PR #284 merged, change to use CommonValue.EXTERNAL_ACTION_PREFIX ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will take care of it.

@saratvemulapalli
Copy link
Contributor Author

One more edge case here - in the case of the .opendistro-anomaly-detectors index not existing yet, can we return 0 here rather than an error? Tested in Kibana dev console

Sure had a chat offline, it makes sense to return 0, false when index doesnt exist.

Copy link
Contributor

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the fix. LGTM

@saratvemulapalli saratvemulapalli merged commit 6138f78 into master Oct 22, 2020
@saratvemulapalli saratvemulapalli deleted the fgac-info-api branch October 22, 2020 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants