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

INFO to DEBUG log level #819

Closed
wants to merge 1 commit into from
Closed

INFO to DEBUG log level #819

wants to merge 1 commit into from

Conversation

banesullivan
Copy link
Contributor

Resolves #690

This changes most INFO level logs to DEBUG - some were changed to ERROR where I thought appropriate

@manthey
Copy link
Member

manthey commented Apr 8, 2022

I don't understand the point of changing a lot of these -- many of them are intended to be visible information, not just extra debug information. I think you really just don't want to see logs from the library, not change their level.

Maybe we should change the default logger that is set up instead. Python recommends libraries have:
logging.getLogger(name).addHandler(logging.NullHandler())
as the default handler for their logger. So changing large_image/config.py to have the fallback logger use the NullHandler instead of the StreamHandler is probably a better choice than changing log levels.

As an example where I'd rather not change from INFO to DEBUG, tiling frames can be slow, so logging information every 10 seconds is useful to see that something is progressing. Similarly, the logs in the thumbnail task in Girder effectively vanish if we set them to debug, losing feedback from the job.

@banesullivan
Copy link
Contributor Author

have the fallback logger use the NullHandler instead of the StreamHandler is probably a better choice than changing log levels.

I didn't know about that. I'll see if I can give this PR another go to implement that and make sure the changes are more useful/relevant. We'd need to change all logger. to config.getConfig('logger'), right?

I think you really just don't want to see logs from the library, not change their level.

Somewhat - there are a few that should be changed to an ERROR level rather than INFO but those are minor. It also makes it easier to filter on papertrail to know what truly is DEBUG info vs INFO info vs logged ERRORs

@manthey
Copy link
Member

manthey commented Apr 8, 2022

I didn't know about that. I'll see if I can give this PR another go to implement that and make sure the changes are more useful/relevant. We'd need to change all logger. to config.getConfig('logger'), right?

I just checked, and outside of the Girder packages, we already are exclusively using the logger from the config. In some girder code we are using the girder logger. The converter utility also uses its own logger rather than the general large-image logger. So, no changes needed for that purpose.

@@ -59,7 +59,7 @@ def createThumbnailsJobTask(item, spec):
except TileGeneralError as exc:
status['failed'] += 1
status['lastFailed'] = str(item['_id'])
logger.info('Failed to get thumbnail for item %s: %r' % (item['_id'], exc))
logger.error('Failed to get thumbnail for item %s: %r' % (item['_id'], exc))
Copy link
Member

Choose a reason for hiding this comment

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

I think when imageKey is present, this should actually do nothing in the exception rather than increment failed and log anything.

@manthey
Copy link
Member

manthey commented Apr 8, 2022

Regarding changing some logging to ERROR level -- let's keep that, though I've commented on one log message that sometimes shouldn't occur; if we don't address that on this PR, we should make an issue so it can be addressed separately.

@manthey
Copy link
Member

manthey commented Apr 27, 2022

I think with #840, we should either close this PR without merging, or just reduce it to changing the non-girder log messages that are being promoted to error level (which I think is just one in the gdal source).

@banesullivan
Copy link
Contributor Author

I'm fine just closing this.

My qualm was that ANY logs from large-image were being captured by sentry.io for django-large-image. So I just did this to mitigate it: girder/django-large-image@13273f5

@manthey manthey deleted the patch/debug-logs branch July 25, 2022 19:42
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.

Change log level to debug where it makes sense
2 participants