NOREF-Better error handling & logging #365
Conversation
managers/oclc_catalog.py
Outdated
| @@ -96,17 +96,20 @@ def _get_other_editions(self, oclc_number: int, offset: int=0): | |||
| ) | |||
|
|
|||
| if other_editions_response.status_code != 200: | |||
| logger.warning(f'OCLC other editions request failed with status {other_editions_response.status_code}') | |||
| logger.warning( | |||
| f"Other editions query for no {oclc_number} failed " | |||
There was a problem hiding this comment.
nit: let's use single quotation marks
managers/oclc_catalog.py
Outdated
| logger.warning( | ||
| f"Other editions query for no {oclc_number} failed " | ||
| f"With status {other_editions_response.status_code} " | ||
| f"and reason {other_editions_response.json()['type']}") |
There was a problem hiding this comment.
I think we should be careful with getting the json body. If it's a 500 error, there may not be a json body and we may error out here.
managers/oclc_catalog.py
Outdated
| except Exception as e: | ||
| logger.error(f'Failed to query other editions endpoint {other_editions_url}', e) | ||
| except Exception as e: | ||
| logger.error(f'Failed to query other editions endpoint {other_editions_url}: Exception: {e}') |
There was a problem hiding this comment.
We may not get the entire stack trace here, which may be helpful to debug.
managers/oclc_catalog.py
Outdated
| @@ -133,7 +136,7 @@ def query_bibs(self, query: str): | |||
|
|
|||
| return bibs | |||
| except Exception as e: | |||
| logger.error(f'Failed to query search bibs with query {query}', e) | |||
| logger.error(f'Failed to query search bibs with query {query}. Exception: {e}') | |||
processes/oclcClassify.py
Outdated
| search_query = self.oclc_catalog_manager.generate_search_query(identifier, identifier_type, title, author) | ||
|
|
||
| try: | ||
| search_query = self.oclc_catalog_manager.generate_search_query(identifier, identifier_type, title, author) |
There was a problem hiding this comment.
hmm are we erring out when we generate the search query?
There was a problem hiding this comment.
In main, if "generate search query" returns None because a search query can't be built, the code still sends the (empty) search query to OCLC and then fails with a warning level log and continues. I'm open to how exceptions should be handled but I do think we should avoid sending empty queries to OCLC - very flexible about how or where we do that!
There was a problem hiding this comment.
I'm going to try a different rework
| @@ -19,17 +24,17 @@ def addDCDWToUpdateList(self, rec): | |||
| existing = self.session.query(Record)\ | |||
| .filter(Record.source_id == rec.record.source_id).first() | |||
| if existing: | |||
| print('EXISTING', existing) | |||
| logger.debug('Existing record: ' + str(existing)) | |||
There was a problem hiding this comment.
nice! this is much needed haha
| f'OCLC search bibs request for OCLC no {oclc_number} failed ' | ||
| f'with status {other_editions_response.status_code}') | ||
| try: | ||
| oclc_error_type = other_editions_response.json()["type"] |
There was a problem hiding this comment.
I think we can do get('type') here instead to just guard.
There was a problem hiding this comment.
I think json() could still throw a JSONDecodeError before the get() is run on type, but it could be split out such that the exception is narrowed to not include both the JSONDecodeError and the KeyError here - do you think that would be better?
There was a problem hiding this comment.
(The possibilities including: the response not having valid JSON, and the response having valid JSON but it might not contain a type)
There was a problem hiding this comment.
But we aren't handling the TypeError right? I think using get would prevent the KeyError.
Todo: