Skip to content
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

Update mc to output new mint log format #114

Closed
nitisht opened this issue Aug 7, 2017 · 10 comments
Closed

Update mc to output new mint log format #114

nitisht opened this issue Aug 7, 2017 · 10 comments
Assignees
Milestone

Comments

@nitisht
Copy link
Contributor

nitisht commented Aug 7, 2017

mc logs need to be updated in the format below so that mint logs can be easily parsed.

{
    "name":"mc", // SDK Name
    "function":"testPresignGetObject", // Test function name
    "description": "test function description (optional)", //  Test function description
    "args":"", // key value map, varName:value. Only arguements that devs may be interested in
    "duration":"", // duration of the whole test
    "status":"", // can be PASS, FAIL, NA
    "alert":"failed to download pre-signed object(optional)", // error related, human readable message. Should be taken care of if present
    "error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program
}
@nitisht nitisht self-assigned this Aug 7, 2017
@nitisht nitisht added this to the Current milestone Aug 9, 2017
@ebozduman
Copy link
Collaborator

ebozduman commented Aug 16, 2017

Question: Do we need to make distinction between required/mandatory and optional arguments for the test case when the arguments are listed in the logs?
aws-sdk-ruby has that information and if decided, it can be included in the "args": field.

Currently, aws-sdk-ruby provides an array of strings for "args": field, like:

{
  "name": "aws-sdk-ruby",
  "function": "copyObjectTest",
  "args": [
    "source_bucket_name",
    "target_bucket_name",
    "data_dir",
    "source_file_name",
    "target_file_name"
  ],
  "comment": "Tests copyObject api command",
  "message": "NULL",
  "error": "NULL",
  "status": "PASS"
}

Please advise.

@ebozduman
Copy link
Collaborator

ebozduman commented Aug 16, 2017

I think we also need to discuss how to differentiate between the same test running with different arg values to test different scenarios, like invalid inputs etc.. At this point, "comment": being set inside the test method, there is no way to differentiate them from each other.
First thing comes to my mind is to have caller define the purpose of the test in the comment, and have comment as an additional argument for the test, but I don't like it. I am sure there is a better way to handle this situation.
Any thoughts?

@nitisht
Copy link
Contributor Author

nitisht commented Aug 17, 2017

@ebozduman I think the args field doesn't really solve a purpose. Instead we can have comment moved just below the function section and have comment indicate short description of what the test is supposed to validate.

@kannappanr
Copy link
Collaborator

@nitisht ,

There was a change in the log format. You are missing "message" field. AB asked us to remove the description field and add the parameters in the function field. Here is a modified version of a sample log entry

{
"name":"mc", // SDK Name
"function":"testPresignGetObject(int index)", // Test function name
"args":"", // key value map, varName:value. Only arguements that devs may be interested in
"duration":"1.5 s", // duration of the whole test in seconds
"status":"", // can be PASS, FAIL, NA
"alert":"Information like whether this is a Blocker/ Gateway, Server etc can go here",
"message":" descriptive error message" // error related, human readable message. Should be taken care of if present
"error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program
}

@ebozduman
Copy link
Collaborator

ebozduman commented Aug 22, 2017

I think there are couple of things I'd like to suggest/clarify and confirm with the team and AB:

  1. "function": My understanding is that this field was going to be the test method name and its arguments, like bucketExistsNegativeTest(bucket_name), not the api name bucketExists(bucket_name).
  2. "description": I'd like to suggest keeping this optional field for those test cases which may need some extra words to explain what is happening inside the test method when the name of the test is not self-explanatory or sufficient enough.

@balamurugana
Copy link
Member

"function": My understanding is that this field was going to be the test method name and its arguments, like bucketExistsNegativeTest(bucket_name), not the api name bucketExists(bucket_name).

For SDK, my recommendation is to use test method name than API name.
For tools, full command line like mc cp file minio/mybucket/myobject

"description": I'd like to suggest keeping this optional field for those test cases which may need some extra words to explain what is happening inside the test method when the name of the test is not self-explanatory.

As we send only fields with values, I am OK with this

@nitisht
Copy link
Contributor Author

nitisht commented Aug 23, 2017

@kannappanr @ebozduman @balamurugana can we have the final/consolidated log format pasted here, so it doesn't lead to rework.

@kannappanr
Copy link
Collaborator

kannappanr commented Aug 23, 2017

Latest Log Format.

{
"name":"mc", // SDK Name
"function":"makeBucket(String bucketName)", // API name
"args":"", // key value map, varName:value. Only arguements that devs may be interested in
"duration":"15 ", // duration of the whole test in milli seconds
"status":"", // can be PASS, FAIL, NA
"alert":"Information like whether this is a Blocker/ Gateway, Server etc can go here",
"message":" descriptive error message" // error related, human readable message. Should be taken care of if present
"error":"stack-trace/exception message(only in case of failure)" // actual low level exception/error thrown by the program
}

@ebozduman
Copy link
Collaborator

The above information Kannappan put together came from an informal meeting Kannappan, AB and I had.
We can definitely have another meeting to talk about the logic and reasons why the latest format is decided.

@nitisht
Copy link
Contributor Author

nitisht commented Aug 23, 2017

The above information Kannappan put together came from an informal meeting Kannappan, AB and I had. We can definitely have another meeting to talk about the logic and reasons why the latest format is decided.

Thanks @ebozduman . We can do that, but as long as this format is agreed upon and approved by @abperiasamy lets adhere to this. We'll review current PRs and implement new logging based on this format.

nitisht added a commit to nitisht/mint that referenced this issue Aug 23, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 23, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 23, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 25, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 25, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 25, 2017
nitisht added a commit to nitisht/mint that referenced this issue Aug 26, 2017
nitisht added a commit to nitisht/mint that referenced this issue Sep 1, 2017
nitisht added a commit to nitisht/mint that referenced this issue Sep 1, 2017
balamurugana pushed a commit that referenced this issue Sep 1, 2017
@nitisht nitisht added the fixed label Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants