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 logging to generate script #66

Merged
merged 3 commits into from
Jan 9, 2021
Merged

Conversation

coni2k
Copy link
Contributor

@coni2k coni2k commented Jan 8, 2021

When we run generate script for large number of repos, it becomes difficult to follow the exceptions.
Not sure what are the best practices but I added basic logging by following this example. Looks like it's doing the job:
https://stackoverflow.com/questions/15474095/writing-a-log-file-from-python-program

As usual, feel free to accept/discard it

criticality_score/generate.py Outdated Show resolved Hide resolved
print(
f'Exception occurred when reading repo: {repo_url}\n{exp}')
msg = f'Exception occurred when reading repo: {repo_url}\n{exp}'
print(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right use of logging methods, to just log errors and not anything else. I think you can add logging handler, but then convert all the prints to use that logging pipelines (regular print should be info level). Then log file will have all log data, info and errors. and you can search for errors using error log level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, since logger also prints the messages to the console.

Updates in last commit:

  • Move logger to global
  • Replace print with logger.info
  • Set loglevel to info

I also tried to share the logger with run script. The only way that I could do it was to pass it as an argument in each function. e.g. def get_repository(url, logger):

I assume that there's a better way to do it, so I'm skipping that one for now.

- Replace print with logger.info
- Set loglevel to info
Copy link
Contributor

@inferno-chromium inferno-chromium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor changes needed.

criticality_score/generate.py Outdated Show resolved Hide resolved
criticality_score/generate.py Outdated Show resolved Hide resolved
@inferno-chromium inferno-chromium merged commit b3859c6 into ossf:main Jan 9, 2021
@coni2k coni2k deleted the add-logging branch January 9, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants