Fixes log format issues #865

ebozduman · 2017-11-08T02:33:26Z

Fixes #833

kannappanr · 2017-11-08T18:52:57Z

functional_tests.go

 		return
 	}

+	args["destination"] = dst
+	// Set args["sourceList"] to "" to stop having 10,001 null headers logged
+	args["sourceList"] = ""


You could say something like "source array of 10001 elements".

kannappanr · 2017-11-08T19:01:42Z

functional_tests.go

-
+	testName := getFuncName()
+	function := "ComposeObject(destination, sourceList)"
+	args := map[string]interface{}{


This can be removed.

kannappanr · 2017-11-08T19:02:20Z

functional_tests.go

-	args := map[string]interface{}{}
+	testName := getFuncName()
+	function := "ComposeObject(destination, sourceList)"
+	args := map[string]interface{}{


this can also be removed

kannappanr · 2017-11-08T19:03:17Z

functional_tests.go

-	args := map[string]interface{}{}
+	testName := getFuncName()
+	function := "CopyObject(destination, source)"
+	args := map[string]interface{}{


This can also be removed.

kannappanr

Please make the changes relating to settings args and function names in the quick test mode

kannappanr

LGTM

krisis · 2017-11-13T10:18:26Z

functional_tests.go

 // Tests bucket re-create errors.
 func testMakeBucketError() {
 	region := "eu-central-1"

 	// initialize logging params
 	startTime := time.Now()
-	function := "MakeBucket(bucketName, region)"
+	testName := getFuncName()
+	function := []string{"MakeBucket(bucketName, region)"}


Isn't each test suppose to test one SDK API primarily, even though it may use many API methods? Why do we need a []string for function field in the log json? This is different from our decided format here - https://github.com/minio/mint#mint-log-format

That's true, but there are still some test cases already part of the existing code, like testFunctionalV2, and testFunctional, which come as a bunch of api tests. I guess these tests have been added as part of the new QuickTest mode which is why the string array is introduced recently.

Does it mean you need to fix the test than breaking JSON?

No, we don't need to fix anything, imho. That is perfectly fine.
So, what I did was to accommodate these kind of test cases and handle them in a kind of a better way.
Now, when these tests run, each and every api name will be printed in the function field as part of the json file if there is an error. If all of the api tests pass, then all of the api names will be printed out in the function field as part of the json file. So, no problem.

Can you provide sample JSONs on success and failure cases?

Here is the success example:

go build functional_tests.go;go run functional_tests.go | jq { "args": { "bucketName": "minio-go-test-hx5t3l96fruilwyi" }, "duration": 2119, "function": [ "MakeBucket(bucketName, region)", "BucketExists(bucketName)", "GetBucketPolicy(bucketName, objectPrefix)", "SetBucketPolicy(bucketName, objectPrefix, bucketPolicy)", "ListBuckets()", "PutObject(bucketName, objectName, reader, contentType)", "ListObjects(bucketName, objectName, isRecursive, doneCh)", "ListObjectsV2(bucketName, objectName, isRecursive, doneCh)", "ListIncompleteUploads(bucketName, objectName, isRecursive, doneCh)", "GetObject(bucketName, objectName)", "FGetObject(bucketName, objectName, fileName)", "PresignedHeadObject(bucketName, objectName, expires, reqParams)", "PresignedGetObject(bucketName, objectName, expires, reqParams)", "PresignedPutObject(bucketName, objectName, expires)", "RemoveObject(bucketName, objectName)", "RemoveBucket(bucketName)" ], "name": "minio-go: testFunctional", "status": "PASS" } { "args": { "bucketName": "minio-go-test-04ihrp4hwhtm2dst", "objectName": "minio-go-test-04ihrp4hwhtm2dstunique-presigned" }, "duration": 634, "function": [ "MakeBucket(bucketName, region)", "BucketExists(bucketName)", "SetBucketPolicy(bucketName, objectPrefix, bucketPolicy)", "ListBuckets()", "ListObjects(bucketName, objectName, isRecursive, doneCh)", "ListIncompleteUploads(bucketName, objectName, isRecursive, doneCh)", "GetObject(bucketName, objectName)", "FGetObject(bucketName, objectName, fileName)", "PresignedHeadObject(bucketName, objectName, expires, reqParams)", "PresignedGetObject(bucketName, objectName, expires, reqParams)", "PresignedPutObject(bucketName, objectName, expires)" ], "name": "minio-go: testFunctionalV2", "status": "PASS" }

... and here is the failure example:
(induced an error just before SetBucketPolicy api execution):

$ go build functional_tests.go;go run functional_tests.go | jq { "args": { "bucketName": "minio-go-test-eomoglxiubs5s9h9", "bucketPolicy": "readonly", "objectPrefix": "" }, "duration": 169, "error": "BOOM!", "function": [ "SetBucketPolicy(bucketName, objectPrefix, bucketPolicy)" ], "message": "SetBucketPolicy failed", "name": "minio-go: testFunctional", "status": "FAIL" }

I see you change function field as array which leads to dynamic JSON i.e. other tests have the field as string (as per currently agreed JSON format). You would need to fix tests, not JSON.

@balamurugana
The issue here is the way quick mode feature is designed and implemented. It is a break in the rule of having a single api tested in a single test case. Having a group of api tests running inside a single test case/wrapper and reporting the wrapper name without the covered api names is a shortage of information that is supposed to be conveyed to the end user. That's what is behind the code change of having function field transformed from string to array type. So, it was an attempt to accommodate the existing quick mode feature design and implementation to our existing standard with minimum effort.

Now, if we don't want to change the function field to array type, then we need to decide if we are ok with not reporting apis tested under testFunctionalV2, and testFunctional test cases.

Please let me know what you think.

We also need to make sure our whole mint team understand that grouping of api tests in one single test case is against our mint testing design philosophy.

@balamurugana
(It is not the finest looking code, but) I kept the function field as string and populated it with the list of all apis tested.
Now here how it loooks like:

Success:

$ go build functional_tests.go;go run functional_tests.go | jq { "args": { "bucketName": "minio-go-test-lj99jvecgld2v1rk" }, "duration": 1486, "function": "MakeBucket(bucketName, region), BucketExists(bucketName), GetBucketPolicy(bucketName, objectPrefix), SetBucketPolicy(bucketName, objectPrefix, bucketPolicy), GetBucketPolicy(bucketName, objectPrefix), SetBucketPolicy(bucketName, objectPrefix, bucketPolicy), GetBucketPolicy(bucketName, objectPrefix), SetBucketPolicy(bucketName, objectPrefix, bucketPolicy), GetBucketPolicy(bucketName, objectPrefix), ListBuckets(), PutObject(bucketName, objectName, reader, contentType), ListObjects(bucketName, objectName, isRecursive, doneCh), ListObjectsV2(bucketName, objectName, isRecursive, doneCh), ListIncompleteUploads(bucketName, objectName, isRecursive, doneCh), GetObject(bucketName, objectName), FGetObject(bucketName, objectName, fileName), PresignedHeadObject(bucketName, objectName, expires, reqParams), PresignedHeadObject(bucketName, objectName, expires, reqParams), PresignedGetObject(bucketName, objectName, expires, reqParams), PresignedGetObject(bucketName, objectName, expires, reqParams), PresignedPutObject(bucketName, objectName, expires), PresignedPutObject(bucketName, objectName, expires), RemoveObject(bucketName, objectName), RemoveBucket(bucketName)", "name": "minio-go: testFunctional", "status": "PASS" } { "args": { "bucketName": "minio-go-test-oimq6as19p2ut49e", "objectName": "minio-go-test-oimq6as19p2ut49eunique-presigned" }, "duration": 437, "function": "MakeBucket(bucketName, location), BucketExists(bucketName), SetBucketPolicy(bucketName, objectPrefix, bucketPolicy), ListBuckets(), ListObjects(bucketName, objectName, isRecursive, doneCh), ListIncompleteUploads(bucketName, objectName, isRecursive, doneCh), GetObject(bucketName, objectName), FGetObject(bucketName, objectName, fileName), PresignedHeadObject(bucketName, objectName, expires, reqParams), PresignedGetObject(bucketName, objectName, expires, reqParams), PresignedPutObject(bucketName, objectName, expires), GetObject(bucketName, objectName)", "name": "minio-go: testFunctionalV2", "status": "PASS" }

Failure (error induced before BucketExists api):

go build functional_tests.go;go run functional_tests.go | jq { "args": { "bucketName": "minio-go-test-w3i5rbgfrvh2j5xe" }, "duration": 212, "error": "BOOM!", "function": "BucketExists(bucketName)", "message": "BucketExists failed", "name": "minio-go: testFunctional", "status": "FAIL" }

I don't know whether I understood quick mode design/implementation here. All tests in minio-go are run in all modes i.e. there is no difference for quick and enhanced modes.

I haven't completely understood any arguments about mint JSON till now, hence I can't comment on that. I am only interested in whether I will find what/where the failure happened in tests so that I find the problem and fix it accordingly. If failure log provides such information, I am OK.

If you say below JSON helps tracing down the problem whether its in test and/or server, its good to go.

{ "args": { "bucketName": "minio-go-test-w3i5rbgfrvh2j5xe" }, "duration": 212, "error": "BOOM!", "function": "BucketExists(bucketName)", "message": "BucketExists failed", "name": "minio-go: testFunctional", "status": "FAIL" }

nitisht · 2017-11-20T07:15:22Z

@ebozduman can you please address the conflicts and travis failures

deekoder · 2017-11-20T20:34:59Z

Waiting on build failures to get resolved.

deekoder requested review from krisis and kannappanr November 8, 2017 03:04

kannappanr reviewed Nov 8, 2017

View reviewed changes

kannappanr requested changes Nov 8, 2017

View reviewed changes

ebozduman force-pushed the log_format branch from bc9d57a to 2f84313 Compare November 9, 2017 22:41

kannappanr approved these changes Nov 10, 2017

View reviewed changes

ebozduman force-pushed the log_format branch from 9c8dc74 to 7088ee5 Compare November 10, 2017 19:43

krisis requested changes Nov 13, 2017

View reviewed changes

ebozduman force-pushed the log_format branch from 6202158 to 4459660 Compare November 15, 2017 20:37

krisis approved these changes Nov 20, 2017

View reviewed changes

nitisht mentioned this pull request Nov 20, 2017

Fix log parsing to check for lowercase fail in status minio/mint#235

Merged

ebozduman force-pushed the log_format branch 2 times, most recently from 1e82ad2 to a7a9fb0 Compare November 20, 2017 23:41

Fixes log format issues

db5b6d8

ebozduman force-pushed the log_format branch from 721b4b2 to db5b6d8 Compare November 21, 2017 19:36

deekoder merged commit f56a10c into minio:master Nov 22, 2017

ebozduman deleted the log_format branch November 22, 2017 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes log format issues #865

Fixes log format issues #865

ebozduman commented Nov 8, 2017 •

edited

Loading

kannappanr Nov 8, 2017

ebozduman Nov 9, 2017

kannappanr Nov 8, 2017

ebozduman Nov 9, 2017

ebozduman Nov 9, 2017

kannappanr Nov 8, 2017

ebozduman Nov 9, 2017

kannappanr Nov 8, 2017

ebozduman Nov 9, 2017

kannappanr left a comment

kannappanr left a comment

krisis Nov 13, 2017

ebozduman Nov 14, 2017

balamurugana Nov 14, 2017

ebozduman Nov 15, 2017

balamurugana Nov 15, 2017

ebozduman Nov 15, 2017 •

edited

Loading

balamurugana Nov 15, 2017 •

edited

Loading

ebozduman Nov 15, 2017 •

edited

Loading

ebozduman Nov 15, 2017 •

edited

Loading

balamurugana Nov 15, 2017

nitisht commented Nov 20, 2017

deekoder commented Nov 20, 2017

Fixes log format issues #865

Fixes log format issues #865

Conversation

ebozduman commented Nov 8, 2017 • 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

kannappanr left a comment

Choose a reason for hiding this comment

kannappanr 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

ebozduman Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

balamurugana Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

ebozduman Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

ebozduman Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitisht commented Nov 20, 2017

deekoder commented Nov 20, 2017

ebozduman commented Nov 8, 2017 •

edited

Loading

ebozduman Nov 15, 2017 •

edited

Loading

balamurugana Nov 15, 2017 •

edited

Loading

ebozduman Nov 15, 2017 •

edited

Loading

ebozduman Nov 15, 2017 •

edited

Loading