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

[v6r21] Clean logs 1 #3991

Merged
merged 3 commits into from
Mar 25, 2019
Merged

Conversation

chaen
Copy link
Contributor

@chaen chaen commented Mar 18, 2019

This PR is the first one of a looooong serie where I will, as announced, clean the logs. I will do such PR every week.
@atsareg I will build on top of that one, so please always merge this one just before doing the release

BEGINRELEASENOTES

*Core
CHANGE: MySQL class prints only debug logs

ENDRELEASENOTES

@coveralls
Copy link

coveralls commented Mar 18, 2019

Coverage Status

Coverage decreased (-0.002%) to 22.14% when pulling 1c7b8d4 on chaen:rel-v6r21_cleanLog_1 into f626bd7 on DIRACGrid:rel-v6r21.

@andresailer
Copy link
Contributor

What about also changing the name of the logger to __name__ ?

@chaen
Copy link
Contributor Author

chaen commented Mar 18, 2019

I am ok with that policy. But what about loggers inside the classes ?

raise Exception( error )
if isinstance( attrValue, list ):
retDict = self._escapeValues( attrValue )
self.log.debug('buildCondition:', error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK to change to debug when it was verbose but these are warn. Shouldn't they be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does not make sense to log it as warning so deep down in the stack. The error is correctly reported to the higher level, and it's up to them to issue the warning

"""
set MySQL connection parameters and try to connect

:param debug: unused
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be removed then altogether ?

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 would have wanted to,but there is no way to "deprecate it" with the decorator as it is now. I will try to find a way fo a further patch release, and we remove it in v7. OK for you ?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

if not retDict['OK']:
return retDict
self.log.info( 'Table %s created' % table )
self.log.debug('Table %s created' % table)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be left at Info. This is only printed at the first service invocation when no tables are there yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, but does it make sense ? the service anyway prints it if creates table.

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, then this one can be dropped completely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. If you don't mind, I will do it in the next PR for log cleaning, such that these changes are in the next patch release and can already go in prod next week

@atsareg atsareg merged commit 68320fd into DIRACGrid:rel-v6r21 Mar 25, 2019
@chaen chaen mentioned this pull request May 2, 2022
@chaen chaen deleted the rel-v6r21_cleanLog_1 branch August 11, 2022 12:56
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

5 participants