Get detector #207

VijayanB · 2020-08-07T20:12:19Z

Cat command to concatenate and print detectors
Added get method to get detector configuration

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

codecov · 2020-08-07T20:13:54Z

Codecov Report

Merging #207 into master will increase coverage by 0.19%.
The diff coverage is 80.41%.

@@             Coverage Diff              @@
##             master     #207      +/-   ##
============================================
+ Coverage     72.21%   72.41%   +0.19%     
- Complexity     1289     1290       +1     
============================================
  Files           139      139              
  Lines          5979     6073      +94     
  Branches        469      469              
============================================
+ Hits           4318     4398      +80     
- Misses         1457     1464       +7     
- Partials        204      211       +7

Flag	Coverage Δ	Complexity Δ
#cli	`80.30% <80.41%> (+0.56%)`	`0.00 <0.00> (ø)`
#plugin	`71.36% <ø> (+0.01%)`	`1290.00 <ø> (+1.00)`

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

Impacted Files	Coverage Δ	Complexity Δ
cli/internal/gateway/ad/ad.go	`65.38% <64.70%> (-0.14%)`	`0.00 <0.00> (ø)`
cli/internal/controller/ad/ad.go	`77.38% <67.74%> (+<0.01%)`	`0.00 <0.00> (ø)`
cli/internal/mapper/ad/ad.go	`86.42% <90.90%> (+1.88%)`	`0.00 <0.00> (ø)`
cli/internal/handler/ad/ad.go	`90.09% <100.00%> (+1.66%)`	`0.00 <0.00> (ø)`
...sticsearch/ad/indices/AnomalyDetectionIndices.java	`62.31% <0.00%> (+0.72%)`	`24.00% <0.00%> (+1.00%)`

kaituo · 2020-08-11T20:00:16Z

cli/internal/gateway/ad/ad.go

@@ -59,6 +61,56 @@ func (g *gateway) buildCreateURL() (*url.URL, error) {
 	return endpoint, nil
 }

+// CreateDetector Creates an anomaly detector job.


Do you mean to remove this line? In go, if a method is exported, it is practice to write comment else linter will complain. At line 114, i have a method CreateDetector, hence the comment.

my bad. I thought the following is a commented out code block. Does Go has block comment?

Yeah. I updated with block comments.

kaituo · 2020-08-11T20:04:04Z

cli/cmd/cat.go

