open-contracting / kingfisher-process

Stores and pre-processes OCDS data in a SQL database
https://kingfisher-process.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2 stars 8 forks source link

Most print statements should be replaced by logging calls #60

Closed tian2992 closed 4 years ago

tian2992 commented 6 years ago

Logging should be handled better, at the very least be possible to implement a flag for -verbose -quiet or for specific parts of the pipeline and/or redirect to logfiles.

odscjames commented 6 years ago

I don't understand what all the bullet points mean, but I agree with the main idea.

Putting it through some sort of logging framework rather than just printing to the screen would allow for much more flexibility to move logs elsewhere later.

Alembic uses some kind of logging framework and puts it's messages through that, maybe we should look at that?

jpmckinney commented 6 years ago

FYI, Python's logging facility is quite powerful and flexible. I believe Alembic uses it as well.

tian2992 commented 6 years ago

+1 on to Python's own logging!

@odscjames the idea was about general "microtasks" to be ticked off, of course open to suggestion.

odscjames commented 6 years ago

I did #161 to get this started! Have a look. If people are happy with #161, there are some other things to look at next that can be done here or on seperate issues.

Add more info/debug statements everywhere! We should maybe have some discussion on what level to use for what, which is why I did not add too many straight away.

Think about removing some of the print statements and use logging instead. (I know this was the original point of this issue, but we should review how it looks to CLI users. I still think adding logging is good generally; once we have more comprehensive messages appear it can help debug problems. I've already had an issue appear on a long run and this would have really helped me debug it!)

It would be good if each source run has it's logging files appear in the same folder as the other files:

data/australia_sample data/australia_sample/2018-07-19-11-27-23 data/australia_sample/2018-07-19-11-27-23/metadb.sqlite3 data/australia_sample/2018-07-19-11-27-23/errors.log data/australia_sample/2018-07-19-11-27-23/info.log data/australia_sample/2018-07-19-11-27-23/packages-023cb74eec440a851a6d5e5befb019a0.json data/australia_sample/2018-07-19-11-27-23/packages-0d77f752076f581875c4e5d223f79ca4.json

However, I don't think this is possible with config files alone. We could do this if we configure the logging directly in the library, but maybe we don't want to do that. So I haven't done this yet - discussion first.

jpmckinney commented 5 years ago

I still see print statements in kingfisher-process, so still relevant.

odscjames commented 5 years ago

Note this issue might relate to #21