AD CLI #196

VijayanB · 2020-07-27T20:35:03Z

Merged esad from private repo to anomaly detection plugin.

Issue #, if available:

Description of changes:

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

Merged esad from private repo to anomaly detection plugin.

ylwu-amzn · 2020-07-27T21:44:59Z

Do we plan to use same repo for AD CLI tool?

VijayanB · 2020-07-27T23:24:37Z

Do we plan to use same repo for AD CLI tool?

Yeah. Recently, SQL CLI is also merged with sql repo.

Added build and publish artifacts

wnbts

question: why is the go programming language used for cli? if the programming language needs to be different from the rest of the project code, why not python for the equivalent implementation could be simpler?

wnbts · 2020-07-29T00:20:11Z

cli/README.md

@@ -0,0 +1,97 @@
+![Test Workflow](https://github.com/VijayanB/esad/workflows/Build%20and%20Test%20Anomaly%20detection%20commandline%20tool/badge.svg)


minor. is this expected?

Thanks for pointing it. I removed it for now.

wnbts · 2020-07-29T00:20:49Z

cli/README.md

+# Open Distro for Elasticsearch AD CLI
+
+The AD CLI component in Open Distro for Elasticsearch (ODFE) is a command line interface for ODFE AD plugin.
+his CLI provides greater flexibility of use. User can use CLI to easily do things that are difficult or sometimes impossible to do with kibana UI. This doesn’t use any additional  system resources to load any of graphical part, thus making it simpler and faster than UI. 


minor. his should be This

Thanks for pointing it. I updated it.

wnbts · 2020-07-29T00:23:49Z

cli/README.md

+
+## Basic Commands
+
+AN ESAD CLI has following structure


minor. AN should be An

Thanks for pointing it. I updated it in next commit.

wnbts · 2020-07-29T00:40:06Z

cli/cmd/create.go

+}
+
+func createDetectors(fileNames []string, status bool) error {
+	h, err := getCommandHandler()


suggestion. for readability, refrain from using single letters for variable names.

i updated it. Nevertheless, accordingly to go style, (https://github.com/golang/go/wiki/CodeReviewComments#variable-names) it is preferred to use short variable name if it is not used below a page down .

wnbts · 2020-07-29T00:45:27Z

cli/cmd/profile.go

+           When you specify a profile to run a command, the settings and credentials are used to run that command. 
+           You can specify a profile in an environment variable (ESAD_PROFILE) which essentially acts as the default profile for commands if default doesn't exists.
+           The ESAD CLI supports using any of multiple named profiles that are stored in the config and credentials files.`,
+	//Run: func(cmd *cobra.Command, args []string) {


minor. unused code can be deleted in multiple places.

I deleted the commented code. Thanks for pointing out.

wnbts · 2020-07-29T00:52:22Z

cli/cmd/profile.go

+	}
+}
+
+func validProfileName(name string) bool {


minor. the name should be existingprofilename as existing name is not valid for creation.

i updated to isProfileExists since it returns bool. But i renamed variable names like you suggested in other place. Can you review it?

wnbts · 2020-07-29T01:05:31Z

cli/cmd/profile.go

+	}
+
+	var safeProfiles []entity.Profile
+	profiles := getProfiles()


minor. retrieving profiles is repeated and can use refactoring.

logic to get profiles (load file, deserialization ) is a single method, just that it is being used in multiple use cases. In this particular case, i had split logic to delete actual profile and verify whether the profile is valid or not . This will make logic easy to maintain than having both use cases entangled.

wnbts · 2020-07-29T01:07:19Z

cli/cmd/profile.go

+	profiles := getProfiles()
+	for _, p := range profiles {
+		safe := true
+		for _, name := range validProfiles {


question. does go have libraries for data structures other than array?

unfortunately no. It has map and slice(array). Usually if there is need for processing lot of data, go recommends concurrent programming by spanning multiple threads. My guess is since go is not meant for data processing and keep language simple and opinionated, it doesn't have data structures like java.

Fixed Code review comments

Update publish only tags are created.

VijayanB · 2020-07-29T20:36:56Z

question: why is the go programming language used for cli? if the programming language needs to be different from the rest of the project code, why not python for the equivalent implementation could be simpler?

I believe, there is a plan to have one cli in long run and at the same time we saw sql cli/ sql odbc individual repos are being integrated into their plugin repo to have single release and simplify maintenance and engage community in the mean time.
Go is being used as modern programming language for latest cli tools like kubectl ( mostly opensource cli ). It has advantages like no runtime is required for the clients to install and use, it is strongly typed like java which is developer friendly and has very good community support.

wnbts · 2020-07-30T01:50:24Z

question: why is the go programming language used for cli? if the programming language needs to be different from the rest of the project code, why not python for the equivalent implementation could be simpler?

I believe, there is a plan to have one cli in long run and at the same time we saw sql cli/ sql odbc individual repos are being integrated into their plugin repo to have single release and simplify maintenance and engage community in the mean time.
Go is being used as modern programming language for latest cli tools like kubectl ( mostly opensource cli ). It has advantages like no runtime is required for the clients to install and use, it is strongly typed like java which is developer friendly and has very good community support.

thanks for the reasonable answers.

I am neither for nor against the decision. Just for the record, aws cli is based on python. Also, for an open source project, a more popular/familiar programming language might invite more contributions.

wnbts · 2020-07-30T01:53:42Z

cli/internal/controller/ad/ad_test.go

+const mockDetectorName = "detector"
+
+func helperLoadBytes(t *testing.T, name string) []byte {
+	path := filepath.Join("testdata", name) // relative path


question. is it also a go convention to not have a separate directory/module for test code & data?

you mean test file and implementation? Yes, go uses package oriented design, hence code/test/testdata should always be close to each other. There is no guideline but i observed this being used in many of application tool
like gofmt / https://golang.org/src/cmd/gofmt/

codecov · 2020-07-30T02:23:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@68429a8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #196   +/-   ##
=========================================
  Coverage          ?   79.74%           
=========================================
  Files             ?        6           
  Lines             ?      622           
  Branches          ?        0           
=========================================
  Hits              ?      496           
  Misses            ?       77           
  Partials          ?       49

Flag	Coverage Δ
#unittests	`79.74% <0.00%> (?)`

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68429a8...7bea3e6. Read the comment docs.

wnbts

suggestion. making pr smaller helps reviewers.

wnbts · 2020-07-31T00:01:00Z

cli/README.md

+
+It only supports [Open Distro for Elasticsearch (ODFE) AD Plugin](https://opendistro.github.io/for-elasticsearch-docs/docs/ad/)
+You must have the ODFE AD plugin installed to your Elasticsearch instance to connect. 
+Users can run this CLI from MacOS and Linux, and connect to any valid Elasticsearch end-point such as Amazon Elasticsearch Service (AES).The ESAD CLI implements AD APIs.


minor. MacOS and Linux and Windows

Ack.

suggestion. making pr smaller helps reviewers.

Definitely, i started as proof of concept and later i added features. I will definitely create small commits and multiple PR to make sure that code is easy to review .

Forgot to add "Windows" or this CLI can't support windows?

wnbts · 2020-07-31T00:36:38Z

cli/cmd/stop.go

+		if idStatus {
+			action = ad.StopAnomalyDetectorByID
+		}
+		err := execute(action, args)


question. why execute is not needed here but needed in start?

Go's package scope allows us to refer to the method outside file as long as it is inside same package. I created one file per command but start and stop shares lot. Now i merged both the command in single file to keep code together.

wnbts · 2020-07-31T00:46:31Z

cli/internal/controller/ad/ad.go

+		if bar != nil {
+			bar.Incr()


minor. should increment be done after an operation is finished?

Yeah. I updated it. Thanks for pointing it.

wnbts · 2020-07-31T00:57:28Z

cli/internal/controller/ad/ad.go

+		res, err := c.gateway.StopDetector(ctx, id) // ignore error
+		if err != nil {
+			return err


issue. the error looks like not ignored.

Removed the comment. Initially i was ignoring but later decided to respect and return the same

wnbts · 2020-07-31T01:02:50Z

cli/internal/controller/ad/ad.go

+	if err != nil {
+		log.Fatal(err)
+	}


issue. is it safe to continue with a fatal error?

You are right. It is not. I updated it.

wnbts · 2020-07-31T01:11:36Z

cli/internal/controller/ad/ad.go

+	if len(detectors) < 1 {
+		return
+	}
+	var deleted []entity.Detector


minor. this name should be undeleted.

You are right. I fixed it now.

ylwu-amzn · 2020-08-04T00:33:34Z

cli/README.md

+- endpoint: https://odfe-node1:9200
+  username: admin
+  password: foobar
+  name: dev


What will happen if I use same name for two profiles. Will throw error or override the prior one?

There is no validation if file is specified by user. We only have validation if user is creating a profile. Let's say if user has two profile with same name and this config is passed as parameter to the command, it will not override instead, it will select the first profile which matches the name.

Sure, so we have error handling for this case. How about add this in readme doc and the help doc?

Added it in readme.

ylwu-amzn · 2020-08-04T00:37:56Z

cli/README.md

+```
+For example to start detector:
+```
+$ esad start [detector-name-pattern]


Seems the commands not follow same name convention. esad profile create looks like esad <resource> <action>, but here esad start [detector-name-pattern] looks like esad <action> <resource>. Is it possible to follow same name convention? Maybe consult Carlos to confirm.

Actually, esad, profile, create, start are commands. You can think like tree structure.
esad -> profile -> create
-> delete
-> Create
-> Start
Anyway, i asked carlos to provide feedback. If i didn't receive anything related to that, i will bring this point.
Thanks for feedback.

ylwu-amzn · 2020-08-04T00:39:50Z

cli/README.md

+```
+To get the version of the ESAD CLI:
+```
+$ esad --version


How about adding version compatibility table in the help doc and read-me doc?

Sure. I will add it.

ylwu-amzn · 2020-08-04T16:42:15Z

cli/internal/gateway/es/es.go

+	return endpoint, nil
+}
+
+func (g *gateway) Search(ctx context.Context, index string, field string) ([]byte, error) {


This method is to get distinct value of one field. How about we rename this method as "SearchDistinctValue"? So I don't need to read the method implementation to learn this.

ylwu-amzn · 2020-08-04T16:55:12Z

cli/internal/controller/es/controller.go

+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+


What's the name convention for the file name? I see this ES controller named as "controller.go", but AD controller named as "ad.go". And I see five "ad.go" files in these folders: entity, mapper, gateway, handler, controller. Is it common practice in Go to use same file name and differentiate them with package path?

I renamed to es.go. In Go, file name is not as vital as package name. I have seen in most of the popular go repositories, they use file name as same as package name. Yes, package is considered as module than file.

ylwu-amzn · 2020-08-04T17:01:16Z

cli/internal/controller/ad/ad.go

+	}
+}
+
+func (c controller) SearchDetectorByName(ctx context.Context, name string) ([]entity.Detector, error) {


Add comments for public methods ?

ylwu-amzn · 2020-08-04T17:07:34Z

cli/internal/controller/ad/ad.go

+	return nil
+}
+
+func (c controller) StopDetectorByName(ctx context.Context, pattern string, display bool) error {


minor: how about move StartDetectorByName method before StopDetectorByName ? This can improve a little bit code readability.

ylwu-amzn · 2020-08-04T17:09:52Z

cli/internal/controller/ad/ad.go

+	return c.processDetectorByAction(ctx, pattern, "stop", c.StopDetector, display)
+}
+
+//DeleteDetector deletes detector


minor: this simple comment looks like some duplicate of the method name "DeleteDetector". How about add more details like what we will do for force is true/false? Suggest to check other places too.

ylwu-amzn · 2020-08-04T17:16:45Z

cli/internal/controller/es/controller.go

+//go:generate mockgen -destination=mocks/mock_ad.go -package=mocks . Controller
+
+//Controller is an interface for ES Cluster to get distinct values
+type Controller interface {


Rename it as EsController considering we have AnomalyDetectorController?

From Go Wiki:
All references to names in your package will be done using the package name, so you can omit that name from the identifiers. For example, if you are in package chubby, you don't need type ChubbyFile, which clients will write as chubby.ChubbyFile. Instead, name the type File, which clients will write as chubby.File.

I renamed adController to Controller to keep consistent.

ylwu-amzn · 2020-08-04T17:20:14Z

cli/internal/gateway/ad/ad.go

+)
+
+const (
+	baseURL           = "_opendistro/_anomaly_detection/detectors"


I think we should write const in uppercase letters.

Unfortunately in GO, it is recommended to write like this. If const will be exported, then we can capitalize the first character.

Sure, interesting way to use naming convention to control access.

ylwu-amzn · 2020-08-04T17:27:40Z

cli/README.md

+1. Install from source:
+
+    ```
+    $ go get github.com/VijayanB/esad/


Should we change the repo ?

Thanks for pointing it out.

ylwu-amzn · 2020-08-04T17:38:27Z

cli/internal/handler/ad/ad.go

+	return h.CreateAnomalyDetector(fileName, interactive)
+}
+
+//GenerateAnomalyDetector creates detector based on file configurations


Wrong comments?

1) Move methods close to help readability 2) Add comments to exported methods 3) Fix typo and copied comments 4) Rename file name

ylwu-amzn · 2020-08-04T19:38:05Z

cli/cmd/profile.go

+		if !isProfileExists(name) {
+			break
+		}
+		fmt.Println("profile", name, "already exists.")


How about print all existing profile names as well? So user don't need to guess which name is not used.

They can use list command to get profile names. Do you still think that providing all names will be good for UX?

Cool. Let's keep it as is.

ylwu-amzn · 2020-08-04T19:42:49Z

cli/cmd/profile.go

+	} else {
+		response = getUserInputAsText()
+	}
+	if len(response) < 1 {


How about we abstract this as validate func and pass it into getUserInput ? So it's flexible to support more parameter's validation logic in future. As currently we have only one simple validation, don't need to make it too complicated currently. Add some todo is fine. It's up to you.

ylwu-amzn · 2020-08-04T19:47:45Z

cli/cmd/profile.go

+func deleteProfiles(names []string) {
+
+	var existingProfileNames []string
+	for _, name := range names {


Not so efficient to run isProfileExists for every profile name, which will getProfile and do iteration to check if name exists. How about read all profiles, construct map (key: profile name, value: profile), then remove deleted profiles ?

ylwu-amzn · 2020-08-04T19:51:24Z

cli/cmd/start_stop.go

+	commandStop  = "stop"
+)
+
+// createCmd represents the create command


Wrong comment?

Yeah. I removed it.

Yeah. I removed it now.

ylwu-amzn · 2020-08-04T19:52:25Z

cli/cmd/start_stop.go

+	},
+}
+
+var stopCmd = &cobra.Command{


Missing comment ? How about add comments for every command?

Typically in go, comment is required for exported variables or functions. Here we are not exporting stopCmd. What do you think?

Comments can help others include ourselves to understand code quickly. I think we should add some basic description for these commands like what the command does, what's the input/output, potential exceptions. There are already some description for this command for line 47-48. If you don't like too much comment, how about make line48 more accurate as "Stops detectors based on detector id or detector name pattern, ...."

Got it. I added comments and updated help as well.

ylwu-amzn · 2020-08-04T19:58:35Z

cli/internal/mapper/ad/ad.go

+	return features, nil
+}
+
+func getUnit(request string) (*string, error) {


We only support minutes on AD Kibana. Should we support more unit options in AD CLI? If user use "Hours" to create index with CLI, then AD Kibana may not show it correctly.

Now, create command fails with response "Hours is not supported". In my opinion this is more as restriction in backend than frontend. What do you think? Do we plan to support this in future? if so then having this will have old clients to use this format.

Currently we have no plan to support other unit options. How about we support only minutes here to keep consistent with AD Kibana (support minutes only) and backend (supports minutes/seconds, code).

Sounds good. Will keep only minutes and will raise error for other cases

ylwu-amzn · 2020-08-04T19:59:50Z

cli/internal/mapper/ad/ad.go

+		featureCount += len(f.AggregationType) * len(f.Field)
+	}
+	if featureCount == 0 || featureCount > featureCountLimit {
+		return fmt.Errorf("trying to create %d feautes, only upto %d features are allowed", featureCount, featureCountLimit)


minor: typo, feautes -> features

ylwu-amzn · 2020-08-04T21:58:30Z

cli/cmd/start_stop.go

+			return
+		}
+		idStatus, _ := cmd.Flags().GetBool("id")
+		action := ad.StopAnomalyDetector


How about rename as "StopAnomalyDetectorByNamePattern" considering this method "StopAnomalyDetectorByID"

Rename Method name to include byNamePattern Support only minutes for interval Added extra comments

ESAD Version 0.1 to interact with AD Plugin through cli.

Merge ESAD CLI into anomaly-detection

0487a45

Merged esad from private repo to anomaly detection plugin.

VijayanB force-pushed the merge-cli branch from 7599562 to 0487a45 Compare July 27, 2020 20:36

VijayanB self-assigned this Jul 27, 2020

VijayanB requested review from sean-zheng-amazon and ylwu-amzn July 27, 2020 23:25

VijayanB added the feature new feature label Jul 28, 2020

VijayanB added 2 commits July 28, 2020 15:15

Include gitignore file

2e49d94

Add workflow

8e31454

Added build and publish artifacts

VijayanB force-pushed the merge-cli branch from 33a505e to 8e31454 Compare July 28, 2020 22:57

wnbts reviewed Jul 29, 2020

View reviewed changes

VijayanB added 5 commits July 28, 2020 18:32

test 1

34dea76

fixed workflow paths to cli directory

12659ce

Refactored profiles

a0f363d

Fixed Code review comments

Update workflow name

fb9dfc6

Update publish only tags are created.

Temporarily allow publish artifact on push/pull

f1ccf8c

VijayanB requested a review from wnbts July 29, 2020 22:21

wnbts reviewed Jul 30, 2020

View reviewed changes

Upload code coverage

9332f7d

wnbts approved these changes Jul 31, 2020

View reviewed changes

VijayanB added 3 commits July 31, 2020 13:47

Fixed code review comments

98ca1f1

Don't print error on root command

a00bc59

fix lint issue

924511e

ylwu-amzn reviewed Aug 4, 2020

View reviewed changes

Update Readme

adfe6dd

ylwu-amzn reviewed Aug 4, 2020

View reviewed changes

VijayanB added 3 commits August 4, 2020 10:56

Rename Search to SearchDistinctValues

b32a33a

rename controller to es

d65eb8a

Fixed following review comments

9b805da

1) Move methods close to help readability 2) Add comments to exported methods 3) Fix typo and copied comments 4) Rename file name

ylwu-amzn reviewed Aug 4, 2020

View reviewed changes

VijayanB added 2 commits August 4, 2020 14:22

use map for get profiles

9659ac6

Fixed typo

2bec3bd

ylwu-amzn reviewed Aug 4, 2020

View reviewed changes

VijayanB added 3 commits August 4, 2020 15:03

Move validation as method argument

5e7336d

Fixed Review comments

d92e263

Rename Method name to include byNamePattern Support only minutes for interval Added extra comments

Renamed gateway

7bea3e6

VijayanB merged commit d6dd8ab into opendistro-for-elasticsearch:master Aug 5, 2020

VijayanB deleted the merge-cli branch August 5, 2020 17:33

VijayanB changed the title ~~Merge esad cli into anomaly-detection~~ AD CLI Aug 25, 2020

yizheliu-amazon pushed a commit that referenced this pull request Aug 28, 2020

Merge esad cli into anomaly-detection (#196)

fbe5a58

ESAD Version 0.1 to interact with AD Plugin through cli.

		@@ -0,0 +1,97 @@
		![Test Workflow](https://github.com/VijayanB/esad/workflows/Build%20and%20Test%20Anomaly%20detection%20commandline%20tool/badge.svg)

AD CLI #196

AD CLI #196

Conversation

VijayanB commented Jul 27, 2020

ylwu-amzn commented Jul 27, 2020

VijayanB commented Jul 27, 2020

wnbts left a 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

VijayanB Jul 29, 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

VijayanB commented Jul 29, 2020

wnbts commented Jul 30, 2020

Choose a reason for hiding this comment

VijayanB Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2020 • edited Loading

Codecov Report

wnbts left a 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

ylwu-amzn Aug 4, 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

ylwu-amzn Aug 4, 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

VijayanB Jul 29, 2020 •

edited

Loading

VijayanB Jul 30, 2020 •

edited

Loading

codecov bot commented Jul 30, 2020 •

edited

Loading

ylwu-amzn Aug 4, 2020 •

edited

Loading

ylwu-amzn Aug 4, 2020 •

edited

Loading