Log refactoring

[burnout87]

Refactoring ongoing

[codecov]

Codecov Report

Merging #39 (4b59ce3) into master (8e979a1) will increase coverage by 1.69%.
The diff coverage is 73.41%.

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   57.78%   59.47%   +1.69%     
==========================================
  Files          33       33              
  Lines        4157     4163       +6     
==========================================
+ Hits         2402     2476      +74     
+ Misses       1755     1687      -68     

Impacted Files	Coverage Δ
...plugins/dummy_instrument/data_server_dispatcher.py	87.32% <ø> (ø)
cdci_data_analysis/analysis/instrument.py	58.33% <44.44%> (+4.63%)	⬆️
cdci_data_analysis/flask_app/dispatcher_query.py	66.26% <76.19%> (+2.63%)	⬆️
cdci_data_analysis/analysis/queries.py	57.77% <85.71%> (+4.24%)	⬆️
cdci_data_analysis/analysis/parameters.py	64.69% <0.00%> (+0.51%)	⬆️
cdci_data_analysis/flask_app/app.py	63.02% <0.00%> (+0.84%)	⬆️
cdci_data_analysis/analysis/products.py	44.18% <0.00%> (+2.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e979a1...4b59ce3. Read the comment docs.

[burnout87]

@volodymyrss Remember if this TODO was discussed at some point in time?

dispatcher-app/cdci_data_analysis/flask_app/dispatcher_query.py

Lines 134 to 138 in 37a0eb8

	# TODO
	# decide if it is worth to add the logger also in this case
	#self.set_scratch_dir(self.par_dic['session_id'], verbose=verbose)
	#self.set_session_logger(self.scratch_dir, verbose=verbose, config=config)
	# self.set_sentry_client()

I think some log might still be useful, but perhaps without the scratch_dir

[lgtm-com]

This pull request introduces 2 alerts when merging 37a0eb8 into 2f47456 - view on LGTM.com

new alerts:

• 2 for Wrong number of arguments in a call

[volodymyrss]

@volodymyrss Remember if this TODO was discussed at some point in time?

dispatcher-app/cdci_data_analysis/flask_app/dispatcher_query.py

Lines 134 to 138 in 37a0eb8

	# TODO
	# decide if it is worth to add the logger also in this case
	#self.set_scratch_dir(self.par_dic['session_id'], verbose=verbose)
	#self.set_session_logger(self.scratch_dir, verbose=verbose, config=config)
	# self.set_sentry_client()

I think some log might still be useful, but perhaps without the scratch_dir

I think it would be still good to store log in the individual scratch directory of the job. It allows to inspect the job for each request easily near data. In some cases it is still useful.
Except now it did not work, and caused some issues, I do not remember, perhaps with overcreation of loggers.

[volodymyrss]

can we add a test to check if log was created? because the problem was that it was not, that's why it was commented, I think

[burnout87]

It was crashing mainly because the job_id was not set for that specific case, and when trying to create the folder it would not find that job_id.

I will add the check

[volodymyrss]

and also that it does not create a new logger for each request. That's what was happening at some points too.
I find it is convenient to inspect loggers with logging_tree, but simply logging would do too.

[burnout87]

I am trying to use logging_tree to inspect the log, but I am struggling a bt, what do you think of verifying that the log file is actually created? Will this be enugh for us?

[volodymyrss]

it will be necessary but not sufficient, since it will not catch an problem which happened in the wild. but it might be unclear what I am trying to explain with the logging, so let me maybe to that part myself.

[lgtm-com]

This pull request introduces 2 alerts and fixes 6 when merging e78527a into e71a9cd - view on LGTM.com

new alerts:

• 1 for Unused local variable
• 1 for Syntax error

fixed alerts:

• 4 for Module is imported more than once
• 1 for Unused import
• 1 for Module is imported with 'import' and 'import from'

[volodymyrss]

I would not necessarily deal with this for 21.05

[volodymyrss]

I just merged master and better coverage testing to see how far it is, and it is not too close anymore. Perhaps it's too much effort right now. I hope no merges were destructive.

[burnout87]

I just merged master and better coverage testing to see how far it is, and it is not too close anymore. Perhaps it's too much effort right now. I hope no merges were destructive.

Thanks! It is not necessary at the moment. So it can be labelled as an enhancement and we can deal with that at a later stage after the release.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 4b59ce3 into 8e979a1 - view on LGTM.com

new alerts:

• 1 for Unused local variable

fixed alerts:

• 1 for Unused local variable