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

add duration of the scan to JSON #1937

Closed
armijnhemel opened this issue Feb 19, 2020 · 16 comments · Fixed by #1942
Closed

add duration of the scan to JSON #1937

armijnhemel opened this issue Feb 19, 2020 · 16 comments · Fixed by #1942

Comments

@armijnhemel
Copy link
Contributor

Short Description

currently there is already a start time stamp and end time stamp in the JSON output. It would be nice to also have a "duration" field so I don't have to compute that myself.

Possible Labels

  • new feature

Select Category

  • Enhancement [X]
  • Add License/Copyright []
  • Scan Feature []
  • Packaging []
  • Documentation []
  • Expand Support []
  • Other []

Describe the Update

How This Feature will help you/your organization

Possible Solution/Implementation Details

end_timestamp - start_timestamp

Example/Links if Any

Can you help with this Feature

@pombredanne
Copy link
Member

@armijnhemel Thank you for this: would a duration in seconds work as in 7776045? (as opposed to a human readable and formatted string such as 3:12:45 for 3 hours, 12 minutes and 45 seconds?
Note also this cannot be 100% accurate as the actual time it takes to write the JSON cannot be integrated, and this could be several seconds on large scans.

@armijnhemel
Copy link
Contributor Author

@armijnhemel Thank you for this: would a duration in seconds work as in 7776045? (as opposed to a human readable and formatted string such as 3:12:45 for 3 hours, 12 minutes and 45 seconds?
Note also this cannot be 100% accurate as the actual time it takes to write the JSON cannot be integrated, and this could be several seconds on large scans.

Having just seconds would be perfect. I could do formatting myself later.

@mnv936
Copy link

mnv936 commented Feb 23, 2020

@pombredanne i would like to work on this feature.
Anything I should know that might help me or I should take care of?

@shivam-bit
Copy link

@pombredanne even I would love to work on this feature

@MankaranSingh
Copy link
Contributor

Hi, I have added the requested feature in the following pull requests:
#1939

Now, you can see a 'time_taken' field in the header of the output JSON file,

@MankaranSingh
Copy link
Contributor

i guess i need to edit the test cases too since this feature requires direct change in the output JSON file, alot of test cases.

So how should i go dealing with this ? any help is appreciated

@pombredanne
Copy link
Member

@MankaranSingh thank you for this!
@armijnhemel do you see this feature as something always present in the JSON scan outputs? Or as guarded by a command line option?

@pombredanne
Copy link
Member

@MankaranSingh let's see if this need to be enabled at all times or not first before updating the tests. There are ways to regenerate test fixtures to help a bit

@mjherzog
Copy link
Member

I think that we can avoid yet another command line option.

@armijnhemel
Copy link
Contributor Author

I would just always include it.

@pombredanne
Copy link
Member

@MankaranSingh so you will have indeed to update a good bunch of test scan files since they will now contain your new field... On te other hand, since this field will have a different value on each run, it is best excluded from most of the scan tests assertions, except for a few tests focused only on the tests of that feature. Does it make sense? There are very few places where these checks are done, mostly in this place https://github.com/nexB/scancode-toolkit/blob/develop/src/scancode/cli_test_utils.py

@MankaranSingh
Copy link
Contributor

ok, on it.
So, if i need further help, shall i ask here or on the gitter discussion channel ?

@pombredanne
Copy link
Member

@MankaranSingh best is to keep ticket related discussions on the ticket and you can ask for help in the chat linking to the ticket.

@MankaranSingh
Copy link
Contributor

MankaranSingh commented Feb 26, 2020

I have opened another pull request #1942
Here are all the changes
@pombredanne ok, so all the tests seem to be passing now.
What i found out was, that the "start_timestamp" and "end_timestamp" were excluded from the JSON file prior to comparing with the expected JSON file. So, it made sense for me to exclude the new "time_taken" field too !

also, i found no other tests that specifically tested the "start_timestamp" and "end_timestamp", so i too didn't write any other test for the "time_taken" feature.

Also, @armijnhemel, i have updated my commit to store time as float instead of string.

@dakshaladia
Copy link

I wish to work on this issue. Is it still available?

@armijnhemel
Copy link
Contributor Author

I wish to work on this issue. Is it still available?

I suppose it is no longer available, but perhaps @pombredanne can clarify?

pombredanne added a commit that referenced this issue Mar 5, 2020
Add new duration field to JSON output #1937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants