-
Notifications
You must be signed in to change notification settings - Fork 18
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
Rework callback logging #2078
Rework callback logging #2078
Conversation
service_callback_url, | ||
response.status_code, | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to log the response here if there's an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @sastels for the long delayed response. I am revisiting this. From what I read here, an error seems to raise a RequestException
and would then be logged as a warning, hence covering the response status. Most likely the status code would be contained within the e
itself I assume. Do you think my read of the code makes sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's a good point 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a good idea. when the code style test is passing!
Can we get this PR merged? |
Summary | Résumé
To get the callback logged when not only successful, I moved the callback level log to prior to the call. This removes the response status from that logging but that would get logged anyway via the error when it is logged as an error when an error is raised and caught.
This might not be that useful in production unfortunately because the latter does not have the logging level set to INFO: we wouldn't see this message to help us on support. But that will be useful when we do enable it temporarily or in other environments where we want to get this debugged more easily.
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur
This is a suggested checklist of questions reviewers might ask during their
review | Voici une suggestion de liste de vérification comprenant des questions
que les réviseurs pourraient poser pendant leur examen :
une baisse de la quantité de code couvert par les tests automatisés?
fonctionnalité existante?
modification de la politique de confidentialité?
préoccupations liées à la sécurité?
façon importante la performance?
risque d’utiliser des dépendances ajoutées?
setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce
changement (fichier README, etc.)?