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

Move some verbose celery INFO-level logs to reduce log size #2060

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

jimleroyer
Copy link
Member

@jimleroyer jimleroyer commented Dec 6, 2023

Summary | Résumé

Because we want to lower AWS CF costs and increase CW agent bandwidth on the K8s pods, this PR contains changes to demote redundant or unnecessary log messages currently set at INFO level into DEBUG level.

  • The list of notifications IDs that are processed and about to be sent to providers.
  • The notification IDs that are saved into the database.
  • The Celery task execution time: this one can be sensitive if we have a dashboard that builds a metric on top of this. I verified and didn't see any but that would require double verifications from a team members.
  • Bounce rate debug data.

This is a repeat of reverted PR #2019 but without the new statsd metrics, as this proved to be a breaking change once deployed in the staging environment. Hence this PR contains the safe changes that can have a positive impact on cost and will keep the statsd metrics changes in a different future PR.

Test instructions | Instructions pour tester la modification

  • Verify that the log output are invisible in staging environment.

Release Instructions | Instructions pour le déploiement

None.

Necessary steps to perform before and after the deployment of these changes.
For example, emptying the cache on a feature that changes the cache data
structure in Redis could be mentioned.


Étapes nécessaires à exécuter avant et après le déploiement des changements
introduits par cette proposition. Par exemple, vider la cache suite à des
changements modifiant une structure de données de la cache pourrait être
mentionné.

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 :

  • Is the code maintainable? | Est-ce que le code peut être maintenu?
  • Have you tested it? | L’avez-vous testé?
  • Are there automated tests? | Y a-t-il des tests automatisés?
  • Does this cause automated test coverage to drop? | Est-ce que ça entraîne
    une baisse de la quantité de code couvert par les tests automatisés?
  • Does this break existing functionality? | Est-ce que ça brise une
    fonctionnalité existante?
  • Does this change the privacy policy? | Est-ce que ça entraîne une
    modification de la politique de confidentialité?
  • Does this introduce any security concerns? | Est-ce que ça introduit des
    préoccupations liées à la sécurité?
  • Does this significantly alter performance? | Est-ce que ça modifie de
    façon importante la performance?
  • What is the risk level of using added dependencies? | Quel est le degré de
    risque d’utiliser des dépendances ajoutées?
  • Should any documentation be updated as a result of this? (i.e. README
    setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce
    changement (fichier README, etc.)?

@jimleroyer jimleroyer requested review from sastels and ben851 December 6, 2023 21:51
@jimleroyer jimleroyer requested a review from whabanks December 6, 2023 21:53
Copy link
Contributor

@whabanks whabanks left a comment

Choose a reason for hiding this comment

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

LGTM! Notification sends is a great place to start since it's straightforward and should yield a large reduction in log messages. ✅

@jimleroyer jimleroyer merged commit 11d7f27 into main Dec 7, 2023
4 checks passed
@jimleroyer jimleroyer deleted the feat/reduce-some-logs branch December 7, 2023 14:32
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.

2 participants