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 #2019

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

jimleroyer
Copy link
Member

@jimleroyer jimleroyer commented Nov 9, 2023

Summary | Résumé

In an effort to downsize the size of logs we send from our FluentBit agent to CloudWatch, 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. I've also set a statsd metric to send this data instead, as this would be more reliable and on purpose with statsd.

I tested locally to make sure that the new integration of the statsd_client into the make_task factory function does not crash. I cannot say if the sending of the metrics will work yet until it hits staging, I am confident it will, but the local development environment has no statsd support.

Test instructions | Instructions pour tester la modification

  • Run locally and verify that nothing crashes.
  • Verify that the log output are invisible in staging environment.
  • Verify that the new statsd sent metric is sent in staging environment (available via the CloudWatch metrics console).

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 changed the title Move celery INFO-level logs to reduce log size Move celery some verbose INFO-level logs to reduce log size Nov 10, 2023
@jimleroyer jimleroyer changed the title Move celery some verbose INFO-level logs to reduce log size Move some verbose celery INFO-level logs to reduce log size Nov 10, 2023
@jimleroyer jimleroyer requested a review from jzbahrai November 14, 2023 21:10
Copy link
Collaborator

@jzbahrai jzbahrai left a comment

Choose a reason for hiding this comment

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

ran this locally and it all worked

app.logger.info("{task_name} took {time}".format(task_name=self.name, time="{0:.4f}".format(elapsed_time)))
task_name = self.name
now = time.time()
statsd_client.timing_with_dates(f"celery-task.{task_name}.total-time", now, self.start)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes sense, but yes lets test in staging based on what you said yes

@jimleroyer jimleroyer merged commit a5cab2a into main Nov 14, 2023
4 checks passed
@jimleroyer jimleroyer deleted the feat/reduce-celery-info-logs branch November 14, 2023 21:26
sastels added a commit that referenced this pull request Nov 14, 2023
jzbahrai pushed a commit that referenced this pull request Nov 14, 2023
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