fix(log-detective): do not log missing run as error#3136
Conversation
Fixes packit#3135 Signed-off-by: Matej Focko <mfocko@packit.dev>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the logging level from error to info in parse_logdetective_analysis_event when a Log Detective analysis ID is received but no matching analysis was requested. However, according to the repository style guide, parser functions that return None because specific conditions are not met must log a warning. The reviewer has requested changing this log level to warning instead of info to comply with this guideline.
| logger.info( | ||
| f"Received results Log Detective analysis: '{analysis_id}' " | ||
| "but no analysis with this id was requested." | ||
| ) |
There was a problem hiding this comment.
According to the Repository Style Guide (lines 537-539), when a parser returns None because the event doesn't satisfy specific conditions, a warning must be logged. Changing this log level to info violates this guideline. Please use logger.warning instead.
| logger.info( | |
| f"Received results Log Detective analysis: '{analysis_id}' " | |
| "but no analysis with this id was requested." | |
| ) | |
| logger.warning( | |
| f"Received results Log Detective analysis: '{analysis_id}' " | |
| "but no analysis with this id was requested." | |
| ) |
References
- Parsers process dictionaries and must either return a parsed object, or None, if the dictionary doesn't satisfy conditions specific to the event. In cases when parser returns None, a warning must be logged. (link)
There was a problem hiding this comment.
This was not true for this parser even before I adjusted this one instance…
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 55s |
+1 this makes a lot of sense @mfocko |
4 participants
I have to admit, I don't really like the fact that we're touching database from within the parser. Parser should be totally stateless… I would move this check to the beginning of the handler, if there are no objections to it.
Fixes #3135
Cc @Jany26