Adding more detailed debugging output #342
Conversation
is unable to find the requested model on lookup (perhpas because it i has not started)
skyemeedan
requested review from
DGaffney and
computermacgyver
as code owners
|
Code Climate has analyzed commit d226820 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 86.8% (50% is the threshold). This pull request will bring the total coverage in the repository to 88.2% (0.0% change). View more on Code Climate. |
| self.respond(task.task_package) | ||
| @staticmethod | ||
| def import_model_class(model_class): | ||
| class_name = re.sub(r"(?<!^)(?=[A-Z])", "_", model_class).lower() |
There was a problem hiding this comment.
What would be the source of a model class name where it could even potentially fall afoul of this list? Is it free text entry at some point upstream of here?
|
|
||
| @staticmethod | ||
| def get_client(model_key, options={}): | ||
| r = redis.Redis( |
There was a problem hiding this comment.
You're instantiating like this over and over. Maybe worth having a RedisServer module or something?
| def __init__(self, model_key, options={}): | ||
| self.options = options | ||
| self.queue_name = model_key | ||
| self.datastore = redis.Redis( |
There was a problem hiding this comment.
Again, there's a ton of redis.Redis in this file - feels like this could be DRY'ed up a bit
| def get_task(self, timeout=0): | ||
| item = self.datastore.blpop(self.queue_name, timeout) | ||
| if item: | ||
| task = Task(**json.loads(item[1].decode("utf-8"))) |
There was a problem hiding this comment.
Worth using pydantic validation on the task data maybe?
|
This PR is intended a one line change to give more descriptive errors back to the caller that I needed timpani debugging. I could open a ticket for more general cleanup of shared_model code and move these comments there? |
Seems reasonable - @computermacgyver what do you think? |
|
The items you've noted @DGaffney are good points --- let's not those in new JIRA tickets. I think we don't need to address them in this PR, however, since they are preexisting and unchanged except for whitespace. |
There was a problem hiding this comment.
Let's please make this two separate PRs. We should make the one line change and merge that without any further whitespace changes. I'm concerned otherwise that git blame will not work well and we'll confuse ourselves in the future about why we edited lines xyz.
Separately, we could have a different PR that fixes whitespace across the whole project?
|
Was planning to open a clean PR but accidentally committed to |
adding more detailed error debugging output in the response when alegre is unable to find the requested model on lookup (perhaps because skye forgot to start it in the local dev ..)
Note: my editor is applying an opinionated formatter, which has adjusted whitespace to python standards. The actual change is on line 64, adding:
Description
Please include a very brief high-level description of the problem or feature and technical details or specific code changes on PR
Reference: TICKET-ID (to provide additional context)
How has this been tested?
Has it been tested locally?
I confirmed that the alegre system ran locally and gave the desired error response
Are there automated tests?
none added
Have you considered secure coding practices when writing this code?
This does return additional detail on an error, which can leak info about internal state of the system, however this is an internal service API