-
Notifications
You must be signed in to change notification settings - Fork 0
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
🔊 add celery logging #87
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 85.62% 92.76% +7.14%
==========================================
Files 8 13 +5
Lines 153 304 +151
==========================================
+ Hits 131 282 +151
Misses 22 22 ☔ View full report in Codecov by Sentry. |
There is currently a If we used I think there are two ways to handle this:
|
@annashamray do you think it is fine to CELERY_LOGLEVEL? Or should I create a separate logging for celery? |
I don't mind your solution of removing CELERY_LOGLEVEL @stevenbal What do you thing? |
@annashamray might be worth asking Sjoerd, but I'd also be fine with removing |
@stevenbal I did ask Sjoerd and he would like both like open-forms.
This does require creating separate loggers for CELERY. |
if it's easy to make it consistent with OF, then let's do that. Though I don't see a specific logger for celery in OF? |
1ec0fa4
to
ce1ff80
Compare
Added the new loggers for console and file. The way its currently implemented in projects:
My current approach is to add back the CELERY_LOGLEVEL envvar and create separate celerly loggers, but we can also use the open-forms approach. It depends of if the Django console is used for debugging celery |
@annashamray @stevenbal can you re-review the new changes and my above comment? Thanks |
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.
Thank you for the research
I have no strong opinion how to configure Celery logs (probably because I don't do devops tasks), so for me all options look fine.
Your approach with logging to the separate file is good!
@@ -507,6 +527,11 @@ | |||
"level": "DEBUG", | |||
"propagate": True, | |||
}, | |||
"celery": { |
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.
While this will add the logs of the celery library itself, does the logging that happens inside celery tasks still work as well? Because from Sjoerd's issue I see that there is no logging at all. The regular logging is picked up by the project handler i think, even if it happens inside the celery worker
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.
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.
It does need LOG_STDOUT
to be enabled though
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.
Check, I think it's fine to accept these changes then. I do think that the logs no longer showing has to do with either LOG_STDOUT
or LOG_LEVEL
, so I've asked Sjoerd to see what the values of those are open-zaak/open-zaak#1802 (comment)
Also note, with these changes, CELERY ignores setting the log level on the command line. edit: this can be fixed in Perhaps there should be a common celery.py or celery functions in utils? |
open_api_framework/conf/base.py
Outdated
@@ -415,6 +422,19 @@ | |||
"class": "logging.StreamHandler", | |||
"formatter": "db", | |||
}, | |||
"console_celery": { |
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.
A note to rename one or the other
Example of above
Really depends on what we want to allow. Also, should this change the |
Discussed with Conor, and as the default was that there was output on stdout lets set LOG_STDOUT to True by default. In Docker and K8s installs you want logging on stdout and that is our default way of deployment, if someone doesn't want this they can turn it off via this envvar |
Partially fixes: maykinmedia/objects-api#470
Partially fixes: open-zaak/open-zaak#1802
Needs OAF release