+}
+
+//executeByNames gets detector output based on name as argument
+func executeByNames(commandHandler *ad.Handler, args []string) []*entity.DetectorOutput {


The code looks same as execyteByID. Am I missing anything?

yes.
execute by name calls method ad.GetAnomalyDetectorsByNamePattern which returns []*entity.DetectorOutput, while
executeby id calls method ad.GetAnomalyDetectorsByID which returns *entity.DetectorOutput. Thats way i am not able to pass function as parameter. In go, type has to be matched.

Does it make sense to pass a boolean or enum say "by field". And then you call different functions based on this by field?

Sure. I wrapped the method for executeByID and used flags to differentiate action .

cli/internal/entity/ad/ad.go

ylwu-amzn · 2020-08-14T22:16:42Z

cli/cmd/cat.go

+	},
+}
+
+func getDetectors(commandHandler *ad.Handler, args []string, get func(*ad.Handler, string) ([]*entity.DetectorOutput, error)) []*entity.DetectorOutput {


This line seems very long. How about breaking it into two lines?

Ack. Changed

ylwu-amzn · 2020-08-14T22:18:39Z

cli/cmd/cat.go

+}
+
+//executeByID gets detector output based on ID as argument
+func executeByID(commandHandler *ad.Handler, ID string) ([]*entity.DetectorOutput, error) {


This method is not to execute a detector by ID. How about rename it as "getDetectorByID"

Ack. Changed

ylwu-amzn · 2020-08-14T22:22:10Z

cli/cmd/cat.go

+		if idStatus {
+			action = executeByID
+		}
+		if results := getDetectors(commandHandler, args, action); results != nil {


If results is nil, what message will user see?

So only possibility getDetectors can get nil is if there is an error from GET method. In that case it will display the error message ( why get failed ). This was handled inside getDetectors. I refactored now to return err and print outside now.

only possibility getDetectors can get nil is if there is an error from GET method

If user input an invalid detector id, will throw an error or return empty results ? In print method for _, d := range results {, if results is empty, will not print anything?

There is will be either json output or error message per detector.

ylwu-amzn · 2020-08-14T22:26:11Z

cli/cmd/cat.go

+	if results == nil {
+		return
+	}
+	for _, d := range results {


Is it best practice to use single-letter variable name in Go? How about replace d with detector ? That will be easier to read the code.

https://github.com/golang/go/wiki/CodeReviewComments#variable-names
The convention is you only need more descriptive if it is used far away from declaration. Especially for iterators, single variable name is preferred.

ylwu-amzn · 2020-08-14T22:36:41Z

cli/cmd/cat.go

+
+func init() {
+	esadCmd.AddCommand(catCmd)
+	catCmd.Flags().BoolP("name", "", true, "Input is name or pattern")


The message "Input is name or pattern" is visible to user? Have you confirmed the wording with tech writer? I feel we should say Input is detector name or name pattern, Input is detector id

As discussed offline, i will fix this accordingly.

Sure, will approve this PR. Please fix this in another PR.

ylwu-amzn · 2020-08-14T22:39:48Z

cli/cmd/cat.go

+		if idStatus {
+			action = executeByID
+		}
+		if results := getDetectors(commandHandler, args, action); results != nil {


only possibility getDetectors can get nil is if there is an error from GET method

If user input an invalid detector id, will throw an error or return empty results ? In print method for _, d := range results {, if results is empty, will not print anything?

ylwu-amzn · 2020-08-14T23:38:57Z

cli/internal/controller/ad/ad.go

+//GetDetectorsByName get detector based on name pattern. It first calls SearchDetectorByName and then
+// gets lists of detectorId and call GetDetector to get individual detector configuration
+func (c controller) GetDetectorsByName(ctx context.Context, pattern string, display bool) ([]*entity.DetectorOutput, error) {
+	matchedDetectors, err := c.getDetectorsToProcess(ctx, "fetch", pattern)


What use cases have you tested for the name pattern? SearchDetectorByName method is using match query. Why not use regexp ?

payload := entity.SearchRequest{ Query: entity.SearchQuery{ Match: entity.Match{ Name: name, }, }, }

I followed this from documentation:
https://opendistro.github.io/for-elasticsearch-docs/docs/ad/api/#search-detector

Can you do some test for different regex use cases? Like "test.*", "test[0-9]" etc.

I will create an issue to update search detecter using regex and will submit new PR with test cases.

https://github.com/opendistro-for-elasticsearch/anomaly-detection/issues/213

Every exported go method should have comments for readability and usage. Add appropriate commands and sample data is copied from https://opendistro.github.io/for-elasticsearch-docs/docs/ad/api

Added function and test to check get response and regenerated mocks as well

Used by GetDetectorController to deserialize response from gateway to type.

Mapper to convert gateway response to structure.

GetDetector will call gateway to get configuration GetDetectorByName will first call SearchDetector to get ID from Name pattern. Later, it will call GetDetector to get details.

Handler for cat command, this will get list of detector based on id and name pattern

Created command to accept list of detectors to concatenate and print detectors. This is similar to cat command in unix.

VijayanB force-pushed the get-detector branch from 8c6c22a to 892f830 Compare August 7, 2020 20:28

VijayanB requested review from wnbts, kaituo and ylwu-amzn August 7, 2020 20:30

VijayanB force-pushed the get-detector branch from 892f830 to 092db6b Compare August 8, 2020 22:40

VijayanB self-assigned this Aug 8, 2020

VijayanB added the enhancement New feature or request label Aug 8, 2020

kaituo reviewed Aug 11, 2020

View reviewed changes

VijayanB requested a review from kaituo August 12, 2020 20:55

VijayanB force-pushed the get-detector branch 2 times, most recently from 05c5426 to 88e6869 Compare August 12, 2020 21:32

kaituo approved these changes Aug 12, 2020

View reviewed changes

ylwu-amzn reviewed Aug 14, 2020

View reviewed changes

VijayanB force-pushed the get-detector branch 2 times, most recently from 98101f2 to 37f4e4c Compare August 14, 2020 22:44

ylwu-amzn reviewed Aug 14, 2020

View reviewed changes

ylwu-amzn approved these changes Aug 19, 2020

View reviewed changes

VijayanB added 7 commits August 20, 2020 11:32

Include comments for gateway methods

ed28a5f

Every exported go method should have comments for readability and usage. Add appropriate commands and sample data is copied from https://opendistro.github.io/for-elasticsearch-docs/docs/ad/api

Add GetDetector Gateway

dddc3bb

Added function and test to check get response and regenerated mocks as well

Entity: Added entity to deserialize response

0746fec

Used by GetDetectorController to deserialize response from gateway to type.

Mapper: function to map from json to structure

9a43f92

Mapper to convert gateway response to structure.

Controller: Get Detector based on Id and Name

8086177

GetDetector will call gateway to get configuration GetDetectorByName will first call SearchDetector to get ID from Name pattern. Later, it will call GetDetector to get details.

Handler: GetDetector handler to call controller

6964ea0

Handler for cat command, this will get list of detector based on id and name pattern

Command: Cat command to concatenate and print detectors

1c790a2

Created command to accept list of detectors to concatenate and print detectors. This is similar to cat command in unix.

VijayanB force-pushed the get-detector branch from 37f4e4c to 1c790a2 Compare August 20, 2020 18:52

VijayanB merged commit 4b52e24 into opendistro-for-elasticsearch:master Aug 20, 2020

VijayanB added feature new feature and removed enhancement New feature or request labels Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get detector #207

Get detector #207

VijayanB commented Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

Loading

kaituo Aug 11, 2020

VijayanB Aug 11, 2020

kaituo Aug 12, 2020

VijayanB Aug 12, 2020

kaituo Aug 11, 2020

VijayanB Aug 11, 2020 •

edited

Loading

kaituo Aug 12, 2020

VijayanB Aug 12, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 19, 2020

ylwu-amzn Aug 14, 2020

ylwu-amzn Aug 14, 2020

VijayanB Aug 14, 2020

ylwu-amzn Aug 17, 2020

VijayanB Aug 20, 2020

VijayanB Aug 20, 2020

Get detector #207

Get detector #207

Conversation

VijayanB commented Aug 7, 2020

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VijayanB Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2020 •

edited

Loading

VijayanB Aug 11, 2020 •

edited

Loading